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@xxxxxxxxxx> Signed-off-by: Daniel Dadap <ddadap@xxxxxxxxxx> --- v2: Convert to WMI subsystem driver, suggested by Mario Limonciello <mario.limonciello@xxxxxxxxxxx>; various cleanups suggested by Barnabás Pőcze <pobrn@xxxxxxxxxxxxxx> v3: Address assorted style nits raised by Andy Shevchenko <andy.shevchenko@xxxxxxxxx> in response to a related patch; remove additional behavior change to WMI subsystem from patch series as recommended by Hans de Goede <hdegoede@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx S: Odd fixes F: drivers/net/wireless/wl3501* +WMAA BACKLIGHT DRIVER +M: Daniel Dadap <ddadap@xxxxxxxxxx> +L: platform-driver-x86@xxxxxxxxxxxxxxx +S: Supported +F: drivers/platform/x86/wmaa-backlight-wmi.c + WOLFSON MICROELECTRONICS DRIVERS L: patches@xxxxxxxxxxxxxxxxxxxxx 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@xxxxxxxxxx>"); +MODULE_AUTHOR("Daniel Dadap <ddadap@xxxxxxxxxx>"); +MODULE_DESCRIPTION("WMAA Backlight WMI driver"); +MODULE_LICENSE("GPL v2"); + +MODULE_ALIAS("wmi:"WMAA_WMI_GUID); -- 2.20.1Best regards, Barnabás Pőcze
