| Message ID | 20210902214718.13628-1-ddadap@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v5] platform/x86: Add driver for ACPI WMAA EC-based backlight control | expand |
Hi Daniel, one more actual thing and two tiny nitpicks. On 2021-09-02T16:47-0500, Daniel Dadap wrote: > 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). system's -> systems > + > + 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/wmaa-backlight-wmi.c b/drivers/platform/x86/wmaa-backlight-wmi.c > new file mode 100644 > index 000000000000..fa3f41302299 > --- /dev/null > +++ b/drivers/platform/x86/wmaa-backlight-wmi.c > @@ -0,0 +1,194 @@ > [..] > +static int wmaa_backlight_update_status(struct backlight_device *bd) > +{ > + struct wmi_device *wdev = bl_get_data(bd); > + > + return wmi_call_wmaa(wdev, WMAA_METHOD_LEVEL, WMAA_MODE_SET, > + &bd->props.brightness); > +} > + > +static int wmaa_backlight_get_brightness(struct backlight_device *bd) > +{ > + struct wmi_device *wdev = bl_get_data(bd); > + u32 level; > + int ret; > + > + ret = wmi_call_wmaa(wdev, WMAA_METHOD_LEVEL, WMAA_MODE_GET, &level); > + if (ret > 0) > + return -EINVAL; > + else if (ret < 0) > + return ret; Nowhere else is the return value of wmi_call_wmaa() checked explicitly for greater than zero and less than zero. Why here? At least wmaa_backlight_update_status() should have identical semantics. The case ret > 0 should never happen. > + > + return level; > +} > [..] > + > + > +#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 }, This could also be inlined as: { "603E9613-EF25-4338-A3D0-C46177516DB7" }, More a stylistic thing though. Thomas
On 9/2/21 6:20 PM, Thomas Weißschuh wrote: > Hi Daniel, > > one more actual thing and two tiny nitpicks. > > On 2021-09-02T16:47-0500, Daniel Dadap wrote: >> 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). > system's -> systems "system's" is what I intended, and I believe it's correct. The meaning I'm going for here is "the Embedded Controller (EC) belonging to the system". I guess maybe there could be some confusion because earlier in the same sentence I refer to "systems" plural, referring to the class of systems that expose this backlight control scheme, and later in the sentence "system's" refers to something belonging to a singular member of this class. It would probably be more clear if I just drop the word "system's" here. > >> + >> + 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/wmaa-backlight-wmi.c b/drivers/platform/x86/wmaa-backlight-wmi.c >> new file mode 100644 >> index 000000000000..fa3f41302299 >> --- /dev/null >> +++ b/drivers/platform/x86/wmaa-backlight-wmi.c >> @@ -0,0 +1,194 @@ >> [..] >> +static int wmaa_backlight_update_status(struct backlight_device *bd) >> +{ >> + struct wmi_device *wdev = bl_get_data(bd); >> + >> + return wmi_call_wmaa(wdev, WMAA_METHOD_LEVEL, WMAA_MODE_SET, >> + &bd->props.brightness); >> +} >> + >> +static int wmaa_backlight_get_brightness(struct backlight_device *bd) >> +{ >> + struct wmi_device *wdev = bl_get_data(bd); >> + u32 level; >> + int ret; >> + >> + ret = wmi_call_wmaa(wdev, WMAA_METHOD_LEVEL, WMAA_MODE_GET, &level); >> + if (ret > 0) >> + return -EINVAL; >> + else if (ret < 0) >> + return ret; > Nowhere else is the return value of wmi_call_wmaa() checked explicitly for > greater than zero and less than zero. Why here? > At least wmaa_backlight_update_status() should have identical semantics. > The case ret > 0 should never happen. wmaa_backlight_update_status() has different semantics precisely because the .get_brightness() and .update_status() callbacks registered in the struct backlight_ops have different semantics. The return value of .get_brightness() serves dual duty, where a positive return value could be interpreted as a valid brightness level value, while in the case of .update_status() a positive return value is invalid. I agree that wmi_call_wmaa() should never return > 0, but I wanted to protect against someone deciding in the future (for whatever reason) to add a code path that returns a positive value, without checking all of the callers of wmi_call_wmaa() to realize that this could cause a problem. There are not many callers of the function, though, and they're all in the same file, so perhaps this is not something worth worrying about. I suppose I could check for just the < 0 case, and add document the API of wmi_call_wmaa(), explicitly stating that the return value should be zero or negative errno. > >> + >> + return level; >> +} >> [..] >> + >> + >> +#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 }, > This could also be inlined as: > { "603E9613-EF25-4338-A3D0-C46177516DB7" }, > > More a stylistic thing though. I considered that when switching to MODULE_DEVICE_TABLE, since the value is now only used in one place, but looking at other similar drivers, it does seem that the most common convention is to define the GUID as a macro even if it's only used once. I'll leave this as-is, I think.
On Fri, Sep 3, 2021 at 3:22 AM Daniel Dadap <ddadap@nvidia.com> wrote: > On 9/2/21 6:20 PM, Thomas Weißschuh wrote: ... > >> +#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 }, > > This could also be inlined as: > > { "603E9613-EF25-4338-A3D0-C46177516DB7" }, > > > > More a stylistic thing though. > > I considered that when switching to MODULE_DEVICE_TABLE, since the value > is now only used in one place, but looking at other similar drivers, it > does seem that the most common convention is to define the GUID as a > macro even if it's only used once. I'll leave this as-is, I think. I'm on Daniel's side here. But the problem I have noticed with the proposal is different, i.e. it loses the C99 style of member definitions.
On 9/3/21 3:14 AM, Andy Shevchenko wrote: > On Fri, Sep 3, 2021 at 3:22 AM Daniel Dadap <ddadap@nvidia.com> wrote: >> On 9/2/21 6:20 PM, Thomas Weißschuh wrote: > ... > >>>> +#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 }, >>> This could also be inlined as: >>> { "603E9613-EF25-4338-A3D0-C46177516DB7" }, >>> >>> More a stylistic thing though. >> I considered that when switching to MODULE_DEVICE_TABLE, since the value >> is now only used in one place, but looking at other similar drivers, it >> does seem that the most common convention is to define the GUID as a >> macro even if it's only used once. I'll leave this as-is, I think. > I'm on Daniel's side here. But the problem I have noticed with the > proposal is different, i.e. it loses the C99 style of member > definitions. Oh. I didn't even notice that the proposal excluded the designated initializer. What I had been considering was inlining the GUID string and getting rid of the preprocessor macro, while retaining the designated initializer. In any case, the prevailing style does seem to be to avoid inlining the constant, AFAICT.
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..fa3f41302299 --- /dev/null +++ b/drivers/platform/x86/wmaa-backlight-wmi.c @@ -0,0 +1,194 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2020, NVIDIA CORPORATION. All rights reserved. + */ + +#include <linux/acpi.h> +#include <linux/backlight.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/types.h> +#include <linux/wmi.h> + +/** + * enum wmaa_method - WMI method IDs for ACPI WMAA + * @WMAA_METHOD_LEVEL: Get or set the brightness level, + * or get the maximum brightness level. + * @WMAA_METHOD_SOURCE: Get the source for backlight control. + */ +enum wmaa_method { + WMAA_METHOD_LEVEL = 1, + WMAA_METHOD_SOURCE = 2, + WMAA_METHOD_MAX +}; + +/** + * enum wmaa_mode - Operation mode for ACPI WMAA method + * @WMAA_MODE_GET: Get the current brightness level or source. + * @WMAA_MODE_SET: Set the brightness level. + * @WMAA_MODE_GET_MAX_LEVEL: Get the maximum brightness level. This is only + * valid when the WMI method is %WMAA_METHOD_LEVEL. + */ +enum wmaa_mode { + WMAA_MODE_GET = 0, + WMAA_MODE_SET = 1, + WMAA_MODE_GET_MAX_LEVEL = 2, + WMAA_MODE_MAX +}; + +/** + * 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, + WMAA_SOURCE_MAX +}; + +/** + * struct wmaa_args - arguments for the ACPI WMAA method + * @mode: Pass in an &enum wmaa_mode value to select between getting or + * setting a value. + * @val: In parameter for value to set when operating in %WMAA_MODE_SET + * mode. Not used in %WMAA_MODE_GET or %WMAA_MODE_GET_MAX_LEVEL mode. + * @ret: Out parameter returning retrieved value when operating in + * %WMAA_MODE_GET or %WMAA_MODE_GET_MAX_LEVEL mode. Not used in + * %WMAA_MODE_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_METHOD_LEVEL, or an &enum wmaa_source value + * when the WMI method ID is %WMAA_METHOD_SOURCE. + */ +struct wmaa_args { + u32 mode; + u32 val; + u32 ret; + u32 ignored[3]; +}; + +static int wmi_call_wmaa(struct wmi_device *w, enum wmaa_method id, enum wmaa_mode mode, u32 *val) +{ + struct wmaa_args args = { + .mode = mode, + .val = 0, + .ret = 0, + }; + struct acpi_buffer buf = { (acpi_size)sizeof(args), &args }; + acpi_status status; + + if (id < WMAA_METHOD_LEVEL || id >= WMAA_METHOD_MAX || + mode < WMAA_MODE_GET || mode >= WMAA_MODE_MAX) + return -EINVAL; + + if (mode == WMAA_MODE_SET) + args.val = *val; + + status = wmidev_evaluate_method(w, 0, id, &buf, &buf); + if (ACPI_FAILURE(status)) { + dev_err(&w->dev, "ACPI WMAA failed: %s\n", + acpi_format_exception(status)); + return -EIO; + } + + if (mode != WMAA_MODE_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_METHOD_LEVEL, WMAA_MODE_SET, + &bd->props.brightness); +} + +static int wmaa_backlight_get_brightness(struct backlight_device *bd) +{ + struct wmi_device *wdev = bl_get_data(bd); + u32 level; + int ret; + + ret = wmi_call_wmaa(wdev, WMAA_METHOD_LEVEL, WMAA_MODE_GET, &level); + if (ret > 0) + return -EINVAL; + else if (ret < 0) + return ret; + + 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 *wdev, const void *ctx) +{ + struct backlight_properties props = {}; + struct backlight_device *bdev; + u32 source; + int ret; + + ret = wmi_call_wmaa(wdev, WMAA_METHOD_SOURCE, WMAA_MODE_GET, &source); + if (ret) + return ret; + + /* + * This driver is only to be used when brightness control is handled + * by the EC; otherwise, the GPU driver(s) should control brightness. + */ + if (source != WMAA_SOURCE_EC) + return -ENODEV; + + /* + * Identify this backlight device as a firmware device so that it can + * be prioritized over any exposed GPU-driven raw device(s). + */ + props.type = BACKLIGHT_FIRMWARE; + + ret = wmi_call_wmaa(wdev, WMAA_METHOD_LEVEL, WMAA_MODE_GET_MAX_LEVEL, + &props.max_brightness); + if (ret) + return ret; + + ret = wmi_call_wmaa(wdev, WMAA_METHOD_LEVEL, WMAA_MODE_GET, + &props.brightness); + if (ret) + return ret; + + bdev = devm_backlight_device_register(&wdev->dev, "wmaa_backlight", + &wdev->dev, wdev, + &wmaa_backlight_ops, &props); + return PTR_ERR_OR_ZERO(bdev); +} + +#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("Daniel Dadap <ddadap@nvidia.com>"); +MODULE_DESCRIPTION("WMAA Backlight WMI driver"); +MODULE_LICENSE("GPL");