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

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

Hi again,

On 9/17/22 08:52, Huacai Chen wrote:
> From: Jianmin Lv <lvjianmin@loongson.cn>
> 
> 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@loongson.cn>
> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> ---
> 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@loongson.cn>");
> +MODULE_AUTHOR("Huacai Chen <chenhuacai@loongson.cn>");
> +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