Thanks for your comments. I'll send a v2 patch as soon as I can get an
answer about the values for the power and scale members of the struct
backlight_properties record. Additional responses below:
On 7/27/20 5:15 PM, Barnabás Pőcze wrote:
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.
Good point. I've adjusted this to return -EIO in the case of an ACPI
error, with a print to indicate the particular error.
+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.)
Perhaps; however, it does seem that at least some other backlight
handlers return negative error codes from their get_brightness
functions, so I'll wait for comment from the subsystem maintainers.
+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?
Probably. I'll need to check to see what the appropriate values are for
this hardware. In the meantime I'll zero-initialize the whole struct
until I can get the correct values.
+ 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
