Re: [PATCH V3] LoongArch: Add ACPI-based generic laptop driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi, Hans,

On Sat, Sep 17, 2022 at 6:00 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi again,
>
> On 9/17/22 08:52, Huacai Chen wrote:
> > From: Jianmin Lv <lvjianmin@xxxxxxxxxxx>
> >
> > This add ACPI-based generic laptop driver for Loongson-3. Some of the
> > codes are derived from drivers/platform/x86/thinkpad_acpi.c.
> >
> > Signed-off-by: Jianmin Lv <lvjianmin@xxxxxxxxxxx>
> > Signed-off-by: Huacai Chen <chenhuacai@xxxxxxxxxxx>
> > ---
> > V2: Fix problems pointed out by Arnd.
> > V3: Use platform driver instead of acpi driver.
>
> A couple more notes which I noticed just after sending my previous email:
>
> > +#define ACPI_LAPTOP_VERSION "1.0"
> > +#define ACPI_LAPTOP_NAME "loongson-laptop"
> > +#define ACPI_LAPTOP_DESC "Loongson Laptop/all-in-one ACPI Driver"
> > +#define ACPI_LAPTOP_FILE ACPI_LAPTOP_NAME "_acpi"
> > +#define ACPI_LAPTOP_DRVR_NAME ACPI_LAPTOP_FILE
> > +#define ACPI_LAPTOP_ACPI_EVENT_PREFIX "loongson"
>
> Do you really need / use all these defines ?
All unneeded macros will be removed, thanks.

>
> > +static const struct acpi_device_id loongson_htk_device_ids[] = {
> > +     {LOONGSON_ACPI_HKEY_HID, 0},
> > +     {"", 0},
> > +};
>
> You will want to put a:
>
> MODULE_DEVICE_TABLE(acpi, loongson_htk_device_ids);
>
> line here for proper automatic loading when build as a module.
OK, thanks.

>
> > +
> > +static struct platform_driver loongson_hotkey_driver = {
> > +     .probe          = loongson_hotkey_probe,
> > +     .driver         = {
> > +             .name   = "loongson-hotkey",
> > +             .owner  = THIS_MODULE,
> > +             .pm     = pm_ptr(&loongson_hotkey_pm),
> > +             .acpi_match_table = ACPI_PTR(loongson_htk_device_ids),
>
> Since you unconditionally define loongson_htk_device_ids above;
> and since you have a "depends on ACPI" in your Kconfig, you can drop
> the ACPI_PTR() here, just use loongson_htk_device_ids directly.
OK, thanks.

>
> > +static int __init generic_acpi_laptop_init(void)
> > +{
> > +     bool ec_found;
> > +     int i, ret, status;
> > +
> > +     if (acpi_disabled)
> > +             return -ENODEV;
> > +
> > +     /* The EC device is required */
> > +     ec_found = acpi_dev_found(LOONGSON_ACPI_EC_HID);
> > +     if (!ec_found)
> > +             return -ENODEV;
> > +
> > +     /* Enable SCI for EC */
> > +     acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1);
> > +
> > +     generic_inputdev = input_allocate_device();
> > +     if (!generic_inputdev) {
> > +             pr_err("Unable to allocate input device\n");
> > +             return -ENOMEM;
> > +     }
> > +
> > +     /* Prepare input device, but don't register */
> > +     generic_inputdev->name =
> > +             "Loongson Generic Laptop/All-in-one Extra Buttons";
> > +     generic_inputdev->phys = ACPI_LAPTOP_DRVR_NAME "/input0";
> > +     generic_inputdev->id.bustype = BUS_HOST;
> > +     generic_inputdev->dev.parent = NULL;
> > +
> > +     /* Init subdrivers */
> > +     for (i = 0; i < ARRAY_SIZE(generic_sub_drivers); i++) {
> > +             ret = generic_subdriver_init(&generic_sub_drivers[i]);
> > +             if (ret < 0) {
> > +                     input_free_device(generic_inputdev);
> > +                     return ret;
> > +             }
> > +     }
>
> I see above that you have only 1 subdriver. Do you expect there to be
> more in the future ?  If not then it would be better to just completely
> remove the subdriver abstraction and simply do everything directly
> from the main probe/remove functions (see below).
At this time we only add the most basic subdriver, and more subdrivers
will be added, so I want to keep it here.

>
> > +
> > +     ret = input_register_device(generic_inputdev);
> > +     if (ret < 0) {
> > +             input_free_device(generic_inputdev);
> > +             pr_err("Unable to register input device\n");
> > +             return ret;
> > +     }
> > +
> > +     input_device_registered = 1;
> > +
> > +     if (acpi_evalf(hotkey_handle, &status, "ECBG", "d")) {
> > +             pr_info("Loongson Laptop used, init brightness is 0x%x\n", status);
> > +             ret = laptop_backlight_register();
> > +             if (ret < 0)
> > +                     pr_err("Loongson Laptop: laptop-backlight device register failed\n");
> > +     }
> > +
> > +     return 0;
> > +}
>
> All of generic_acpi_laptop_init should be done from loongson_hotkey_probe()
> and instead of using global variables all data you need should be in a struct
> and that struct should be alloc-ed from loongson_hotkey_probe() and then tied
> to the platform_device using platform_set_drvdata() and retreived on remove
> using platform_get_drvdata() and on suspend/resume using dev_get_drvdata().
>
> > +static void __exit generic_acpi_laptop_exit(void)
> > +{
> > +     if (generic_inputdev) {
> > +             if (input_device_registered)
> > +                     input_unregister_device(generic_inputdev);
> > +             else
> > +                     input_free_device(generic_inputdev);
> > +     }
> > +}
>
> This should be done from a remove function which then gets set as the
> remove callback in loongson_hotkey_driver.
>
> I see at a quick glance that you based this driver on thinkpad_acpi.c
> but that is a very old driver which does a bunch of things in old,
> deprecated ways which are hard to fix for userspace API compatibility
> reasons.
>
> Now a days we try to avoid global variables and also custom
> module_init()/module_exit() functions.
>
> > +module_init(generic_acpi_laptop_init);
> > +module_exit(generic_acpi_laptop_exit);
>
> Once the work of these 2 functions is done from loongson_hotkey_driver.probe /
> loongson_hotkey_driver.remove, you can replace this with:
>
> module_platform_driver(loongson_hotkey_driver);
>
> > +
> > +MODULE_ALIAS("platform:acpi-laptop");
>
> This is not necessary, what you need for autoloading is the:
>
> MODULE_DEVICE_TABLE(acpi, loongson_htk_device_ids);
>
> mentioned above.
OK, thanks.

>
> > +MODULE_AUTHOR("Jianmin Lv <lvjianmin@xxxxxxxxxxx>");
> > +MODULE_AUTHOR("Huacai Chen <chenhuacai@xxxxxxxxxxx>");
> > +MODULE_DESCRIPTION(ACPI_LAPTOP_DESC);
>
> You only use the ACPI_LAPTOP_DESC #define once, please just
> put its contents directly here.
OK, thanks.

>
> > +MODULE_VERSION(ACPI_LAPTOP_VERSION);
>
> Modules having there own versioning separate from the kernel
> is something from the past. Please drop the MODULE_VERSION() line
> and the ACPI_LAPTOP_VERSION #define.
OK, thanks.

Huacai
>
> > +MODULE_LICENSE("GPL");
>
> Regards,
>
> Hans
>



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux