| Message ID | 20210824220437.14175-1-ddadap@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v3] platform/x86: Add driver for ACPI WMAA EC-based backlight control | expand |
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. > + 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. > + > + 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()`? > + 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
On Wed, Aug 25, 2021 at 1:09 AM Daniel Dadap <ddadap@nvidia.com> wrote: > > 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. I tried to avoid repetition of the comments given by others. So, mine below. ... > +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. Please, add a sentence to tell how the module will be called. There are plenty of examples in the kernel. ... > +struct wmaa_args { > + u32 set; > + u32 val; > + u32 ret; > + u32 ignored[3]; > +}; I guess this structure deserves a kernel doc. ... > +static const struct wmi_device_id wmaa_backlight_wmi_id_table[] = { > + { .guid_string = WMAA_WMI_GUID }, > + { }, No comma for termination. > +}; ... > +static int wmaa_backlight_get_brightness(struct backlight_device *bd) > +{ > + u32 level; > + int ret; > + > + ret = wmaa_get_brightness(&level); > + WARN_ON(ret != 0); Why? > + return ret == 0 ? level : 0; > +} ... > +static int wmaa_backlight_wmi_probe(struct wmi_device *w, const void *ctx) > +{ > + struct backlight_properties props = {0}; {} is slightly better. > + struct wmi_wmaa_priv *priv; > + u32 source; > + int ret; > + > + priv = devm_kmalloc(&w->dev, sizeof(*priv), GFP_KERNEL); > + if(!priv) > + return -ENOMEM; > + wdev = w; I'm wondering if it's possible to avoid having a global variable. > + 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); > + if (IS_ERR(priv->backlight)) > + return PTR_ERR(priv->backlight); > + > + dev_set_drvdata(&w->dev, priv); > +done: Useless. Return directly. > + return ret; > +} ... > +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, > +}; > + Redundant blank line. > +module_wmi_driver(wmaa_backlight_wmi_driver); ... > + > +MODULE_ALIAS("wmi:"WMAA_WMI_GUID); Can you move this closer to GUID? But I'm not sure what is the preferred style. Hans?
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
Thanks again: On 8/25/21 4:05 AM, Andy Shevchenko wrote: > On Wed, Aug 25, 2021 at 1:09 AM Daniel Dadap <ddadap@nvidia.com> wrote: >> 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. > I tried to avoid repetition of the comments given by others. > > So, mine below. > > ... > >> +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. > Please, add a sentence to tell how the module will be called. There > are plenty of examples in the kernel. > > ... > >> +struct wmaa_args { >> + u32 set; >> + u32 val; >> + u32 ret; >> + u32 ignored[3]; >> +}; > I guess this structure deserves a kernel doc. Do you have a recommended location? From a quick skim I didn't see any document in Documentation/ that seemed most appropriate to add this to. > > ... > >> +static const struct wmi_device_id wmaa_backlight_wmi_id_table[] = { >> + { .guid_string = WMAA_WMI_GUID }, >> + { }, > No comma for termination. > >> +}; > ... > >> +static int wmaa_backlight_get_brightness(struct backlight_device *bd) >> +{ >> + u32 level; >> + int ret; >> + >> + ret = wmaa_get_brightness(&level); >> + WARN_ON(ret != 0); > Why? To differentiate a 0 because the level is actually 0 versus a 0 because there was an error. The backlight device API doesn't seem to have a way to report errors. >> + return ret == 0 ? level : 0; >> +} > ... > >> +static int wmaa_backlight_wmi_probe(struct wmi_device *w, const void *ctx) >> +{ >> + struct backlight_properties props = {0}; > {} is slightly better. > >> + struct wmi_wmaa_priv *priv; >> + u32 source; >> + int ret; >> + >> + priv = devm_kmalloc(&w->dev, sizeof(*priv), GFP_KERNEL); >> + if(!priv) >> + return -ENOMEM; >> + wdev = w; > I'm wondering if it's possible to avoid having a global variable. It is; this can be stored in the struct backlight_device. > >> + 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); >> + if (IS_ERR(priv->backlight)) >> + return PTR_ERR(priv->backlight); >> + >> + dev_set_drvdata(&w->dev, priv); >> +done: > Useless. Return directly. Yeah, most likely there used to be some other cleanup here. Thanks. > >> + return ret; >> +} > ... > >> +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, >> +}; >> + > Redundant blank line. > >> +module_wmi_driver(wmaa_backlight_wmi_driver); > ... > >> + >> +MODULE_ALIAS("wmi:"WMAA_WMI_GUID); > Can you move this closer to GUID? But I'm not sure what is the > preferred style. Hans? I'll do whatever is most stylistically preferred.
On Mi, 2021-08-25T11:47-0500, Daniel Dadap wrote: >> +static const struct wmi_device_id wmaa_backlight_wmi_id_table[] = { >> + { .guid_string = WMAA_WMI_GUID }, >> + { }, > > > +MODULE_ALIAS("wmi:"WMAA_WMI_GUID); > > Can you move this closer to GUID? But I'm not sure what is the > > preferred style. Hans? > > > I'll do whatever is most stylistically preferred. > This could also be expressed as, which is presumably the nicer way: MODULE_DEVICE_TABLE(wmi, wmaa_backlight_wmi_id_table); Thomas
On 8/24/21 5:47 PM, Barnabás Pőcze wrote:
> Have you looked at `devm_backlight_device_register()`?
So regarding this suggestion, I have switched to
devm_backlight_device_register(), but now I can't unload the module; it
fails with EBUSY. I tried unbinding the device (via
/sys/class/backlight/wmaa_backlight/device/driver/unbind), but that
fails with ENODEV, maybe due to missing plumbing in wmi?
I'm not sure if it's expected for users to be able to unload their
backlight drivers; on the random selection of notebooks that I have
immediately available to me at the moment, the backlight driver is
registered by the GPU driver, and that tends to be not unloadable
because e.g. it's driving the console.
It's probably best to use devm_backlight_device_register() anyway since
it seems the ordinary backlight_device_register() is deprecated, but I
wanted to see if there were any strong opinions about whether it would
be a problem that the driver can't be unloaded.
On 8/25/21 5:06 PM, Thomas Weißschuh wrote: > On Mi, 2021-08-25T11:47-0500, Daniel Dadap wrote: >>> +static const struct wmi_device_id wmaa_backlight_wmi_id_table[] = { >>> + { .guid_string = WMAA_WMI_GUID }, >>> + { }, >>>> +MODULE_ALIAS("wmi:"WMAA_WMI_GUID); >>> Can you move this closer to GUID? But I'm not sure what is the >>> preferred style. Hans? >> >> I'll do whatever is most stylistically preferred. >> > This could also be expressed as, which is presumably the nicer way: > > MODULE_DEVICE_TABLE(wmi, wmaa_backlight_wmi_id_table); Okay, thanks. In the meantime I've moved the declaration of this table and the #define for the WMAA_WMI_GUID macro down to the end of the file, right before the pile of MODULE_* macros where we also use WMAA_WMI_GUID for the modalias, but if there's a more preferred style here just let me know.
On Wed, Aug 25, 2021 at 7:48 PM Daniel Dadap <ddadap@nvidia.com> wrote: > On 8/25/21 4:05 AM, Andy Shevchenko wrote: > > On Wed, Aug 25, 2021 at 1:09 AM Daniel Dadap <ddadap@nvidia.com> wrote: ... > >> +struct wmaa_args { > >> + u32 set; > >> + u32 val; > >> + u32 ret; > >> + u32 ignored[3]; > >> +}; > > I guess this structure deserves a kernel doc. > > Do you have a recommended location? From a quick skim I didn't see any > document in Documentation/ that seemed most appropriate to add this to. It's in a form of the comment on top of the data structure /** * struct wmaa_args .... * ... */ ... > >> + WARN_ON(ret != 0); > > Why? > > To differentiate a 0 because the level is actually 0 versus a 0 because > there was an error. The backlight device API doesn't seem to have a way > to report errors. I meant why do you need WARN_ON() here? This kind of stuff must be justified. -- With Best Regards, Andy Shevchenko
On 8/26/21 8:35 AM, Andy Shevchenko wrote: > On Wed, Aug 25, 2021 at 7:48 PM Daniel Dadap <ddadap@nvidia.com> wrote: >> On 8/25/21 4:05 AM, Andy Shevchenko wrote: >>> On Wed, Aug 25, 2021 at 1:09 AM Daniel Dadap <ddadap@nvidia.com> wrote: > ... > >>>> +struct wmaa_args { >>>> + u32 set; >>>> + u32 val; >>>> + u32 ret; >>>> + u32 ignored[3]; >>>> +}; >>> I guess this structure deserves a kernel doc. >> Do you have a recommended location? From a quick skim I didn't see any >> document in Documentation/ that seemed most appropriate to add this to. > It's in a form of the comment on top of the data structure > > /** > * struct wmaa_args .... > * ... > */ > > ... Ah, of course. Sorry for misunderstanding. >>>> + WARN_ON(ret != 0); >>> Why? >> To differentiate a 0 because the level is actually 0 versus a 0 because >> there was an error. The backlight device API doesn't seem to have a way >> to report errors. > I meant why do you need WARN_ON() here? This kind of stuff must be justified. Thanks, I see what you mean now. I'll change it to a dev_warn() or maybe even a dev_err(), since if the ACPI call does fail there probably is something quite amiss.
On 8/25/21 5:26 PM, Daniel Dadap wrote: > On 8/24/21 5:47 PM, Barnabás Pőcze wrote: >> Have you looked at `devm_backlight_device_register()`? > > > So regarding this suggestion, I have switched to > devm_backlight_device_register(), but now I can't unload the module; > it fails with EBUSY. I tried unbinding the device (via > /sys/class/backlight/wmaa_backlight/device/driver/unbind), but that > fails with ENODEV, maybe due to missing plumbing in wmi? > > I'm not sure if it's expected for users to be able to unload their > backlight drivers; on the random selection of notebooks that I have > immediately available to me at the moment, the backlight driver is > registered by the GPU driver, and that tends to be not unloadable > because e.g. it's driving the console. > > It's probably best to use devm_backlight_device_register() anyway > since it seems the ordinary backlight_device_register() is deprecated, > but I wanted to see if there were any strong opinions about whether it > would be a problem that the driver can't be unloaded. > Okay, actually, I *can* unbind the device; I was apparently pasting a truncated GUID into the shell so it wasn't finding it. However, even after successfully unbinding, I still can't unload the module; it still fails with EBUSY, even if I explicitly call devm_backlight_device_unregister() in a wmi_driver .remove callback. I confirmed that .remove() is getting called when the device is unbound.
On 8/27/21 11:47 AM, Daniel Dadap wrote: > On 8/25/21 5:26 PM, Daniel Dadap wrote: >> On 8/24/21 5:47 PM, Barnabás Pőcze wrote: >>> Have you looked at `devm_backlight_device_register()`? >> >> >> So regarding this suggestion, I have switched to >> devm_backlight_device_register(), but now I can't unload the module; >> it fails with EBUSY. I tried unbinding the device (via >> /sys/class/backlight/wmaa_backlight/device/driver/unbind), but that >> fails with ENODEV, maybe due to missing plumbing in wmi? >> >> I'm not sure if it's expected for users to be able to unload their >> backlight drivers; on the random selection of notebooks that I have >> immediately available to me at the moment, the backlight driver is >> registered by the GPU driver, and that tends to be not unloadable >> because e.g. it's driving the console. >> >> It's probably best to use devm_backlight_device_register() anyway >> since it seems the ordinary backlight_device_register() is >> deprecated, but I wanted to see if there were any strong opinions >> about whether it would be a problem that the driver can't be unloaded. >> > > Okay, actually, I *can* unbind the device; I was apparently pasting a > truncated GUID into the shell so it wasn't finding it. However, even > after successfully unbinding, I still can't unload the module; it > still fails with EBUSY, even if I explicitly call > devm_backlight_device_unregister() in a wmi_driver .remove callback. I > confirmed that .remove() is getting called when the device is unbound. Sorry for the noise. It can be unloaded after unbinding if I *don't* explicitly call devm_backlight_device_unregister() and leave the .remove() callback unimplemented. I must have accidentally been test-loading the wrong version of the kernel module in an earlier experiment. So all does indeed seem to be well. I'll provide a revised patch shortly, after some additional testing.
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 +}; + +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)); + 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; + + 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); + 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);