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
>
