Re: [RFC PATCH v3 4/5] input: add onkey driver for Marvell 88PM886 PMIC
On Mon, Mar 11, 2024 at 11:26:16AM +0100, 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
> > >
> > > 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
> > > ---
> > >
> > > 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:" 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
> 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
> to me to effectively be the same setup as with my driver and that does
> not load automatically (because of the missing alias).
Yes, ioc3kbd is broken as far as module auto-loading goes. It probably
did not get noticed before because the driver is likely to be built-in
on the target architecture.
I'll take patches.
Thanks.
--
Dmitry
Re: [RFC PATCH v3 4/5] input: add onkey driver for Marvell 88PM886 PMIC
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
> >>>
> >>> 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
> >>> ---
> >>>
> >>> 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:" 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
Re: [RFC PATCH v3 4/5] input: add onkey driver for Marvell 88PM886 PMIC
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
>>>
>>> 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
>>> ---
>>>
>>> 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:" 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?
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. Probing is done also by driver name. Auto-loading,
not sure, maybe by name as well? However it is also likely that
auto-loading is broken. Several drivers had such issues in the past.
Best regards,
Krzysztof
Re: [RFC PATCH v3 4/5] input: add onkey driver for Marvell 88PM886 PMIC
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
> >
> > 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
> > ---
> >
> > 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:" 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
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
to me to effectively be the same setup as with my driver and that does
not load automatically (because of the missing alias).
> Just run `modinfo`.
Thank you very much,
K. B.
Re: [RFC PATCH v3 4/5] input: add onkey driver for Marvell 88PM886 PMIC
On Sun, Mar 10, 2024 at 09:35:36PM +0100, Krzysztof Kozlowski wrote:
> 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
> >
> > 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
> > ---
> >
> > 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:" 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.
> Just run `modinfo`.
MODULE_DEVICE_TABLE() and MODULE_ALIAS() reduce to the same thing, but I
agree that we should not try to be too clever and simply use the ID
table.
Thanks.
--
Dmitry
Re: [RFC PATCH v3 4/5] input: add onkey driver for Marvell 88PM886 PMIC
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
>
> 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
> ---
>
> 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:" 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.
Just run `modinfo`.
Best regards,
Krzysztof
Re: [RFC PATCH v3 4/5] input: add onkey driver for Marvell 88PM886 PMIC
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
> > > >
> > > > 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
> > > > ---
> > > >
> > > > 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:" 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?
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`...
Thank you, best regards,
K. B.
Re: [RFC PATCH v3 4/5] input: add onkey driver for Marvell 88PM886 PMIC
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 > > > > > > 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 > > > --- > > > > > > 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:" 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/...). Thanks. -- Dmitry
Re: [RFC PATCH v3 4/5] input: add onkey driver for Marvell 88PM886 PMIC
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 > > > > 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 > > --- > > > > 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? Thank you, kind regards, K. B.
Re: [RFC PATCH v3 4/5] input: add onkey driver for Marvell 88PM886 PMIC
Hi Karel, On Sun, Mar 03, 2024 at 11:04:25AM +0100, Karel Balej wrote: > From: Karel Balej > > 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 > --- > > 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. With that fixed: Acked-by: Dmitry Torokhov Thanks. -- Dmitry
