Re: [PATCH 1/2] platform/x86: Add ACPI quickstart button (PNP0C32) driver

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Wed, 31 Jan 2024, Armin Wolf wrote:

> This drivers supports the ACPI quickstart button device, which
> is used to send manufacturer-specific events to userspace.
> Since the meaning of those events is not standardized, userspace
> has to use for example hwdb to decode them.
> 
> The driver itself is based on an earlier proposal, but contains
> some improvements and uses the device wakeup API instead of a
> custom sysfs file.
> 
> Compile-tested only.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

> diff --git a/drivers/platform/x86/quickstart.c b/drivers/platform/x86/quickstart.c
> new file mode 100644
> index 000000000000..ba3a7a25dda7
> --- /dev/null
> +++ b/drivers/platform/x86/quickstart.c
> @@ -0,0 +1,225 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * quickstart.c - ACPI Direct App Launch driver

ACPI Quickstart ?

> + * Copyright (C) 2024 Armin Wolf <W_Armin@gmx.de>
> + * Copyright (C) 2022 Arvid Norlander <lkml@vorapal.se>
> + * Copyright (C) 2007-2010 Angelo Arrifano <miknix@gmail.com>
> + *
> + * Information gathered from disassembled dsdt and from here:
> + * <https://archive.org/details/microsoft-acpi-dirapplaunch>
> + */

> +static void quickstart_notify(acpi_handle handle, u32 event, void *context)
> +{
> +	struct quickstart_data *data = context;
> +
> +	switch (event) {
> +	case QUICKSTART_EVENT_RUNTIME:
> +		sparse_keymap_report_event(data->input_device, 0x1, 1, true);
> +		acpi_bus_generate_netlink_event(DRIVER_NAME, dev_name(data->dev), event, 0);
> +		break;
> +	default:
> +		dev_err(data->dev, FW_INFO "Unexpected ACPI notify event (%u)\n", event);

Could this end up spamming the logs so perhaps use _ratelimited variant?

> +static int quickstart_probe(struct platform_device *pdev)
> +{
> +	struct quickstart_data *data;
> +	acpi_handle handle;
> +	acpi_status status;
> +	int ret;
> +
> +	handle = ACPI_HANDLE(&pdev->dev);
> +	if (!handle)
> +		return -ENODEV;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->dev = &pdev->dev;
> +	dev_set_drvdata(&pdev->dev, data);
> +
> +	/* We have to initialize the device wakeup before evaluating GHID because
> +	 * doing so will notify the device if the button was used to wake the machine
> +	 * from S5.
> +	 */
> +	device_init_wakeup(&pdev->dev, true);
> +
> +	ret = quickstart_get_ghid(data);
> +	if (ret < 0)
> +		return ret;
> +
> +	data->input_device = devm_input_allocate_device(&pdev->dev);
> +	if (!data->input_device)
> +		return -ENOMEM;
> +
> +	ret = sparse_keymap_setup(data->input_device, quickstart_keymap, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	snprintf(data->input_name, sizeof(data->input_name), "Quickstart Button %u", data->id);
> +	snprintf(data->phys, sizeof(data->phys), DRIVER_NAME "/input%u", data->id);
> +
> +	data->input_device->name = data->input_name;
> +	data->input_device->phys = data->phys;

Why not devm_kasprintf() these directly into data->input_device->xx + 
NULL check instead of storing into struct quickstart_data ?

> +static struct platform_driver quickstart_platform_driver = {
> +	.driver	= {
> +		.name = DRIVER_NAME,
> +		.dev_groups = quickstart_groups,
> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> +		.acpi_match_table = quickstart_device_ids,
> +	},
> +	.probe = quickstart_probe,
> +};
> +module_platform_driver(quickstart_platform_driver);
> +
> +MODULE_AUTHOR("Armin Wolf <W_Armin@gmx.de>");
> +MODULE_AUTHOR("Arvid Norlander <lkml@vorpal.se>");
> +MODULE_AUTHOR("Angelo Arrifano");
> +MODULE_DESCRIPTION("ACPI Direct App Launch driver");

ACPI Quickstart Button ?

> +MODULE_LICENSE("GPL");
> --
> 2.39.2
> 

-- 
 i.