I am no authority here to say if this patch is good or bad, but I hope to help with my comments. > A number of upcoming notebook computer designs drive the internal > display panel's backlight PWM through the Embedded Controller (EC). > This EC-based backlight control can be plumbed through to an ACPI > "WMAA" method interface, which in turn can be wrapped by WMI with > the GUID handle 603E9613-EF25-4338-A3D0-C46177516DB7. > > Add a new driver, aliased to the WMAA WMI GUID, to expose a sysfs > backlight class driver to control backlight levels on systems with > EC-driven backlights. > > Signed-off-by: Aaron Plattner <aplattner@xxxxxxxxxx> > Signed-off-by: Daniel Dadap <ddadap@xxxxxxxxxx> > --- > MAINTAINERS | 6 + > drivers/platform/x86/Kconfig | 11 ++ > drivers/platform/x86/Makefile | 2 + > drivers/platform/x86/wmaa-backlight-wmi.c | 153 ++++++++++++++++++++++ > 4 files changed, 172 insertions(+) > create mode 100644 drivers/platform/x86/wmaa-backlight-wmi.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index eeff55560759..e5ce6544a3c8 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -18249,6 +18249,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 > T: git https://github.com/CirrusLogic/linux-drivers.git > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 0ad7ad8cf8e1..db342e480aa9 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -1368,6 +1368,17 @@ config INTEL_TELEMETRY > directly via debugfs files. Various tools may use > this interface for SoC state monitoring. > > +config WMAA_BACKLIGHT_WMI > + tristate "ACPI WMAA Backlight Driver" > + depends on ACPI_WMI > + depends on ACPI > + 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. > + > endif # X86_PLATFORM_DEVICES > > config PMC_ATOM > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > index 53408d965874..fb6e16d62031 100644 > --- a/drivers/platform/x86/Makefile > +++ b/drivers/platform/x86/Makefile > @@ -146,3 +146,5 @@ obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o intel_telemetry_pltdrv.o intel_telemetry_debugfs.o > obj-$(CONFIG_PMC_ATOM) += pmc_atom.o > + > +obj-$(CONFIG_WMAA_BACKLIGHT_WMI) += wmaa-backlight-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..890e9371f91a > --- /dev/null > +++ b/drivers/platform/x86/wmaa-backlight-wmi.c > @@ -0,0 +1,153 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * WMAA Backlight WMI driver > + * > + * Copyright (c) 2020, NVIDIA CORPORATION. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + */ > + > +#include <linux/module.h> > +#include <linux/acpi.h> > +#include <linux/backlight.h> > + > +MODULE_AUTHOR("Aaron Plattner <aplattner@xxxxxxxxxx>"); > +MODULE_AUTHOR("Daniel Dadap <ddadap@xxxxxxxxxx>"); > +MODULE_DESCRIPTION("WMAA Backlight WMI driver"); > +MODULE_LICENSE("GPL v2"); > + > +#define WMAA_WMI_GUID "603E9613-EF25-4338-A3D0-C46177516DB7" > + > +MODULE_ALIAS("wmi:"WMAA_WMI_GUID); > + > +static struct backlight_device *backlight; > + > +enum wmaa_method { > + WMAA_BRIGHTNESS_LEVEL = 1, > + WMAA_BRIGHTNESS_SOURCE = 2, > +}; > + > +enum wmaa_get_or_set { > + WMAA_GET = 0, > + WMAA_SET = 1, > + WMAA_GET_MAX = 2, // for WMAA_BRIGHTNESS_LEVEL only > +}; > + > +enum wmaa_source { > + WMAA_SOURCE_CLEAR = 0, > + WMAA_SOURCE_GPU = 1, > + WMAA_SOURCE_EC = 2, > + WMAA_SOURCE_AUX = 3, > + WMAA_SOURCE_COUNT > +}; > + > +struct wmaa_args { > + u32 set; > + u32 val; > + u32 ret; > + u32 ignored[3]; > +}; > + > +static int wmi_call_wmaa(enum wmaa_method method, enum wmaa_get_or_set set, > + u32 *val) > +{ > + struct wmaa_args args = { > + .set = set, > + .val = 0, > + .ret = 0, > + }; > + struct acpi_buffer buf = { (acpi_size)sizeof(args), &args }; > + acpi_status status; > + > + if (set == WMAA_SET) > + args.val = *val; > + > + status = wmi_evaluate_method(WMAA_WMI_GUID, 0, method, &buf, &buf); > + if (ACPI_FAILURE(status)) > + return status; > + if (set != WMAA_SET) > + *val = args.ret; > + return status; > +} > + This might be a moot point, but can't the conversion from acpi_status to int have unwanted effects? And furthermore, as far as I see, ACPICA error constants do not map to Exxxx constants, so either an Exxxx constant should be returned from wmi_call_wmaa() on error, or it should return an acpi_status value, and the conversion should be handled in the functions using wmi_call_wmaa(), in my opinion. > +static int wmaa_get_brightness(u32 *level) > +{ > + return wmi_call_wmaa(WMAA_BRIGHTNESS_LEVEL, WMAA_GET, level); > +} > + > +static int wmaa_set_brightness(u32 level) > +{ > + return wmi_call_wmaa(WMAA_BRIGHTNESS_LEVEL, WMAA_SET, &level); > +} > + > +static int wmaa_backlight_update_status(struct backlight_device *bd) > +{ > + return wmaa_set_brightness(bd->props.brightness); > +} > + > +static int wmaa_backlight_get_brightness(struct backlight_device *bd) > +{ > + u32 level; > + int ret = wmaa_get_brightness(&level); > + > + WARN_ON(ret != 0); > + return level; > +} > + Also another moot point, but wouldn't it be "safer" (certainly more deterministic) to return zero if something goes wrong? (As far as I see, the error codes are not handled by the backlight subsystem for get_brightness.) > +static const struct backlight_ops wmaa_backlight_ops = { > + .update_status = wmaa_backlight_update_status, > + .get_brightness = wmaa_backlight_get_brightness, > +}; > + > +static int wmaa_get_max_brightness(u32 *level) > +{ > + return wmi_call_wmaa(WMAA_BRIGHTNESS_LEVEL, WMAA_GET_MAX, level); > +} > + > +static int wmaa_get_brightness_source(u32 *source) > +{ > + return wmi_call_wmaa(WMAA_BRIGHTNESS_SOURCE, WMAA_GET, source); > +} > + > +static int __init wmaa_backlight_wmi_init(void) > +{ > + struct backlight_properties props; And a third possibly insignificant point: shouldn't the power and scale members of props be initialized as well? > + u32 source; > + > + if (!wmi_has_guid(WMAA_WMI_GUID)) > + return -ENODEV; > + > + if (wmaa_get_brightness_source(&source)) > + return -EINVAL; > + if (source != WMAA_SOURCE_EC) > + return -ENODEV; > + > + // Register a backlight handler > + props.type = BACKLIGHT_PLATFORM; > + if (wmaa_get_max_brightness(&props.max_brightness) || > + wmaa_get_brightness(&props.brightness)) > + return -EINVAL; > + > + backlight = backlight_device_register("wmaa_backlight", NULL, NULL, > + &wmaa_backlight_ops, &props); > + if (IS_ERR(backlight)) > + return PTR_ERR(backlight); > + > + return 0; > +} > + > +static void __exit wmaa_backlight_wmi_exit(void) > +{ > + backlight_device_unregister(backlight); > +} > + > +module_init(wmaa_backlight_wmi_init); > +module_exit(wmaa_backlight_wmi_exit); > -- > 2.18.4 I think this should be sent to the subsystem maintainers as well, so I CCd them. Barnabás Pőcze
