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@xxxxxxxxxxxxxx>
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@xxxxxxxxxx>
Signed-off-by: Daniel Dadap <ddadap@xxxxxxxxxx>
---
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)
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@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..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@xxxxxxxxxx>");
+MODULE_AUTHOR("Daniel Dadap <ddadap@xxxxxxxxxx>");
+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)
