Re: [PATCH 5/8] platform/x86: x86-android-tablets: Create a platform_device from module_init()

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 9/10/23 10:07, Andy Shevchenko wrote:
> On Sat, Sep 9, 2023 at 5:18 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Create a platform_device from module_init() and change
>> x86_android_tablet_init() / cleanup() into platform_device
>> probe() and remove() functions.
>>
>> This is a preparation patch for refactoring x86_android_tablet_get_gpiod()
>> to no longer use gpiolib private functions like gpiochip_find().
> 
> ...
> 
>> +static int __init x86_android_tablet_init(void)
>> +{
> 
>> +       if (!dmi_first_match(x86_android_tablet_ids)) {
> 
> Why do we need this? Module alias is based on DMI matching, is it
> against manual loading?

Yes I added this to protect against manual loading.

> 
>> +               pr_err("error loaded on unknown tablet model\n");
>> +               return -ENODEV;
>> +       }
>> +
>> +       x86_android_tablet_device = platform_create_bundle(&x86_android_tablet_driver,
>> +                                                  x86_android_tablet_probe,
>> +                                                  NULL, 0, NULL, 0);
> 
> So, in case of manual loading, would it be harmful for non-listed platforms?

No this will not be harmful, x86_android_tablet_probe() also
checks the DMI table and it will return -ENODEV when there is
no match.

So we just end up with an unused x86_android_tablets platform-device
and otherwise no harm is done.

I guess my main reason here is to not change manual loading
behavior, before the entire insmod would fail since
module_init() would return -ENODEV, this preserves this
behavior.

But you are right that this check can be dropped without
any bad side-effects.

I'll drop the check before merging this.

> 
>> +       return PTR_ERR_OR_ZERO(x86_android_tablet_device);
>> +}
>> +
>> +static void __exit x86_android_tablet_exit(void)
>> +{
>> +       platform_device_unregister(x86_android_tablet_device);
>> +       platform_driver_unregister(&x86_android_tablet_driver);
>> +}
> 
>> +
> 
> Instead of adding this blank line you can move
> module_init()/module_exit() closer to the respective callbacks.

Ack, I'll fix this before merging this.

Regards,

Hans



> 
>>  module_init(x86_android_tablet_init);
>> -module_exit(x86_android_tablet_cleanup);
>> +module_exit(x86_android_tablet_exit);
>