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 ?
> +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.
> +
> +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.
> +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).
> +
> + 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.
> +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.
> +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.
> +MODULE_LICENSE("GPL");
Regards,
Hans
