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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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@matfyz.cz>
>>>>>
>>>>> 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@matfyz.cz>
>>>>> ---
>>>>>
>>>>> 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.
Just run `modinfo`.

Best regards,
Krzysztof