Re: [PATCH 1/2] power: act8945a: add charger driver for ACT8945A

[Javier Martinez Canillas]

Hello Wenyou,

On Tue, Jan 12, 2016 at 5:23 AM, Yang, Wenyou <Wenyou.Yang@xxxxxxxxx> wrote:
> Hi Javier,
>
> Thank you for your review.
>
>> -----Original Message-----
>> From: Javier Martinez Canillas [mailto:javier@xxxxxxxxxxxx]
>> Sent: 2016å1æ8æ 23:00
>> To: Yang, Wenyou <Wenyou.Yang@xxxxxxxxx>
>> Cc: Sebastian Reichel <sre@xxxxxxxxxx>; Dmitry Eremin-Solenikov
>> <dbaryshkov@xxxxxxxxx>; David Woodhouse <dwmw2@xxxxxxxxxxxxx>; Rob
>> Herring <robh+dt@xxxxxxxxxx>; Pawel Moll <pawel.moll@xxxxxxx>; Mark
>> Rutland <mark.rutland@xxxxxxx>; Ian Campbell <ijc+devicetree@xxxxxxxxxxxxxx>;
>> Kumar Gala <galak@xxxxxxxxxxxxxx>; Ferre, Nicolas
>> <Nicolas.FERRE@xxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Linux
>> Kernel <linux-kernel@xxxxxxxxxxxxxxx>; linux-pm@xxxxxxxxxxxxxxx
>> Subject: Re: [PATCH 1/2] power: act8945a: add charger driver for ACT8945A
>>
>> Hello Wenyou,
>>
>> On Thu, Jan 7, 2016 at 11:13 PM, Wenyou Yang <wenyou.yang@xxxxxxxxx>
>> wrote:
>> > This patch adds new driver for Active-semi ACT8945A ActivePath charger
>> > (part of ACT8945A MFD driver) providing power supply class information
>> > to userspace.
>> >
>> > The driver is configured through DTS (battery and system related
>> > settings) and sysfs entries (timers and input over-voltage threshold).
>> >
>> > Signed-off-by: Wenyou Yang <wenyou.yang@xxxxxxxxx>
>> > ---
>> >
>>
>> [snip]
>>
>> > +
>> > +static struct platform_driver act8945a_charger_driver = {
>> > +       .driver = {
>> > +               .name = "act8945a-charger",
>> > +       },
>> > +       .probe  = act8945a_charger_probe,
>> > +       .remove = act8945a_charger_remove, };
>> > +
>>
>> The driver is missing an OF match table (and also a platform ID match table if
>> non-DT is supported) so this means that module autoloading will be broken for this
>> driver since the platform uevent function will report a MODALIAS=of:N*T*Cactive-
>> semi,act8945a-charger
>
> This is an MFD device, it compatible is declared in the ../mfd/act8945a.c file.
>
>         .of_compatible = "active-semi,act8945a-charger".
>

I know that the device is instantiated from a MFD driver but having a
.of_compatible property in the MFD cell, it only means that the
pdev->dev.of_node field will be filled by the mfd-core [0]. It has
nothing to do with how the platform device is matched to the platform
driver.

The mfd-core calls platform_device_add() to add the device so the
platform bus_type .match function callback [1] will be used to match
the device with the driver and since the driver doesn't have a OF
device ID table to match, it means that this function will fallback to
match the device name with the platform driver name.

Also, you didn't answer my question about module auto-loading. The
platform bus_type .uevent callback calls of_device_uevent_modalias()
and since the device has an .of_node filled by the mfd-core (since you
provided an .of_compatible for the MFD cell), this means that an OF
modalias will be reported but since the driver doesn't have neither a
.of_match_table nor a MODULE_DEVICE_TABLE(of,...) so udev/kmod won't
be able to load the driver's module based on the modalias uevent
reported by the kernel.

> The non-DT  is not supported in this patch. So a platform ID match table is not included.
>

Ok, I just asked for completeness.

[0]: http://lxr.free-electrons.com/source/drivers/mfd/mfd-core.c#L177
[1]: http://lxr.free-electrons.com/source/drivers/base/platform.c#L833
[2]: http://lxr.free-electrons.com/source/drivers/base/platform.c#L787

Best regards,
Javier