Hi Pali, On 9/20/21 3:29 PM, Pali Rohár wrote: > On Monday 13 September 2021 11:01:50 Hans de Goede wrote: >> Hi, >> >> On 9/3/21 2:38 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: Daniel Dadap <ddadap@xxxxxxxxxx> >>> Reviewed-By: Thomas Weißschuh <linux@xxxxxxxxxxxxxx> >> >> Thank you for your patch, I've applied this patch to my review-hans >> branch: >> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans >> >> Note it will show up in my review-hans branch once I've pushed my >> local branch there, which might take a while. >> >> Once I've run some tests on this branch the patches there will be >> added to the platform-drivers-x86/for-next branch and eventually >> will be included in the pdx86 pull-request to Linus for the next >> merge-window. > > Hello Hans! > > I would suggest to rename this driver and config option to not include > -AA in its name. WMAA is just internal name of ACPI method, composed > from two parts: "WM" and "AA". Second part "AA" is read from the _WDG > where is the translation table from WMI GUID (in this case 603E9613...) > to ACPI method name. "AA" is just autogenerated identifier by wmi > compiler and because names are ASCII strings, I guess "AA" could mean > the first (autogenerated) method. In the whole driver code you are not > using AA function name, but directly WMI GUID, which also means that > driver is prepared if vendor "recompiles" wmi code in acpi (and compiler > generates another identifier, not AA). Also another argument is that > there can be lot of other laptops which have WMAA ACPI method but they > can have different API or do something totally different. So name WMAA > is in this wmi context very misleading. Rather it should be named by > vendor. Right, that is a very valid point. I should have spotted this myself. So what would be a better name wmi-nvidia-backlight.ko I guess ? (and update the rest to match ?) > Anyway, could you provide BMF code for this wmi acpi functionality? In > BMF could be encoded more informations, including real, meaning human > readable name of this function. Not only internal (autogenerated) AA > identifier. BMF can be extracted from dumps created by 'acpidump' tool > or with new kernels directly from files 'find /sys/ -name bmof'. I will leave answering this part up to Daniel Dadap, since I don't have access to any of this. Regards, Hans > For example Windows drivers use directly those real function names and > let Windows NT kernel to translate real function names. So if for this > functionality there is Windows driver, its .ini (or other text files) > could also provide more information. > > In past I have written bmf2mof decompiler which decompiles binary BMF > code back to MOF source code: https://github.com/pali/bmfdec > > So if you extract BMF code from that machine, then this bmf2mof tool can > help with developing or identifying parts of wmi, and showing real > function names. Sometimes there are also comments/descriptions in binary > BMF code (but due to its format, not readable by 'strings', and > decompilation is needed) which can provide even more information about > usage. > >> Regards, >> >> Hans >> >> >>> --- >>> >>> v2: Convert to WMI subsystem driver, suggested by Mario Limonciello >>> <mario.limonciello@xxxxxxxxxxx>; various cleanups suggested by >>> Barnabás Pőcze <pobrn@xxxxxxxxxxxxxx> >>> v3: Address assorted style nits raised by Andy Shevchenko >>> <andy.shevchenko@xxxxxxxxx> in response to a related patch; remove >>> additional behavior change to WMI subsystem from patch series as >>> recommended by Hans de Goede <hdegoede@xxxxxxxxxx> >>> v4: Use MODULE_DEVICE_TABLE() (Thomas Weißschuh <thomas@xxxxxxxx>) >>> 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) >>> v5: Remove Aaron Plattner as author, as suggested by Aaron Plattner >>> <aplattner@xxxxxxxxxx> >>> Register as BACKLIGHT_FIRMWARE rather than BACKLIGHT_PLATFORM; >>> Return negative errno if .get_brightness() fails (Thomas Weißschuh) >>> Assorted style improvements (Andy Shevchenko, Thomas Weißschuh) >>> v6: Make behavior of callers of wmi_call_wmaa() more consistent; >>> simplify potentially confusing description in Kconfig help text >>> (Thomas Weißschuh) >>> >>> MAINTAINERS | 6 + >>> drivers/platform/x86/Kconfig | 16 ++ >>> drivers/platform/x86/Makefile | 1 + >>> drivers/platform/x86/wmaa-backlight-wmi.c | 205 ++++++++++++++++++++++ >>> 4 files changed, 228 insertions(+) >>> create mode 100644 drivers/platform/x86/wmaa-backlight-wmi.c >>> >>> diff --git a/MAINTAINERS b/MAINTAINERS >>> index bbaecde94aa0..fd7362a86c6d 100644 >>> --- a/MAINTAINERS >>> +++ b/MAINTAINERS >>> @@ -20008,6 +20008,12 @@ L: linux-wireless@xxxxxxxxxxxxxxx >>> S: Odd fixes >>> F: drivers/net/wireless/wl3501* >>> >>> +WMAA BACKLIGHT DRIVER >>> +M: Daniel Dadap <ddadap@xxxxxxxxxx> >>> +L: platform-driver-x86@xxxxxxxxxxxxxxx >>> +S: Supported >>> +F: drivers/platform/x86/wmaa-backlight-wmi.c >>> + >>> WOLFSON MICROELECTRONICS DRIVERS >>> L: patches@xxxxxxxxxxxxxxxxxxxxx >>> S: Supported >>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig >>> index d12db6c316ea..00f021eda25c 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 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..f5c4f8337c2c >>> --- /dev/null >>> +++ b/drivers/platform/x86/wmaa-backlight-wmi.c >>> @@ -0,0 +1,205 @@ >>> +// 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]; >>> +}; >>> + >>> +/** >>> + * wmi_call_wmaa() - helper function for calling ACPI WMAA via its WMI wrapper >>> + * @w: Pointer to the struct wmi_device identified by %WMAA_WMI_GUID >>> + * @id: The method ID for the ACPI WMAA method (e.g. %WMAA_METHOD_LEVEL or >>> + * %WMAA_METHOD_SOURCE) >>> + * @mode: The operation to perform on the ACPI WMAA method (e.g. %WMAA_MODE_SET >>> + * or %WMAA_MODE_GET) >>> + * @val: Pointer to a value passed in by the caller when @mode is >>> + * %WMAA_MODE_SET, or a value passed out to the caller when @mode is >>> + * %WMAA_MODE_GET or %WMAA_MODE_GET_MAX_LEVEL. >>> + * >>> + * Returns 0 on success, or a negative error number on failure. >>> + */ >>> +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 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@xxxxxxxxxx>"); >>> +MODULE_DESCRIPTION("WMAA Backlight WMI driver"); >>> +MODULE_LICENSE("GPL"); >>> >> >
