Re: [PATCH v3] platform/x86: Add driver for ACPI WMAA EC-based backlight control

[Daniel Dadap <ddadap@nvidia.com>]

Thanks again:

On 8/24/21 5:47 PM, Barnabás Pőcze wrote:
> Hi
>
>
> 2021. augusztus 25., szerda 0:04 keltezéssel, Daniel Dadap írta:
>> A number of upcoming notebook computer designs drive the internal
>> display panel's backlight PWM through the Embedded Controller (EC).
>> This EC-based backlight control can be plumbed through to an ACPI
>> "WMAA" method interface, which in turn can be wrapped by WMI with
>> the GUID handle 603E9613-EF25-4338-A3D0-C46177516DB7.
>>
>> Add a new driver, aliased to the WMAA WMI GUID, to expose a sysfs
>> backlight class driver to control backlight levels on systems with
>> EC-driven backlights.
>>
>> Signed-off-by: Aaron Plattner <aplattner@nvidia.com>
>> Signed-off-by: Daniel Dadap <ddadap@nvidia.com>
>> ---
>>
>> v2: Convert to WMI subsystem driver, suggested by Mario Limonciello
>>      <mario.limonciello@outlook.com>; various cleanups suggested by
>>      Barnabás Pőcze <pobrn@protonmail.com>
>> v3: Address assorted style nits raised by Andy Shevchenko
>>      <andy.shevchenko@gmail.com> in response to a related patch; remove
>>      additional behavior change to WMI subsystem from patch series as
>>      recommended by Hans de Goede <hdegoede@redhat.com>
>>
>>   MAINTAINERS                               |   6 +
>>   drivers/platform/x86/Kconfig              |  10 ++
>>   drivers/platform/x86/Makefile             |   1 +
>>   drivers/platform/x86/wmaa-backlight-wmi.c | 184 ++++++++++++++++++++++
>>   4 files changed, 201 insertions(+)
>>   create mode 100644 drivers/platform/x86/wmaa-backlight-wmi.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index bbaecde94aa0..fd7362a86c6d 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -20008,6 +20008,12 @@ L:	linux-wireless@vger.kernel.org
>>   S:	Odd fixes
>>   F:	drivers/net/wireless/wl3501*
>>
>> +WMAA BACKLIGHT DRIVER
>> +M:	Daniel Dadap <ddadap@nvidia.com>
>> +L:	platform-driver-x86@vger.kernel.org
>> +S:	Supported
>> +F:	drivers/platform/x86/wmaa-backlight-wmi.c
>> +
>>   WOLFSON MICROELECTRONICS DRIVERS
>>   L:	patches@opensource.cirrus.com
>>   S:	Supported
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index d12db6c316ea..e54449c16d03 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -113,6 +113,16 @@ config PEAQ_WMI
>>   	help
>>   	 Say Y here if you want to support WMI-based hotkeys on PEAQ 2-in-1s.
>>
>> +config WMAA_BACKLIGHT_WMI
>> +	tristate "ACPI WMAA Backlight Driver"
>> +	depends on ACPI_WMI
>> +	depends on BACKLIGHT_CLASS_DEVICE
>> +	help
>> +	  This driver provides a sysfs backlight interface for notebook
>> +	  systems which expose the WMAA ACPI method and an associated WMI
>> +	  wrapper to drive LCD backlight levels through the system's
>> +	  Embedded Controller.
>> +
>>   config XIAOMI_WMI
>>   	tristate "Xiaomi WMI key driver"
>>   	depends on ACPI_WMI
>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
>> index 7ee369aab10d..109c1714237d 100644
>> --- a/drivers/platform/x86/Makefile
>> +++ b/drivers/platform/x86/Makefile
>> @@ -14,6 +14,7 @@ obj-$(CONFIG_INTEL_WMI_SBL_FW_UPDATE)	+= intel-wmi-sbl-fw-update.o
>>   obj-$(CONFIG_INTEL_WMI_THUNDERBOLT)	+= intel-wmi-thunderbolt.o
>>   obj-$(CONFIG_MXM_WMI)			+= mxm-wmi.o
>>   obj-$(CONFIG_PEAQ_WMI)			+= peaq-wmi.o
>> +obj-$(CONFIG_WMAA_BACKLIGHT_WMI)	+= wmaa-backlight-wmi.o
>>   obj-$(CONFIG_XIAOMI_WMI)		+= xiaomi-wmi.o
>>   obj-$(CONFIG_GIGABYTE_WMI)		+= gigabyte-wmi.o
>>
>> diff --git a/drivers/platform/x86/wmaa-backlight-wmi.c b/drivers/platform/x86/wmaa-backlight-wmi.c
>> new file mode 100644
>> index 000000000000..b607d3f88fc2
>> --- /dev/null
>> +++ b/drivers/platform/x86/wmaa-backlight-wmi.c
>> @@ -0,0 +1,184 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2020, NVIDIA CORPORATION.  All rights reserved.
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/backlight.h>
>> +#include <linux/module.h>
>> +#include <linux/wmi.h>
>> +
>> +#define WMAA_WMI_GUID "603E9613-EF25-4338-A3D0-C46177516DB7"
>> +
>> +struct wmi_wmaa_priv {
>> +	struct backlight_device *backlight;
>> +};
>> +
>> +enum wmaa_method {
>> +	WMAA_BRIGHTNESS_LEVEL = 1,
>> +	WMAA_BRIGHTNESS_SOURCE = 2,
>> +};
>> +
>> +enum wmaa_get_or_set {
>> +	WMAA_GET = 0,
>> +	WMAA_SET = 1,
>> +	WMAA_GET_MAX = 2, // for WMAA_BRIGHTNESS_LEVEL only
> /* this type of comments is preferred */
>
>
>> +};
>> +
>> +enum wmaa_source {
>> +	WMAA_SOURCE_CLEAR = 0,
>> +	WMAA_SOURCE_GPU = 1,
>> +	WMAA_SOURCE_EC = 2,
>> +	WMAA_SOURCE_AUX = 3,
>> +	WMAA_SOURCE_COUNT
>> +};
>> +
>> +struct wmaa_args {
>> +	u32 set;
>> +	u32 val;
>> +	u32 ret;
>> +	u32 ignored[3];
>> +};
>> +
>> +static const struct wmi_device_id wmaa_backlight_wmi_id_table[] = {
>> +	{ .guid_string = WMAA_WMI_GUID },
>> +	{ },
>> +};
>> +
>> +static struct wmi_device *wdev;
>> +
>> +static int wmi_call_wmaa(enum wmaa_method method, enum wmaa_get_or_set set, u32 *val)
>> +{
>> +	struct wmaa_args args = {
>> +		.set = set,
>> +		.val = 0,
>> +		.ret = 0,
>> +	};
>> +	struct acpi_buffer buf = { (acpi_size)sizeof(args), &args };
>> +	acpi_status status;
>> +
>> +	if (set == WMAA_SET)
>> +		args.val = *val;
>> +
>> +	status = wmidev_evaluate_method(wdev, 0, method, &buf, &buf);
>> +	if (ACPI_FAILURE(status)) {
>> +		pr_err("ACPI WMAA failed with %s\n", acpi_format_exception(status));
> I think this could be `dev_err()` instead.


Yes, it could. Looks like this is the only pr_* macro used as well, so 
we can delete the pr_fmt define.


>
>> +		return -EIO;
>> +	}
>> +
>> +	if (set != WMAA_SET)
>> +		*val = args.ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int wmaa_get_brightness(u32 *level)
>> +{
>> +	return wmi_call_wmaa(WMAA_BRIGHTNESS_LEVEL, WMAA_GET, level);
>> +}
>> +
>> +static int wmaa_set_brightness(u32 level)
>> +{
>> +	return wmi_call_wmaa(WMAA_BRIGHTNESS_LEVEL, WMAA_SET, &level);
>> +}
>> +
>> +static int wmaa_backlight_update_status(struct backlight_device *bd)
>> +{
>> +	return wmaa_set_brightness(bd->props.brightness);
>> +}
>> +
>> +static int wmaa_backlight_get_brightness(struct backlight_device *bd)
>> +{
>> +	u32 level;
>> +	int ret;
>> +
>> +	ret = wmaa_get_brightness(&level);
>> +
>> +	WARN_ON(ret != 0);
>> +	return ret == 0 ? level : 0;
>> +}
>> +
>> +static const struct backlight_ops wmaa_backlight_ops = {
>> +	.update_status = wmaa_backlight_update_status,
>> +	.get_brightness = wmaa_backlight_get_brightness,
>> +};
>> +
>> +static int wmaa_get_max_brightness(u32 *level)
>> +{
>> +	return wmi_call_wmaa(WMAA_BRIGHTNESS_LEVEL, WMAA_GET_MAX, level);
>> +}
>> +
>> +static int wmaa_get_brightness_source(u32 *source)
>> +{
>> +	return wmi_call_wmaa(WMAA_BRIGHTNESS_SOURCE, WMAA_GET, source);
>> +}
>> +
>> +static int wmaa_backlight_wmi_probe(struct wmi_device *w, const void *ctx)
>> +{
>> +	struct backlight_properties props = {0};
>> +	struct wmi_wmaa_priv *priv;
>> +	u32 source;
>> +	int ret;
>> +
>> +	priv = devm_kmalloc(&w->dev, sizeof(*priv), GFP_KERNEL);
>> +	if(!priv)
>> +		return -ENOMEM;
>> +
>> +	wdev = w;
> It seems odd to me that half of the state is per-device, but the other half is global.


You're right; the global state is unnecessary since this can be 
associated with the backlight_dev.


>
>> +
>> +	ret = wmaa_get_brightness_source(&source);
>> +	if (ret)
>> +		goto done;
>> +
>> +	if (source != WMAA_SOURCE_EC) {
>> +		ret = -ENODEV;
>> +		goto done;
>> +	}
>> +
>> +	// Register a backlight handler
>> +	props.type = BACKLIGHT_PLATFORM;
>> +	ret = wmaa_get_max_brightness(&props.max_brightness);
>> +	if (ret)
>> +		goto done;
>> +
>> +	ret = wmaa_get_brightness(&props.brightness);
>> +	if (ret)
>> +		goto done;
>> +
>> +	priv->backlight = backlight_device_register("wmaa_backlight",
>> +		NULL, NULL, &wmaa_backlight_ops, &props);
> Have you looked at `devm_backlight_device_register()`?


No; I wasn't aware of it. That does seem like it would simplify things 
by removing some more boilerplate. I'm currently testing a new patch 
incorporating your suggestions.


>
>
>> +	if (IS_ERR(priv->backlight))
>> +		return PTR_ERR(priv->backlight);
>> +
>> +	dev_set_drvdata(&w->dev, priv);
>> +
>> +done:
>> +	return ret;
>> +}
>> +
>> +static void wmaa_backlight_wmi_remove(struct wmi_device *wdev)
>> +{
>> +	struct wmi_wmaa_priv *priv = dev_get_drvdata(&wdev->dev);
>> +
>> +	backlight_device_unregister(priv->backlight);
>> +}
>> +
>> +static struct wmi_driver wmaa_backlight_wmi_driver = {
>> +	.driver = {
>> +		.name = "wmaa-backlight",
>> +	},
>> +	.probe = wmaa_backlight_wmi_probe,
>> +	.remove = wmaa_backlight_wmi_remove,
>> +	.id_table = wmaa_backlight_wmi_id_table,
>> +};
>> +
>> +module_wmi_driver(wmaa_backlight_wmi_driver);
>> +
>> +MODULE_AUTHOR("Aaron Plattner <aplattner@nvidia.com>");
>> +MODULE_AUTHOR("Daniel Dadap <ddadap@nvidia.com>");
>> +MODULE_DESCRIPTION("WMAA Backlight WMI driver");
>> +MODULE_LICENSE("GPL v2");
>> +
>> +MODULE_ALIAS("wmi:"WMAA_WMI_GUID);
>> --
>> 2.20.1
>
> Best regards,
> Barnabás Pőcze