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.
