| Message ID | 20210831224906.1072-1-ddadap@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v4] platform/x86: Add driver for ACPI WMAA EC-based backlight control | expand |
Hi Daniel, a few minor comments, not sure if they should be reason for a reroll. Thomas Reviewed-By: Thomas Weißschuh <linux@weissschuh.net> On Di, 2021-08-31T17:49-0500, Daniel Dadap 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. > > 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> > v4: Use MODULE_DEVICE_TABLE() (Thomas Wei??schuh <thomas@t-8ch.de>) > Fix scope of internal driver state; various style fixes (Barnab??s > P??cze, Andy Shevchenko) > Use devm_backlight_device_register() (Barnab??s P??cze) > Add kerneldoc comments for enums and structs (Andy Shevchenko) It seems your Mailsetup breaks Unicode. > MAINTAINERS | 6 + > drivers/platform/x86/Kconfig | 16 ++ > drivers/platform/x86/Makefile | 1 + > drivers/platform/x86/wmaa-backlight-wmi.c | 185 ++++++++++++++++++++++ > 4 files changed, 208 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..0df908ef8d7c 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -113,6 +113,22 @@ 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 (EC). > + > + Say Y or M here if you want to control the backlight on a notebook > + system with an EC-driven backlight using the ACPI WMAA method. > + > + If you choose to compile this driver as a module the module will be > + called wmaa-backlight-wmi. > + > 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..cb1a973803b1 > --- /dev/null > +++ b/drivers/platform/x86/wmaa-backlight-wmi.c > @@ -0,0 +1,185 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2020, NVIDIA CORPORATION. All rights reserved. Also 2021? > + */ > + > +#include <linux/backlight.h> > +#include <linux/module.h> > +#include <linux/wmi.h> > + > +/** > + * enum wmaa_method - WMI method IDs for ACPI WMAA > + * > + * @WMAA_LEVEL: Get or set the brightness level, > + * or get the maximum brightness level. > + * @WMAA_SOURCE: Get the source for backlight control. > + */ > +enum wmaa_method { > + WMAA_LEVEL = 1, > + WMAA_SOURCE = 2, > +}; Nitpick: No need for explicit values here. > +/** > + * enum wmaa_get_or_set - Operation mode for ACPI WMAA method > + * > + * @WMAA_GET: Get the current brightness level or backlight source. > + * @WMAA_SET: Set the brightness level. > + * @WMAA_GET_MAX: Get the maximum brightness level. This is only valid when the > + * WMI method is %WMAA_LEVEL. > + */ > +enum wmaa_get_or_set { Nitpick: wmaa_operation/command > + WMAA_GET = 0, > + WMAA_SET = 1, > + WMAA_GET_MAX = 2 > +}; Nitpick: No need for explicit values here. > +/** > + * enum wmaa_source - Backlight brightness control source identification > + * > + * @WMAA_SOURCE_GPU: Backlight brightness is controlled by the GPU. > + * @WMAA_SOURCE_EC: Backlight brightness is controlled by the system's > + * Embedded Controller (EC). > + * @WMAA_SOURCE_AUX: Backlight brightness is controlled over the DisplayPort > + * AUX channel. > + */ > +enum wmaa_source { > + WMAA_SOURCE_GPU = 1, > + WMAA_SOURCE_EC = 2, > + WMAA_SOURCE_AUX = 3 > +}; > + > +/** > + * struct wmaa_args - arguments for the ACPI WMAA method > + * > + * @set: Pass in an &enum wmaa_get_or_set value to select between getting or > + * setting a value. > + * @val: In parameter for value to set when operating in %WMAA_SET mode. Not > + * used in %WMAA_GET or %WMAA_GET_MAX mode. > + * @ret: Out parameter returning retrieved value when operating in %WMAA_GET > + * or %WMAA_GET_MAX mode. Not used in %WMAA_SET mode. > + * @ignored: Padding; not used. The ACPI method expects a 24 byte params struct. > + * > + * This is the parameters structure for the ACPI WMAA method as wrapped by WMI. > + * The value passed in to @val or returned by @ret will be a brightness value > + * when the WMI method ID is %WMAA_LEVEL, or an &enum wmaa_source value when > + * the WMI method ID is %WMAA_SOURCE. > + */ > +struct wmaa_args { > + u32 set; > + u32 val; > + u32 ret; > + u32 ignored[3]; > +}; > + > +static int wmi_call_wmaa(struct wmi_device *w, 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(w, 0, method, &buf, &buf); > + if (ACPI_FAILURE(status)) { > + dev_err(&w->dev, "ACPI WMAA failed with %s\n", > + acpi_format_exception(status)); > + return -EIO; > + } > + > + if (set != WMAA_SET) > + *val = args.ret; > + > + return 0; > +} > + > +static int wmaa_backlight_update_status(struct backlight_device *bd) > +{ > + struct wmi_device *wdev = bl_get_data(bd); > + > + return wmi_call_wmaa(wdev, WMAA_LEVEL, WMAA_SET, &bd->props.brightness); > +} > + > +static int wmaa_backlight_get_brightness(struct backlight_device *bd) > +{ > + struct wmi_device *wdev = bl_get_data(bd); > + u32 level = 0; > + int ret; > + > + ret = wmi_call_wmaa(wdev, WMAA_LEVEL, WMAA_GET, &level); > + > + if (ret) > + dev_err(&bd->dev, "ACPI WMAA failed to get backlight level.\n"); Would it make sense to log the actual error? Also while the backlight core does not seem to actually handle it, other drivers return a negative errno on failure. > + > + return level; > +} > + > +static const struct backlight_ops wmaa_backlight_ops = { > + .update_status = wmaa_backlight_update_status, > + .get_brightness = wmaa_backlight_get_brightness > +}; > + > +static int wmaa_backlight_wmi_probe(struct wmi_device *w, const void *ctx) > +{ > + struct backlight_properties props = {}; > + struct backlight_device *backlight; > + u32 source; > + int ret; > + > + ret = wmi_call_wmaa(w, WMAA_SOURCE, WMAA_GET, &source); > + if (ret) > + return ret; > + > + if (source != WMAA_SOURCE_EC) { > + /* This driver is only to be used when brightness control is > + * handled by the EC; otherwise, the GPU driver(s) should handle > + * brightness control. */ checkpatch.pl wants this end-of-comment to be on its own line. > + return -ENODEV; > + } > + > + /* Identify this backlight device as a platform device so that it can > + * be prioritized over any exposed GPU-driven raw device(s). */ checkpatch.pl wants this end-of-comment to be on its own line. > + props.type = BACKLIGHT_PLATFORM; Is this WMI method a standard interface? If so, BACKLIGHT_FIRMWARE might actually fit better and still fulfill the requirements. The actual maintainers would know better than me, though. > + > + ret = wmi_call_wmaa(w, WMAA_LEVEL, WMAA_GET_MAX, &props.max_brightness); > + if (ret) > + return ret; > + > + ret = wmi_call_wmaa(w, WMAA_LEVEL, WMAA_GET, &props.brightness); > + if (ret) > + return ret; > + > + backlight = devm_backlight_device_register( > + &w->dev, "wmaa_backlight", > + &w->dev, w, &wmaa_backlight_ops, &props); > + if (IS_ERR(backlight)) > + return PTR_ERR(backlight); > + > + return 0; Could use PTR_ERR_OR_ZERO(). > +} > + > +#define WMAA_WMI_GUID "603E9613-EF25-4338-A3D0-C46177516DB7" > + > +static const struct wmi_device_id wmaa_backlight_wmi_id_table[] = { > + { .guid_string = WMAA_WMI_GUID }, > + { } > +}; > +MODULE_DEVICE_TABLE(wmi, wmaa_backlight_wmi_id_table); > + > +static struct wmi_driver wmaa_backlight_wmi_driver = { > + .driver = { > + .name = "wmaa-backlight" > + }, > + .probe = wmaa_backlight_wmi_probe, > + .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"); "GPL" seems to be the more modern one. (Equivalent to "GPL v2", see Documentation/process/license-rules.rst)
On Wed, Sep 1, 2021 at 2:27 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. > Signed-off-by: Aaron Plattner <aplattner@nvidia.com> Seems like missed Co-developed-by. Or you are the wrong author. Fix accordingly. > Signed-off-by: Daniel Dadap <ddadap@nvidia.com> ... > +#include <linux/backlight.h> > +#include <linux/module.h> > +#include <linux/wmi.h> types.h ? mod_devicetable.h ? ... > +enum wmaa_get_or_set { > + WMAA_GET = 0, > + WMAA_SET = 1, > + WMAA_GET_MAX = 2 Is it part of HW protocol? Otherwise drop assignments. > +}; ... > + */ > +enum wmaa_source { > + WMAA_SOURCE_GPU = 1, > + WMAA_SOURCE_EC = 2, > + WMAA_SOURCE_AUX = 3 Missed comma. > +}; ... > +/** > + * struct wmaa_args - arguments for the ACPI WMAA method > + * Redundant blank line. > + * @set: Pass in an &enum wmaa_get_or_set value to select between getting or > + * setting a value. > + * @val: In parameter for value to set when operating in %WMAA_SET mode. Not > + * used in %WMAA_GET or %WMAA_GET_MAX mode. > + * @ret: Out parameter returning retrieved value when operating in %WMAA_GET > + * or %WMAA_GET_MAX mode. Not used in %WMAA_SET mode. > + * @ignored: Padding; not used. The ACPI method expects a 24 byte params struct. > + * > + * This is the parameters structure for the ACPI WMAA method as wrapped by WMI. > + * The value passed in to @val or returned by @ret will be a brightness value > + * when the WMI method ID is %WMAA_LEVEL, or an &enum wmaa_source value when > + * the WMI method ID is %WMAA_SOURCE. > + */ ... > + ret = wmi_call_wmaa(wdev, WMAA_LEVEL, WMAA_GET, &level); > + Ditto. > + if (ret) > + dev_err(&bd->dev, "ACPI WMAA failed to get backlight level.\n"); ... > +static const struct backlight_ops wmaa_backlight_ops = { > + .update_status = wmaa_backlight_update_status, > + .get_brightness = wmaa_backlight_get_brightness Missed comma. > +}; ... > + if (source != WMAA_SOURCE_EC) { > + /* This driver is only to be used when brightness control is > + * handled by the EC; otherwise, the GPU driver(s) should handle > + * brightness control. */ /* * Wrong multi-line comment * style. Use this example to fix * it properly. */ > + return -ENODEV; > + } > + /* Identify this backlight device as a platform device so that it can > + * be prioritized over any exposed GPU-driven raw device(s). */ Ditto. And for any other multi-line comments in this driver. ... > + backlight = devm_backlight_device_register( > + &w->dev, "wmaa_backlight", > + &w->dev, w, &wmaa_backlight_ops, &props); > + if (IS_ERR(backlight)) > + return PTR_ERR(backlight); > + > + return 0; return PTR_ERR_OR_ZERO(backlight); ... > +MODULE_AUTHOR("Aaron Plattner <aplattner@nvidia.com>"); > +MODULE_AUTHOR("Daniel Dadap <ddadap@nvidia.com>"); So, Co-developed-by seems missing above. ... > +MODULE_LICENSE("GPL v2"); "GPL" is enough (it's a synonim).
On 9/1/21 8:57 AM, Andy Shevchenko wrote: > On Wed, Sep 1, 2021 at 2:27 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. >> Signed-off-by: Aaron Plattner <aplattner@nvidia.com> > Seems like missed Co-developed-by. Or you are the wrong author. Fix accordingly. I wrote the initial version of this driver that we used internally for testing, but Daniel almost completely rewrote it for upstreaming. I'd be fine with just dropping me from the patch entirely and listing Daniel as the author since it looks almost nothing like the code I wrote. -- Aaron
Thanks, Thomas: On 9/1/21 4:25 AM, Thomas Weißschuh wrote: > Hi Daniel, > > a few minor comments, not sure if they should be reason for a reroll. I don't mind rerolling until I get an ACK from the maintainer. :) > > Thomas > > Reviewed-By: Thomas Weißschuh <linux@weissschuh.net> > > On Di, 2021-08-31T17:49-0500, Daniel Dadap 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. >> >> 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> >> v4: Use MODULE_DEVICE_TABLE() (Thomas Wei??schuh <thomas@t-8ch.de>) >> Fix scope of internal driver state; various style fixes (Barnab??s >> P??cze, Andy Shevchenko) >> Use devm_backlight_device_register() (Barnab??s P??cze) >> Add kerneldoc comments for enums and structs (Andy Shevchenko) > It seems your Mailsetup breaks Unicode. Strange. I've just been sending the patches via git email-patch. These characters are appearing correctly with my MUA (Thunderbird) in the copy of the message I received, so I don't know where in the pipeline they're getting corrupted. I'll ignore this problem unless anybody has some specific tips on fixing it, since the non-ASCII characters aren't going to actually be part of the git history of the patch. > >> MAINTAINERS | 6 + >> drivers/platform/x86/Kconfig | 16 ++ >> drivers/platform/x86/Makefile | 1 + >> drivers/platform/x86/wmaa-backlight-wmi.c | 185 ++++++++++++++++++++++ >> 4 files changed, 208 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..0df908ef8d7c 100644 >> --- a/drivers/platform/x86/Kconfig >> +++ b/drivers/platform/x86/Kconfig >> @@ -113,6 +113,22 @@ 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 (EC). >> + >> + Say Y or M here if you want to control the backlight on a notebook >> + system with an EC-driven backlight using the ACPI WMAA method. >> + >> + If you choose to compile this driver as a module the module will be >> + called wmaa-backlight-wmi. >> + >> 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..cb1a973803b1 >> --- /dev/null >> +++ b/drivers/platform/x86/wmaa-backlight-wmi.c >> @@ -0,0 +1,185 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (c) 2020, NVIDIA CORPORATION. All rights reserved. > Also 2021? The code was originally written in 2020. I'm not certain it's changed substantially enough to warrant updating the copyright date. Certainly there's no need to change the copyright date every time the file is touched. I suppose it wouldn't be wrong to have it read 2020-2021 or the like for the initial commit, but I personally prefer just having the initial date unless there's stronger guidance to do otherwise. > >> + */ >> + >> +#include <linux/backlight.h> >> +#include <linux/module.h> >> +#include <linux/wmi.h> >> + >> +/** >> + * enum wmaa_method - WMI method IDs for ACPI WMAA >> + * >> + * @WMAA_LEVEL: Get or set the brightness level, >> + * or get the maximum brightness level. >> + * @WMAA_SOURCE: Get the source for backlight control. >> + */ >> +enum wmaa_method { >> + WMAA_LEVEL = 1, >> + WMAA_SOURCE = 2, >> +}; > Nitpick: No need for explicit values here. I think at least the 1 is required, but in any case I prefer to be explicit here to make it clear that these are the exact values required by the ACPI WMAA interface. > >> +/** >> + * enum wmaa_get_or_set - Operation mode for ACPI WMAA method >> + * >> + * @WMAA_GET: Get the current brightness level or backlight source. >> + * @WMAA_SET: Set the brightness level. >> + * @WMAA_GET_MAX: Get the maximum brightness level. This is only valid when the >> + * WMI method is %WMAA_LEVEL. >> + */ >> +enum wmaa_get_or_set { > Nitpick: wmaa_operation/command That is indeed better; thanks. I'll go with "mode", mainly because it's shorter. > >> + WMAA_GET = 0, >> + WMAA_SET = 1, >> + WMAA_GET_MAX = 2 >> +}; > Nitpick: No need for explicit values here. > >> +/** >> + * enum wmaa_source - Backlight brightness control source identification >> + * >> + * @WMAA_SOURCE_GPU: Backlight brightness is controlled by the GPU. >> + * @WMAA_SOURCE_EC: Backlight brightness is controlled by the system's >> + * Embedded Controller (EC). >> + * @WMAA_SOURCE_AUX: Backlight brightness is controlled over the DisplayPort >> + * AUX channel. >> + */ >> +enum wmaa_source { >> + WMAA_SOURCE_GPU = 1, >> + WMAA_SOURCE_EC = 2, >> + WMAA_SOURCE_AUX = 3 >> +}; >> + >> +/** >> + * struct wmaa_args - arguments for the ACPI WMAA method >> + * >> + * @set: Pass in an &enum wmaa_get_or_set value to select between getting or >> + * setting a value. >> + * @val: In parameter for value to set when operating in %WMAA_SET mode. Not >> + * used in %WMAA_GET or %WMAA_GET_MAX mode. >> + * @ret: Out parameter returning retrieved value when operating in %WMAA_GET >> + * or %WMAA_GET_MAX mode. Not used in %WMAA_SET mode. >> + * @ignored: Padding; not used. The ACPI method expects a 24 byte params struct. >> + * >> + * This is the parameters structure for the ACPI WMAA method as wrapped by WMI. >> + * The value passed in to @val or returned by @ret will be a brightness value >> + * when the WMI method ID is %WMAA_LEVEL, or an &enum wmaa_source value when >> + * the WMI method ID is %WMAA_SOURCE. >> + */ >> +struct wmaa_args { >> + u32 set; >> + u32 val; >> + u32 ret; >> + u32 ignored[3]; >> +}; >> + >> +static int wmi_call_wmaa(struct wmi_device *w, 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(w, 0, method, &buf, &buf); >> + if (ACPI_FAILURE(status)) { >> + dev_err(&w->dev, "ACPI WMAA failed with %s\n", >> + acpi_format_exception(status)); >> + return -EIO; >> + } >> + >> + if (set != WMAA_SET) >> + *val = args.ret; >> + >> + return 0; >> +} >> + >> +static int wmaa_backlight_update_status(struct backlight_device *bd) >> +{ >> + struct wmi_device *wdev = bl_get_data(bd); >> + >> + return wmi_call_wmaa(wdev, WMAA_LEVEL, WMAA_SET, &bd->props.brightness); >> +} >> + >> +static int wmaa_backlight_get_brightness(struct backlight_device *bd) >> +{ >> + struct wmi_device *wdev = bl_get_data(bd); >> + u32 level = 0; >> + int ret; >> + >> + ret = wmi_call_wmaa(wdev, WMAA_LEVEL, WMAA_GET, &level); >> + >> + if (ret) >> + dev_err(&bd->dev, "ACPI WMAA failed to get backlight level.\n"); > Would it make sense to log the actual error? I think the underlying ACPI error is already logged by wmi_call_wmaa(). > Also while the backlight core does not seem to actually handle it, other > drivers return a negative errno on failure. Thanks, I was aware that the backlight core doesn't seem to handle errors at all for .get_brightness, but I didn't think to check for precedent on backlight drivers returning error codes anyway. Returning negative errno does seem somewhat common amongst backlight drivers, so I'll go ahead and do that in case the backlight core is updated to handle them in the future. > >> + >> + return level; >> +} >> + >> +static const struct backlight_ops wmaa_backlight_ops = { >> + .update_status = wmaa_backlight_update_status, >> + .get_brightness = wmaa_backlight_get_brightness >> +}; >> + >> +static int wmaa_backlight_wmi_probe(struct wmi_device *w, const void *ctx) >> +{ >> + struct backlight_properties props = {}; >> + struct backlight_device *backlight; >> + u32 source; >> + int ret; >> + >> + ret = wmi_call_wmaa(w, WMAA_SOURCE, WMAA_GET, &source); >> + if (ret) >> + return ret; >> + >> + if (source != WMAA_SOURCE_EC) { >> + /* This driver is only to be used when brightness control is >> + * handled by the EC; otherwise, the GPU driver(s) should handle >> + * brightness control. */ > checkpatch.pl wants this end-of-comment to be on its own line. > >> + return -ENODEV; >> + } >> + >> + /* Identify this backlight device as a platform device so that it can >> + * be prioritized over any exposed GPU-driven raw device(s). */ > checkpatch.pl wants this end-of-comment to be on its own line. Thanks. I neglected to run checkpatch.pl after adding those comments. Running it on the current version of the patch now only flags an over-long line (the signature of wmi_call_wmaa()); I had originally wrapped that to 80 characters, but was told to leave it as one line earlier in the review process. >> + props.type = BACKLIGHT_PLATFORM; > Is this WMI method a standard interface? > If so, BACKLIGHT_FIRMWARE might actually fit better and still fulfill the > requirements. > The actual maintainers would know better than me, though. I'm not certain what you mean by standard. It's defined as part of system design guidelines that NVIDIA shares with OEMs, but I'm not sure if it's part of the ACPI spec. It is implemented by multiple notebook system vendors, hence naming the driver after the ACPI method rather than a particular vendor. Reading the documentation on the backlight driver types, it does seem that this may fall more under the purview of BACKLIGHT_FIRMWARE than BACKLIGHT_PLATFORM. I'll go ahead and retest with the driver registered as BACKLIGHT_FIRMWARE, but I'm sure it'll still work after inspecting the code for at least gnome-settings-daemon, which implements the policy recommended by Documentation/ABI/stable/sysfs-class-backlight. >> + >> + ret = wmi_call_wmaa(w, WMAA_LEVEL, WMAA_GET_MAX, &props.max_brightness); >> + if (ret) >> + return ret; >> + >> + ret = wmi_call_wmaa(w, WMAA_LEVEL, WMAA_GET, &props.brightness); >> + if (ret) >> + return ret; >> + >> + backlight = devm_backlight_device_register( >> + &w->dev, "wmaa_backlight", >> + &w->dev, w, &wmaa_backlight_ops, &props); >> + if (IS_ERR(backlight)) >> + return PTR_ERR(backlight); >> + >> + return 0; > Could use PTR_ERR_OR_ZERO(). Thanks, Andy suggested this too. > >> +} >> + >> +#define WMAA_WMI_GUID "603E9613-EF25-4338-A3D0-C46177516DB7" >> + >> +static const struct wmi_device_id wmaa_backlight_wmi_id_table[] = { >> + { .guid_string = WMAA_WMI_GUID }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(wmi, wmaa_backlight_wmi_id_table); >> + >> +static struct wmi_driver wmaa_backlight_wmi_driver = { >> + .driver = { >> + .name = "wmaa-backlight" >> + }, >> + .probe = wmaa_backlight_wmi_probe, >> + .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"); > "GPL" seems to be the more modern one. > (Equivalent to "GPL v2", see Documentation/process/license-rules.rst)
Thanks again, Andy: On 9/1/21 10:57 AM, Andy Shevchenko wrote: > On Wed, Sep 1, 2021 at 2:27 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. >> Signed-off-by: Aaron Plattner <aplattner@nvidia.com> > Seems like missed Co-developed-by. Or you are the wrong author. Fix accordingly. Indeed; as Aaron mentioned, he was the author of a much earlier version of this driver which we used internally for testing purposes. He actually gave me his blessing to leave his name off the patch even before I submitted the original version of the patch for review on this list, but I figured it wasn't a big deal to leave him on. Since Aaron has suggested taking him off the patch again, I'll just go ahead and do that. > >> Signed-off-by: Daniel Dadap <ddadap@nvidia.com> > ... > >> +#include <linux/backlight.h> >> +#include <linux/module.h> >> +#include <linux/wmi.h> > types.h ? > mod_devicetable.h ? Evidently, these must already be implicitly picked up by the headers which I am explicitly including here. I can see the argument for types.h, since it's always possible that future changes to the kernel might remove it from the implicit include chain, but I'm not certain why you're recommending mod_devicetable.h here. If it's because I use the MODULE_DEVICE_TABLE() macro, that seems to be defined in module.h. > > ... > >> +enum wmaa_get_or_set { >> + WMAA_GET = 0, >> + WMAA_SET = 1, >> + WMAA_GET_MAX = 2 > Is it part of HW protocol? Otherwise drop assignments. Does ACPI count as HW here? I'd certainly prefer to keep the assignments, since the other side of this interface is not in the Linux kernel. > >> +}; > ... > >> + */ >> +enum wmaa_source { >> + WMAA_SOURCE_GPU = 1, >> + WMAA_SOURCE_EC = 2, >> + WMAA_SOURCE_AUX = 3 > Missed comma. Oops. I am definitely a fan of using commas here, but I removed the commas that I had in place for the last elements of these enums, and members of static initialized structs, because I was trying to more broadly apply feedback from earlier to drop the terminal comma in the static initialized device table. I realize now that this feedback was meant only for the case of the empty struct terminator element in the device table. > >> +}; > ... > >> +/** >> + * struct wmaa_args - arguments for the ACPI WMAA method >> + * > Redundant blank line. > >> + * @set: Pass in an &enum wmaa_get_or_set value to select between getting or >> + * setting a value. >> + * @val: In parameter for value to set when operating in %WMAA_SET mode. Not >> + * used in %WMAA_GET or %WMAA_GET_MAX mode. >> + * @ret: Out parameter returning retrieved value when operating in %WMAA_GET >> + * or %WMAA_GET_MAX mode. Not used in %WMAA_SET mode. >> + * @ignored: Padding; not used. The ACPI method expects a 24 byte params struct. >> + * >> + * This is the parameters structure for the ACPI WMAA method as wrapped by WMI. >> + * The value passed in to @val or returned by @ret will be a brightness value >> + * when the WMI method ID is %WMAA_LEVEL, or an &enum wmaa_source value when >> + * the WMI method ID is %WMAA_SOURCE. >> + */ > ... > >> + ret = wmi_call_wmaa(wdev, WMAA_LEVEL, WMAA_GET, &level); >> + > Ditto. > >> + if (ret) >> + dev_err(&bd->dev, "ACPI WMAA failed to get backlight level.\n"); > ... >> +static const struct backlight_ops wmaa_backlight_ops = { >> + .update_status = wmaa_backlight_update_status, >> + .get_brightness = wmaa_backlight_get_brightness > Missed comma. > >> +}; > ... > >> + if (source != WMAA_SOURCE_EC) { >> + /* This driver is only to be used when brightness control is >> + * handled by the EC; otherwise, the GPU driver(s) should handle >> + * brightness control. */ > /* > * Wrong multi-line comment > * style. Use this example to fix > * it properly. > */ > >> + return -ENODEV; >> + } >> + /* Identify this backlight device as a platform device so that it can >> + * be prioritized over any exposed GPU-driven raw device(s). */ > Ditto. > And for any other multi-line comments in this driver. > > ... > >> + backlight = devm_backlight_device_register( >> + &w->dev, "wmaa_backlight", >> + &w->dev, w, &wmaa_backlight_ops, &props); >> + if (IS_ERR(backlight)) >> + return PTR_ERR(backlight); >> + >> + return 0; > return PTR_ERR_OR_ZERO(backlight); > > ... Thanks; Thomas made the same suggestion. > >> +MODULE_AUTHOR("Aaron Plattner <aplattner@nvidia.com>"); >> +MODULE_AUTHOR("Daniel Dadap <ddadap@nvidia.com>"); > So, Co-developed-by seems missing above. > > ... > >> +MODULE_LICENSE("GPL v2"); > "GPL" is enough (it's a synonim). Indeed, Thomas pointed this out as well. I will do some retesting since I am changing the backlight driver type from BACKLIGHT_PLATFORM to BACKLIGHT_FIRMWARE, then send the updated patch.
On Wed, Sep 1, 2021 at 12:25 PM Thomas Weißschuh <linux@weissschuh.net> wrote: > > v4: Use MODULE_DEVICE_TABLE() (Thomas Wei??schuh <thomas@t-8ch.de>) > > Fix scope of internal driver state; various style fixes (Barnab??s > > P??cze, Andy Shevchenko) > > Use devm_backlight_device_register() (Barnab??s P??cze) > > Add kerneldoc comments for enums and structs (Andy Shevchenko) > > It seems your Mailsetup breaks Unicode. I see it properly, so, the problem is on your branch of email delivery.
On Do, 2021-09-02T12:28+0300, Andy Shevchenko wrote: > On Wed, Sep 1, 2021 at 12:25 PM Thomas Weißschuh <linux@weissschuh.net> wrote: > > > > v4: Use MODULE_DEVICE_TABLE() (Thomas Wei??schuh <thomas@t-8ch.de>) > > > Fix scope of internal driver state; various style fixes (Barnab??s > > > P??cze, Andy Shevchenko) > > > Use devm_backlight_device_register() (Barnab??s P??cze) > > > Add kerneldoc comments for enums and structs (Andy Shevchenko) > > > > It seems your Mailsetup breaks Unicode. > > I see it properly, so, the problem is on your branch of email delivery. It's also broken on lore.kernel.org: https://lore.kernel.org/platform-driver-x86/20210831224906.1072-1-ddadap@nvidia.com/ Anyways, as it won't be part of the history it shouldn't be urgent. Thomas
On Thu, Sep 2, 2021 at 5:37 AM Daniel Dadap <ddadap@nvidia.com> wrote: > On 9/1/21 10:57 AM, Andy Shevchenko wrote: > > On Wed, Sep 1, 2021 at 2:27 AM Daniel Dadap <ddadap@nvidia.com> wrote: ... > >> +#include <linux/backlight.h> > >> +#include <linux/module.h> > >> +#include <linux/wmi.h> > > types.h ? > > mod_devicetable.h ? > > Evidently, these must already be implicitly picked up by the headers > which I am explicitly including here. I can see the argument for > types.h, since it's always possible that future changes to the kernel > might remove it from the implicit include chain, but I'm not certain why > you're recommending mod_devicetable.h here. If it's because I use the > MODULE_DEVICE_TABLE() macro, that seems to be defined in module.h. No, it's because of the ID table type which leaves there. Basically you have to find a compromise between what to include and what to not. At least those I have mentioned are kinda core stuff that usually should be included explicitly in the modules like this driver. ... > >> +enum wmaa_get_or_set { > >> + WMAA_GET = 0, > >> + WMAA_SET = 1, > >> + WMAA_GET_MAX = 2 > > Is it part of HW protocol? Otherwise drop assignments. > > > Does ACPI count as HW here? I'd certainly prefer to keep the > assignments, since the other side of this interface is not in the Linux > kernel. Yes, that counts as FW and as you noticed out of the Linux kernel realm. > >> +}; ... > >> + */ > >> +enum wmaa_source { > >> + WMAA_SOURCE_GPU = 1, > >> + WMAA_SOURCE_EC = 2, > >> + WMAA_SOURCE_AUX = 3 > > Missed comma. > > > Oops. I am definitely a fan of using commas here, but I removed the > commas that I had in place for the last elements of these enums, and > members of static initialized structs, because I was trying to more > broadly apply feedback from earlier to drop the terminal comma in the > static initialized device table. I realize now that this feedback was > meant only for the case of the empty struct terminator element in the > device table. Not only, the _MAX in the above enum is correct in leaving commas out. > >> +}; ... > >> + backlight = devm_backlight_device_register( > >> + &w->dev, "wmaa_backlight", Strange indentation, btw. > >> + &w->dev, w, &wmaa_backlight_ops, &props);
On Thu, Sep 2, 2021 at 12:33 PM Thomas Weißschuh <linux@weissschuh.net> wrote: > On Do, 2021-09-02T12:28+0300, Andy Shevchenko wrote: > > On Wed, Sep 1, 2021 at 12:25 PM Thomas Weißschuh <linux@weissschuh.net> wrote: > > > > > > v4: Use MODULE_DEVICE_TABLE() (Thomas Wei??schuh <thomas@t-8ch.de>) > > > > Fix scope of internal driver state; various style fixes (Barnab??s > > > > P??cze, Andy Shevchenko) > > > > Use devm_backlight_device_register() (Barnab??s P??cze) > > > > Add kerneldoc comments for enums and structs (Andy Shevchenko) > > > > > > It seems your Mailsetup breaks Unicode. > > > > I see it properly, so, the problem is on your branch of email delivery. > > It's also broken on lore.kernel.org: > https://lore.kernel.org/platform-driver-x86/20210831224906.1072-1-ddadap@nvidia.com/ Ah, okay then. It seems I got a direct copy through a different chain of servers.
On Mi, 2021-09-01T21:12-0500, Daniel Dadap wrote: > Thanks, Thomas: > > On 9/1/21 4:25 AM, Thomas Weißschuh wrote: > > Hi Daniel, > > > > a few minor comments, not sure if they should be reason for a reroll. > > > I don't mind rerolling until I get an ACK from the maintainer. :) > > > > > > Thomas > > > > Reviewed-By: Thomas Weißschuh <linux@weissschuh.net> > > > > On Di, 2021-08-31T17:49-0500, Daniel Dadap wrote: > > > 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> > > > v4: Use MODULE_DEVICE_TABLE() (Thomas Wei??schuh <thomas@t-8ch.de>) > > > Fix scope of internal driver state; various style fixes (Barnab??s > > > P??cze, Andy Shevchenko) > > > Use devm_backlight_device_register() (Barnab??s P??cze) > > > Add kerneldoc comments for enums and structs (Andy Shevchenko) > > It seems your Mailsetup breaks Unicode. > > > Strange. I've just been sending the patches via git email-patch. These > characters are appearing correctly with my MUA (Thunderbird) in the copy of > the message I received, so I don't know where in the pipeline they're > getting corrupted. I'll ignore this problem unless anybody has some specific > tips on fixing it, since the non-ASCII characters aren't going to actually > be part of the git history of the patch. It seems that the encoding of the mail has been set as plain ASCII while it is actually UTF-8. Thunderbird is probly correctly inferring the actual encoding. Normally git-format-patch would detect the Unicode characters and add a matching content-type header to the patch. But as these Unicode characters are added after format-patch has run it is now git-send-emails task to add those headers again. In my version of git I get the following message from git-send-email: The following files are 8bit, but do not declare a Content-Transfer-Encoding. 0001-....patch Which 8bit encoding should I declare [UTF-8]? As this feature seems to be really old already it should be available. Is your "git email-patch" using "git send-email"? Do you have "sendemail.assume8bitEncoding" set in your git config? (It should either not be set or to UTF-8. > > > diff --git a/drivers/platform/x86/wmaa-backlight-wmi.c b/drivers/platform/x86/wmaa-backlight-wmi.c > > > new file mode 100644 > > > index 000000000000..cb1a973803b1 > > > --- /dev/null > > > +++ b/drivers/platform/x86/wmaa-backlight-wmi.c > > > @@ -0,0 +1,185 @@ > > > +// SPDX-License-Identifier: GPL-2.0-only > > > +/* > > > + * Copyright (c) 2020, NVIDIA CORPORATION. All rights reserved. > > Also 2021? > > > The code was originally written in 2020. I'm not certain it's changed > substantially enough to warrant updating the copyright date. Certainly > there's no need to change the copyright date every time the file is touched. > I suppose it wouldn't be wrong to have it read 2020-2021 or the like for the > initial commit, but I personally prefer just having the initial date unless > there's stronger guidance to do otherwise. Ok. > > > > > + */ > > > + > > > +#include <linux/backlight.h> > > > +#include <linux/module.h> > > > +#include <linux/wmi.h> > > > + > > > +/** > > > + * enum wmaa_method - WMI method IDs for ACPI WMAA > > > + * > > > + * @WMAA_LEVEL: Get or set the brightness level, > > > + * or get the maximum brightness level. > > > + * @WMAA_SOURCE: Get the source for backlight control. > > > + */ > > > +enum wmaa_method { > > > + WMAA_LEVEL = 1, > > > + WMAA_SOURCE = 2, > > > +}; > > Nitpick: No need for explicit values here. > > > I think at least the 1 is required, but in any case I prefer to be explicit > here to make it clear that these are the exact values required by the ACPI > WMAA interface. As this enum is part of the ABI to the firmware, the values should indeed have explicit values. Sorry for the noise. > > > + > > > + if (ret) > > > + dev_err(&bd->dev, "ACPI WMAA failed to get backlight level.\n"); > > Would it make sense to log the actual error? > > > I think the underlying ACPI error is already logged by wmi_call_wmaa(). Good point, disregard that comment then. > > Also while the backlight core does not seem to actually handle it, other > > drivers return a negative errno on failure. > > > Thanks, I was aware that the backlight core doesn't seem to handle errors at > all for .get_brightness, but I didn't think to check for precedent on > backlight drivers returning error codes anyway. Returning negative errno > does seem somewhat common amongst backlight drivers, so I'll go ahead and do > that in case the backlight core is updated to handle them in the future. FYI I plan on sending a patch that adds the missing errorhandling next week. > > > + props.type = BACKLIGHT_PLATFORM; > > Is this WMI method a standard interface? > > If so, BACKLIGHT_FIRMWARE might actually fit better and still fulfill the > > requirements. > > The actual maintainers would know better than me, though. > > > I'm not certain what you mean by standard. It's defined as part of system > design guidelines that NVIDIA shares with OEMs, but I'm not sure if it's > part of the ACPI spec. It is implemented by multiple notebook system > vendors, hence naming the driver after the ACPI method rather than a > particular vendor. Reading the documentation on the backlight driver types, > it does seem that this may fall more under the purview of BACKLIGHT_FIRMWARE > than BACKLIGHT_PLATFORM. I'll go ahead and retest with the driver registered > as BACKLIGHT_FIRMWARE, but I'm sure it'll still work after inspecting the > code for at least gnome-settings-daemon, which implements the policy > recommended by Documentation/ABI/stable/sysfs-class-backlight. I only went by the comment about "standard firmware interface" from backlight.h. This was mostly a pointer for you (and the pdx86 maintainers) to take a look and decide. Thomas
Hi Daniel, On 9/1/21 12:49 AM, Daniel Dadap 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. > > 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> > v4: Use MODULE_DEVICE_TABLE() (Thomas Weißschuh <thomas@t-8ch.de>) > Fix scope of internal driver state; various style fixes (Barnabás > Pőcze, Andy Shevchenko) > Use devm_backlight_device_register() (Barnabás Pőcze) > Add kerneldoc comments for enums and structs (Andy Shevchenko) Thank you for your patch, given Andy's (small) set of remarks I believe it would be best to do a v5 addressing those, after that I expect this to be ready for merging. Regards, Hans > > MAINTAINERS | 6 + > drivers/platform/x86/Kconfig | 16 ++ > drivers/platform/x86/Makefile | 1 + > drivers/platform/x86/wmaa-backlight-wmi.c | 185 ++++++++++++++++++++++ > 4 files changed, 208 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..0df908ef8d7c 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -113,6 +113,22 @@ 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 (EC). > + > + Say Y or M here if you want to control the backlight on a notebook > + system with an EC-driven backlight using the ACPI WMAA method. > + > + If you choose to compile this driver as a module the module will be > + called wmaa-backlight-wmi. > + > 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..cb1a973803b1 > --- /dev/null > +++ b/drivers/platform/x86/wmaa-backlight-wmi.c > @@ -0,0 +1,185 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2020, NVIDIA CORPORATION. All rights reserved. > + */ > + > +#include <linux/backlight.h> > +#include <linux/module.h> > +#include <linux/wmi.h> > + > +/** > + * enum wmaa_method - WMI method IDs for ACPI WMAA > + * > + * @WMAA_LEVEL: Get or set the brightness level, > + * or get the maximum brightness level. > + * @WMAA_SOURCE: Get the source for backlight control. > + */ > +enum wmaa_method { > + WMAA_LEVEL = 1, > + WMAA_SOURCE = 2, > +}; > + > +/** > + * enum wmaa_get_or_set - Operation mode for ACPI WMAA method > + * > + * @WMAA_GET: Get the current brightness level or backlight source. > + * @WMAA_SET: Set the brightness level. > + * @WMAA_GET_MAX: Get the maximum brightness level. This is only valid when the > + * WMI method is %WMAA_LEVEL. > + */ > +enum wmaa_get_or_set { > + WMAA_GET = 0, > + WMAA_SET = 1, > + WMAA_GET_MAX = 2 > +}; > + > +/** > + * enum wmaa_source - Backlight brightness control source identification > + * > + * @WMAA_SOURCE_GPU: Backlight brightness is controlled by the GPU. > + * @WMAA_SOURCE_EC: Backlight brightness is controlled by the system's > + * Embedded Controller (EC). > + * @WMAA_SOURCE_AUX: Backlight brightness is controlled over the DisplayPort > + * AUX channel. > + */ > +enum wmaa_source { > + WMAA_SOURCE_GPU = 1, > + WMAA_SOURCE_EC = 2, > + WMAA_SOURCE_AUX = 3 > +}; > + > +/** > + * struct wmaa_args - arguments for the ACPI WMAA method > + * > + * @set: Pass in an &enum wmaa_get_or_set value to select between getting or > + * setting a value. > + * @val: In parameter for value to set when operating in %WMAA_SET mode. Not > + * used in %WMAA_GET or %WMAA_GET_MAX mode. > + * @ret: Out parameter returning retrieved value when operating in %WMAA_GET > + * or %WMAA_GET_MAX mode. Not used in %WMAA_SET mode. > + * @ignored: Padding; not used. The ACPI method expects a 24 byte params struct. > + * > + * This is the parameters structure for the ACPI WMAA method as wrapped by WMI. > + * The value passed in to @val or returned by @ret will be a brightness value > + * when the WMI method ID is %WMAA_LEVEL, or an &enum wmaa_source value when > + * the WMI method ID is %WMAA_SOURCE. > + */ > +struct wmaa_args { > + u32 set; > + u32 val; > + u32 ret; > + u32 ignored[3]; > +}; > + > +static int wmi_call_wmaa(struct wmi_device *w, 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(w, 0, method, &buf, &buf); > + if (ACPI_FAILURE(status)) { > + dev_err(&w->dev, "ACPI WMAA failed with %s\n", > + acpi_format_exception(status)); > + return -EIO; > + } > + > + if (set != WMAA_SET) > + *val = args.ret; > + > + return 0; > +} > + > +static int wmaa_backlight_update_status(struct backlight_device *bd) > +{ > + struct wmi_device *wdev = bl_get_data(bd); > + > + return wmi_call_wmaa(wdev, WMAA_LEVEL, WMAA_SET, &bd->props.brightness); > +} > + > +static int wmaa_backlight_get_brightness(struct backlight_device *bd) > +{ > + struct wmi_device *wdev = bl_get_data(bd); > + u32 level = 0; > + int ret; > + > + ret = wmi_call_wmaa(wdev, WMAA_LEVEL, WMAA_GET, &level); > + > + if (ret) > + dev_err(&bd->dev, "ACPI WMAA failed to get backlight level.\n"); > + > + return level; > +} > + > +static const struct backlight_ops wmaa_backlight_ops = { > + .update_status = wmaa_backlight_update_status, > + .get_brightness = wmaa_backlight_get_brightness > +}; > + > +static int wmaa_backlight_wmi_probe(struct wmi_device *w, const void *ctx) > +{ > + struct backlight_properties props = {}; > + struct backlight_device *backlight; > + u32 source; > + int ret; > + > + ret = wmi_call_wmaa(w, WMAA_SOURCE, WMAA_GET, &source); > + if (ret) > + return ret; > + > + if (source != WMAA_SOURCE_EC) { > + /* This driver is only to be used when brightness control is > + * handled by the EC; otherwise, the GPU driver(s) should handle > + * brightness control. */ > + return -ENODEV; > + } > + > + /* Identify this backlight device as a platform device so that it can > + * be prioritized over any exposed GPU-driven raw device(s). */ > + props.type = BACKLIGHT_PLATFORM; > + > + ret = wmi_call_wmaa(w, WMAA_LEVEL, WMAA_GET_MAX, &props.max_brightness); > + if (ret) > + return ret; > + > + ret = wmi_call_wmaa(w, WMAA_LEVEL, WMAA_GET, &props.brightness); > + if (ret) > + return ret; > + > + backlight = devm_backlight_device_register( > + &w->dev, "wmaa_backlight", > + &w->dev, w, &wmaa_backlight_ops, &props); > + if (IS_ERR(backlight)) > + return PTR_ERR(backlight); > + > + return 0; > +} > + > +#define WMAA_WMI_GUID "603E9613-EF25-4338-A3D0-C46177516DB7" > + > +static const struct wmi_device_id wmaa_backlight_wmi_id_table[] = { > + { .guid_string = WMAA_WMI_GUID }, > + { } > +}; > +MODULE_DEVICE_TABLE(wmi, wmaa_backlight_wmi_id_table); > + > +static struct wmi_driver wmaa_backlight_wmi_driver = { > + .driver = { > + .name = "wmaa-backlight" > + }, > + .probe = wmaa_backlight_wmi_probe, > + .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"); >
On 9/2/21 4:35 AM, Andy Shevchenko wrote: > On Thu, Sep 2, 2021 at 5:37 AM Daniel Dadap<ddadap@nvidia.com> wrote: >> On 9/1/21 10:57 AM, Andy Shevchenko wrote: >>> On Wed, Sep 1, 2021 at 2:27 AM Daniel Dadap<ddadap@nvidia.com> wrote: > ... > >>>> +#include <linux/backlight.h> >>>> +#include <linux/module.h> >>>> +#include <linux/wmi.h> >>> types.h ? >>> mod_devicetable.h ? >> Evidently, these must already be implicitly picked up by the headers >> which I am explicitly including here. I can see the argument for >> types.h, since it's always possible that future changes to the kernel >> might remove it from the implicit include chain, but I'm not certain why >> you're recommending mod_devicetable.h here. If it's because I use the >> MODULE_DEVICE_TABLE() macro, that seems to be defined in module.h. > No, it's because of the ID table type which leaves there. > > Basically you have to find a compromise between what to include and > what to not. At least those I have mentioned are kinda core stuff that > usually should be included explicitly in the modules like this driver. Okay, thanks. I suppose I should have <linux/acpi.h> as well: I originally had that on there, then removed it when I realized that it's included implicitly via <linux/wmi.h>, but I do use some ACPI types and macros that are defined in that header. > ... > >>>> +enum wmaa_get_or_set { >>>> + WMAA_GET = 0, >>>> + WMAA_SET = 1, >>>> + WMAA_GET_MAX = 2 >>> Is it part of HW protocol? Otherwise drop assignments. >> Does ACPI count as HW here? I'd certainly prefer to keep the >> assignments, since the other side of this interface is not in the Linux >> kernel. > Yes, that counts as FW and as you noticed out of the Linux kernel realm. > >>>> +}; > ... > >>>> + */ >>>> +enum wmaa_source { >>>> + WMAA_SOURCE_GPU = 1, >>>> + WMAA_SOURCE_EC = 2, >>>> + WMAA_SOURCE_AUX = 3 >>> Missed comma. >> Oops. I am definitely a fan of using commas here, but I removed the >> commas that I had in place for the last elements of these enums, and >> members of static initialized structs, because I was trying to more >> broadly apply feedback from earlier to drop the terminal comma in the >> static initialized device table. I realize now that this feedback was >> meant only for the case of the empty struct terminator element in the >> device table. > Not only, the _MAX in the above enum is correct in leaving commas out. No, I think it does need a comma, unless I'm misunderstanding why you're saying it doesn't. WMAA_GET_MAX here isn't saying "this is the final element of the enum which is also a count of the 'real' enum values"; it's saying "retrieve the maximum valid brightness level from the firmware". I renamed the enumerant to WMAA_GET_MAX_LEVEL to avoid aliasing with the common "_MAX" convention for the final value defined in an enum. > >>>> +}; > ... > >>>> + backlight = devm_backlight_device_register( >>>> + &w->dev, "wmaa_backlight", > Strange indentation, btw. Indeed. I'm not totally sure why I did it that way, maybe because the long variable and function name didn't leave much space for arguments to indent normally? I shortened the variable name and reindented it. > >>>> + &w->dev, w, &wmaa_backlight_ops, &props);
On 9/2/21 5:19 AM, Thomas Weißschuh wrote: > It seems that the encoding of the mail has been set as plain ASCII while it is > actually UTF-8. > Thunderbird is probly correctly inferring the actual encoding. > Normally git-format-patch would detect the Unicode characters and add a > matching content-type header to the patch. > But as these Unicode characters are added after format-patch has run it is now > git-send-emails task to add those headers again. > > In my version of git I get the following message from git-send-email: > > The following files are 8bit, but do not declare a Content-Transfer-Encoding. > 0001-....patch > Which 8bit encoding should I declare [UTF-8]? > > As this feature seems to be really old already it should be available. > Is your "git email-patch" using "git send-email"? > Do you have "sendemail.assume8bitEncoding" set in your git config? > (It should either not be set or to UTF-8. Ah. That's likely the problem. The actual patch and associated commit message don't contain any characters that require more than 7 bits to encode, but the eventually sent e-mail does, since I added the information about the review revisions. Presumably git send-email is evaluating the encoding before the edit stage, but not after. sendemail.assume8bitEncoding is unset in my git config; I'll explicitly set it to UTF-8. >>>> + props.type = BACKLIGHT_PLATFORM; >>> Is this WMI method a standard interface? >>> If so, BACKLIGHT_FIRMWARE might actually fit better and still fulfill the >>> requirements. >>> The actual maintainers would know better than me, though. >> I'm not certain what you mean by standard. It's defined as part of system >> design guidelines that NVIDIA shares with OEMs, but I'm not sure if it's >> part of the ACPI spec. It is implemented by multiple notebook system >> vendors, hence naming the driver after the ACPI method rather than a >> particular vendor. Reading the documentation on the backlight driver types, >> it does seem that this may fall more under the purview of BACKLIGHT_FIRMWARE >> than BACKLIGHT_PLATFORM. I'll go ahead and retest with the driver registered >> as BACKLIGHT_FIRMWARE, but I'm sure it'll still work after inspecting the >> code for at least gnome-settings-daemon, which implements the policy >> recommended by Documentation/ABI/stable/sysfs-class-backlight. > I only went by the comment about "standard firmware interface" from > backlight.h. > This was mostly a pointer for you (and the pdx86 maintainers) to take a look > and decide. Got it. From my interpretation of linux/backlight.h and Documentation/ABI/stable/sysfs-class-backlight, I *think* BACKLIGHT_FIRMWARE is probably more appropriate, so I'll change it to that, unless a maintainer disagrees. Hans/Andy?
Hi, On 9/2/21 10:15 PM, Daniel Dadap wrote: > > On 9/2/21 5:19 AM, Thomas Weißschuh wrote: >> It seems that the encoding of the mail has been set as plain ASCII while it is >> actually UTF-8. >> Thunderbird is probly correctly inferring the actual encoding. >> Normally git-format-patch would detect the Unicode characters and add a >> matching content-type header to the patch. >> But as these Unicode characters are added after format-patch has run it is now >> git-send-emails task to add those headers again. >> >> In my version of git I get the following message from git-send-email: >> >> The following files are 8bit, but do not declare a Content-Transfer-Encoding. >> 0001-....patch >> Which 8bit encoding should I declare [UTF-8]? >> >> As this feature seems to be really old already it should be available. >> Is your "git email-patch" using "git send-email"? >> Do you have "sendemail.assume8bitEncoding" set in your git config? >> (It should either not be set or to UTF-8. > > > Ah. That's likely the problem. The actual patch and associated commit message don't contain any characters that require more than 7 bits to encode, but the eventually sent e-mail does, since I added the information about the review revisions. Presumably git send-email is evaluating the encoding before the edit stage, but not after. sendemail.assume8bitEncoding is unset in my git config; I'll explicitly set it to UTF-8. > > >>>>> + props.type = BACKLIGHT_PLATFORM; >>>> Is this WMI method a standard interface? >>>> If so, BACKLIGHT_FIRMWARE might actually fit better and still fulfill the >>>> requirements. >>>> The actual maintainers would know better than me, though. >>> I'm not certain what you mean by standard. It's defined as part of system >>> design guidelines that NVIDIA shares with OEMs, but I'm not sure if it's >>> part of the ACPI spec. It is implemented by multiple notebook system >>> vendors, hence naming the driver after the ACPI method rather than a >>> particular vendor. Reading the documentation on the backlight driver types, >>> it does seem that this may fall more under the purview of BACKLIGHT_FIRMWARE >>> than BACKLIGHT_PLATFORM. I'll go ahead and retest with the driver registered >>> as BACKLIGHT_FIRMWARE, but I'm sure it'll still work after inspecting the >>> code for at least gnome-settings-daemon, which implements the policy >>> recommended by Documentation/ABI/stable/sysfs-class-backlight. >> I only went by the comment about "standard firmware interface" from >> backlight.h. >> This was mostly a pointer for you (and the pdx86 maintainers) to take a look >> and decide. > > > Got it. From my interpretation of linux/backlight.h and Documentation/ABI/stable/sysfs-class-backlight, I *think* BACKLIGHT_FIRMWARE is probably more appropriate, so I'll change it to that, unless a maintainer disagrees. Hans/Andy? I agree that BACKLIGHT_FIRMWARE is more appropriate, that is the same as what drivers/acpi/acpi_video.c uses and this is in essence operating at the same level. Regards, Hans
On Thu, Sep 2, 2021 at 11:15 PM Daniel Dadap <ddadap@nvidia.com> wrote: > On 9/2/21 4:35 AM, Andy Shevchenko wrote: > > On Thu, Sep 2, 2021 at 5:37 AM Daniel Dadap<ddadap@nvidia.com> wrote: > >> On 9/1/21 10:57 AM, Andy Shevchenko wrote: > >>> On Wed, Sep 1, 2021 at 2:27 AM Daniel Dadap<ddadap@nvidia.com> wrote: ... > Okay, thanks. I suppose I should have <linux/acpi.h> as well: Yes. ... > >>> Missed comma. > >> Oops. I am definitely a fan of using commas here, but I removed the > >> commas that I had in place for the last elements of these enums, and > >> members of static initialized structs, because I was trying to more > >> broadly apply feedback from earlier to drop the terminal comma in the > >> static initialized device table. I realize now that this feedback was > >> meant only for the case of the empty struct terminator element in the > >> device table. > > Not only, the _MAX in the above enum is correct in leaving commas out. > > No, I think it does need a comma, unless I'm misunderstanding why you're > saying it doesn't. WMAA_GET_MAX here isn't saying "this is the final > element of the enum which is also a count of the 'real' enum values"; > it's saying "retrieve the maximum valid brightness level from the > firmware". I renamed the enumerant to WMAA_GET_MAX_LEVEL to avoid > aliasing with the common "_MAX" convention for the final value defined > in an enum. In that case, you are right.
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..0df908ef8d7c 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -113,6 +113,22 @@ 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 (EC). + + Say Y or M here if you want to control the backlight on a notebook + system with an EC-driven backlight using the ACPI WMAA method. + + If you choose to compile this driver as a module the module will be + called wmaa-backlight-wmi. + 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..cb1a973803b1 --- /dev/null +++ b/drivers/platform/x86/wmaa-backlight-wmi.c @@ -0,0 +1,185 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2020, NVIDIA CORPORATION. All rights reserved. + */ + +#include <linux/backlight.h> +#include <linux/module.h> +#include <linux/wmi.h> + +/** + * enum wmaa_method - WMI method IDs for ACPI WMAA + * + * @WMAA_LEVEL: Get or set the brightness level, + * or get the maximum brightness level. + * @WMAA_SOURCE: Get the source for backlight control. + */ +enum wmaa_method { + WMAA_LEVEL = 1, + WMAA_SOURCE = 2, +}; + +/** + * enum wmaa_get_or_set - Operation mode for ACPI WMAA method + * + * @WMAA_GET: Get the current brightness level or backlight source. + * @WMAA_SET: Set the brightness level. + * @WMAA_GET_MAX: Get the maximum brightness level. This is only valid when the + * WMI method is %WMAA_LEVEL. + */ +enum wmaa_get_or_set { + WMAA_GET = 0, + WMAA_SET = 1, + WMAA_GET_MAX = 2 +}; + +/** + * enum wmaa_source - Backlight brightness control source identification + * + * @WMAA_SOURCE_GPU: Backlight brightness is controlled by the GPU. + * @WMAA_SOURCE_EC: Backlight brightness is controlled by the system's + * Embedded Controller (EC). + * @WMAA_SOURCE_AUX: Backlight brightness is controlled over the DisplayPort + * AUX channel. + */ +enum wmaa_source { + WMAA_SOURCE_GPU = 1, + WMAA_SOURCE_EC = 2, + WMAA_SOURCE_AUX = 3 +}; + +/** + * struct wmaa_args - arguments for the ACPI WMAA method + * + * @set: Pass in an &enum wmaa_get_or_set value to select between getting or + * setting a value. + * @val: In parameter for value to set when operating in %WMAA_SET mode. Not + * used in %WMAA_GET or %WMAA_GET_MAX mode. + * @ret: Out parameter returning retrieved value when operating in %WMAA_GET + * or %WMAA_GET_MAX mode. Not used in %WMAA_SET mode. + * @ignored: Padding; not used. The ACPI method expects a 24 byte params struct. + * + * This is the parameters structure for the ACPI WMAA method as wrapped by WMI. + * The value passed in to @val or returned by @ret will be a brightness value + * when the WMI method ID is %WMAA_LEVEL, or an &enum wmaa_source value when + * the WMI method ID is %WMAA_SOURCE. + */ +struct wmaa_args { + u32 set; + u32 val; + u32 ret; + u32 ignored[3]; +}; + +static int wmi_call_wmaa(struct wmi_device *w, 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(w, 0, method, &buf, &buf); + if (ACPI_FAILURE(status)) { + dev_err(&w->dev, "ACPI WMAA failed with %s\n", + acpi_format_exception(status)); + return -EIO; + } + + if (set != WMAA_SET) + *val = args.ret; + + return 0; +} + +static int wmaa_backlight_update_status(struct backlight_device *bd) +{ + struct wmi_device *wdev = bl_get_data(bd); + + return wmi_call_wmaa(wdev, WMAA_LEVEL, WMAA_SET, &bd->props.brightness); +} + +static int wmaa_backlight_get_brightness(struct backlight_device *bd) +{ + struct wmi_device *wdev = bl_get_data(bd); + u32 level = 0; + int ret; + + ret = wmi_call_wmaa(wdev, WMAA_LEVEL, WMAA_GET, &level); + + if (ret) + dev_err(&bd->dev, "ACPI WMAA failed to get backlight level.\n"); + + return level; +} + +static const struct backlight_ops wmaa_backlight_ops = { + .update_status = wmaa_backlight_update_status, + .get_brightness = wmaa_backlight_get_brightness +}; + +static int wmaa_backlight_wmi_probe(struct wmi_device *w, const void *ctx) +{ + struct backlight_properties props = {}; + struct backlight_device *backlight; + u32 source; + int ret; + + ret = wmi_call_wmaa(w, WMAA_SOURCE, WMAA_GET, &source); + if (ret) + return ret; + + if (source != WMAA_SOURCE_EC) { + /* This driver is only to be used when brightness control is + * handled by the EC; otherwise, the GPU driver(s) should handle + * brightness control. */ + return -ENODEV; + } + + /* Identify this backlight device as a platform device so that it can + * be prioritized over any exposed GPU-driven raw device(s). */ + props.type = BACKLIGHT_PLATFORM; + + ret = wmi_call_wmaa(w, WMAA_LEVEL, WMAA_GET_MAX, &props.max_brightness); + if (ret) + return ret; + + ret = wmi_call_wmaa(w, WMAA_LEVEL, WMAA_GET, &props.brightness); + if (ret) + return ret; + + backlight = devm_backlight_device_register( + &w->dev, "wmaa_backlight", + &w->dev, w, &wmaa_backlight_ops, &props); + if (IS_ERR(backlight)) + return PTR_ERR(backlight); + + return 0; +} + +#define WMAA_WMI_GUID "603E9613-EF25-4338-A3D0-C46177516DB7" + +static const struct wmi_device_id wmaa_backlight_wmi_id_table[] = { + { .guid_string = WMAA_WMI_GUID }, + { } +}; +MODULE_DEVICE_TABLE(wmi, wmaa_backlight_wmi_id_table); + +static struct wmi_driver wmaa_backlight_wmi_driver = { + .driver = { + .name = "wmaa-backlight" + }, + .probe = wmaa_backlight_wmi_probe, + .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");