Re: [RFC PATCH v3 4/5] input: add onkey driver for Marvell 88PM886 PMIC

[Karel Balej]

Krzysztof Kozlowski, 2024-03-11T11:41:53+01:00:
> On 11/03/2024 11:26, Karel Balej wrote:
> > Krzysztof Kozlowski, 2024-03-10T21:35:36+01:00:
> >> On 10/03/2024 12:35, Karel Balej wrote:
> >>> Dmitry Torokhov, 2024-03-04T17:10:59-08:00:
> >>>> On Mon, Mar 04, 2024 at 09:28:45PM +0100, Karel Balej wrote:
> >>>>> Dmitry,
> >>>>>
> >>>>> Dmitry Torokhov, 2024-03-03T12:39:46-08:00:
> >>>>>> On Sun, Mar 03, 2024 at 11:04:25AM +0100, Karel Balej wrote:
> >>>>>>> From: Karel Balej <balejk@xxxxxxxxx>
> >>>>>>>
> >>>>>>> Marvell 88PM886 PMIC provides onkey among other things. Add client
> >>>>>>> driver to handle it. The driver currently only provides a basic support
> >>>>>>> omitting additional functions found in the vendor version, such as long
> >>>>>>> onkey and GPIO integration.
> >>>>>>>
> >>>>>>> Signed-off-by: Karel Balej <balejk@xxxxxxxxx>
> >>>>>>> ---
> >>>>>>>
> >>>>>>> Notes:
> >>>>>>>     RFC v3:
> >>>>>>>     - Drop wakeup-source.
> >>>>>>>     RFC v2:
> >>>>>>>     - Address Dmitry's feedback:
> >>>>>>>       - Sort includes alphabetically.
> >>>>>>>       - Drop onkey->irq.
> >>>>>>>       - ret -> err in irq_handler and no initialization.
> >>>>>>>       - Break long lines and other formatting.
> >>>>>>>       - Do not clobber platform_get_irq error.
> >>>>>>>       - Do not set device parent manually.
> >>>>>>>       - Use input_set_capability.
> >>>>>>>       - Use the wakeup-source DT property.
> >>>>>>>       - Drop of_match_table.
> >>>>>>
> >>>>>> I only said that you should not be using of_match_ptr(), but you still
> >>>>>> need to have of_match_table set and have MODULE_DEVICE_TABLE() for the
> >>>>>> proper module loading support.
> >>>>>
> >>>>> I removed of_match_table because I no longer need compatible for this --
> >>>>> there are no device tree properties and the driver is being instantiated
> >>>>> by the MFD driver.
> >>>>>
> >>>>> Is the MODULE_DEVICE_TABLE() entry needed for the driver to probe when
> >>>>> compiled as module? If that is the case, given what I write above, am I
> >>>>> correct that MODULE_DEVICE_TABLE(platform,...) would be the right thing
> >>>>> to use here?
> >>>>
> >>>> Yes, if uevent generated for the device is "platform:<name>" then
> >>>> MODULE_DEVICE_TABLE(platform,...) will suffice. I am not sure how MFD
> >>>> sets it up (OF modalias or platform), but you should be able to check
> >>>> the format looking at the "uevent" attribute for your device in sysfs
> >>>> (/sys/devices/bus/platform/...). 
> >>>
> >>> The uevent is indeed platform.
> >>>
> >>> But since there is only one device, perhaps having a device table is
> >>> superfluous and using `MODULE_ALIAS("platform:88pm886-onkey")` is more
> >>> fitting?
> >>
> >> Adding aliases for standard IDs and standard cases is almost never
> >> correct. If you need module alias, it means your ID table is wrong (or
> >> missing, which is usually wrong).
> >>
> >>>
> >>> Although I don't understand why this is even necessary when the driver
> >>> name is such and the module is registered using
> >>> `module_platform_driver`...
> >>
> >> ID table and MODULE_DEVICE_TABLE() are necessary for modprobe to work.
> > 
> > I think I understand the practical reasons. My point was that I would
> > expect the alias to be added automatically even in the case that the
> > device table is absent based solely on the driver name and the
> > registration method (*module*_*platform*_driver). Why is that not the
> > case? Obviously the driver name matching the mfd_cell name is sufficient
>
> You mean add it automatically by macro-magic based on presence of
> id_table and/or of_match_table?

Yes, that plus: if id_table is present, use that for module aliases,
otherwise use driver name. In fact, I checked the platform core and it
seems to proceed in exactly this way when determining whether there is a
match. And while autoloading and probing are two different things, I
assume that we always want to consider a module for autoloading when we
know that it will probe because we have a compatible device.

> That's a good question. I cannot find answer why not, except that maybe
> no one ever wrote it...
>
>
> > for the driver to probe when it is built in so the name does seem to
> > serve as some identification for the device just as a device table entry
> > would.
> > 
> > Furthermore, drivers/input/serio/ioc3kbd.c does not seem to have an ID
> > table either, nor a MODULE_ALIAS -- is that a mistake? If not, what
> > mechanism causes the driver to probe when compiled as a module? It seems
>
> You are now mixing two different things: probing of driver (so bind) and
> module auto-loading.

Yes, sorry, I meant autoloading.

> Probing is done also by driver name. Auto-loading,
> not sure, maybe by name as well?

Well probably not, otherwise it would work here too, no? Unless there
are some fundamental differences in this between PCI and platform
drivers. But the input driver is platform too and is required through
the MFD cell, so I think it should be the same scenario.

> However it is also likely that
> auto-loading is broken. Several drivers had such issues in the past.

OK, I see, thank you. I think this was the main source of my confusion
because I looked at other drivers for reference when trying to
understand which properties (name/device table) are necessary for what
action (probing/autoloading).

> Best regards,
> Krzysztof

Thank you again, kind regards,
K. B.