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

[Andy Shevchenko]

On Tue, Nov 24, 2015 at 5:09 PM, Chen-Yu Tsai <wens@xxxxxxxx> wrote:
> On Tue, Nov 24, 2015 at 8:35 PM, Andy Shevchenko
> <andy.shevchenko@xxxxxxxxx> wrote:
>> On Tue, Nov 24, 2015 at 1:28 PM, Chen-Yu Tsai <wens@xxxxxxxx> wrote:
>>> On Tue, Nov 24, 2015 at 5:37 PM, Andy Shevchenko
>>> <andy.shevchenko@xxxxxxxxx> wrote:
>>>> On Tue, Nov 24, 2015 at 5:48 AM, Chen-Yu Tsai <wens@xxxxxxxx> wrote:
>>>>> 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â
>>
>>>>> +       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.

OK.

It would be ideal to move to unified device properties API at some
point, but it's out of scope of this series anyway.

>
>>>>       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?

I would not.

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/