[PATCH v4 2/6] mfd: axp20x: Split the driver into core and i2c bits

On Tue, Nov 24, 2015 at 8:35 PM, Andy Shevchenko
[off-list ref] wrote:

On Tue, Nov 24, 2015 at 1:28 PM, Chen-Yu Tsai [off-list ref] wrote:

quoted

Hi,

On Tue, Nov 24, 2015 at 5:37 PM, Andy Shevchenko
[off-list ref] wrote:

quoted

On Tue, Nov 24, 2015 at 5:48 AM, Chen-Yu Tsai [off-list ref] wrote:

quoted

The axp20x driver assumes the device is i2c based. This is not the
case with later chips, which use a proprietary 2 wire serial bus
by Allwinner called "Reduced Serial Bus".

This patch follows the example of mfd/wm831x and splits it into
an interface independent core, and an i2c specific glue layer.
MFD_AXP20X and the new MFD_AXP20X_I2C are changed to tristate
symbols, allowing the driver to be built as modules.

Included but unused header files are removed as well.

So?

quoted

quoted

quoted

+       if (dev->of_node) {

What about

if (?of_node) {
      const struct of_device_id *id;
?
} else if ACPI_COMPANION(?) {

This should be has_acpi_companion().

I don't think the "else if" is necessary. There's only 2 possible ways
the device gets probed, either device tree or ACPI.

quoted

quoted

      const struct acpi_device_id *id;
?
} else {
 return -ENODEV;
}

I really don't want to change code that I'm just moving around.
Same goes for the other comments about this patch. I can do another
patch on top of this to fix the style issues if it really bothers
people.

Fair enough.
My comments mostly about unnecessity of second parameter in the functions.

So,  you already did some clean up in this patch (above), what about
to do another? I also prefer separate patch *before* you do a split.

Sure. I'll do a patch or 2 before the split. Would you mind if I add your
Suggested-by tag?


Regards
ChenYu

quoted

quoted

quoted

+       axp20x = devm_kzalloc(&i2c->dev, sizeof(*axp20x), GFP_KERNEL);
+       if (!axp20x)
+               return -ENOMEM;
+
+       ret = axp20x_i2c_match_device(axp20x, &i2c->dev);
+       if (ret)
+               return ret;
+
+       axp20x->dev = &i2c->dev;
+       axp20x->irq = i2c->irq;

If you move _match_device() here you will be able to drop away struct
device * parameter.

--
With Best Regards,
Andy Shevchenko