Re: [PATCH v2 2/4] platform/x86: Add Lenovo GameZone WMI Driver
From: Armin Wolf
Date: Fri Jan 10 2025 - 18:24:26 EST
Am 10.01.25 um 22:33 schrieb Derek John Clark:
On Thu, Jan 9, 2025 at 2:12 PM Armin Wolf <W_Armin@xxxxxx> wrote:
Am 02.01.25 um 01:47 schrieb Derek J. Clark:
Adds lenovo-wmi-gamezone.c which provides a driver for the Lenovo
GameZone WMI interface that comes on Lenovo "Gaming Series" hardware.
Provides ACPI platform profiles over WMI.
v2:
- Use devm_kzalloc to ensure driver can be instanced, remove global
reference.
- Ensure reverse Christmas tree for all variable declarations.
- Remove extra whitespace.
- Use guard(mutex) in all mutex instances, global mutex.
- Use pr_fmt instead of adding the driver name to each pr_err.
- Remove noisy pr_info usage.
- Rename gamezone_wmi to lenovo_wmi_gz_priv and gz_wmi to priv.
- Remove GZ_WMI symbol exporting.
Signed-off-by: Derek J. Clark <derekjohn.clark@xxxxxxxxx>
---
MAINTAINERS | 7 +
drivers/platform/x86/Kconfig | 11 ++
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/lenovo-wmi-gamezone.c | 203 +++++++++++++++++++++
drivers/platform/x86/lenovo-wmi.h | 105 +++++++++++
5 files changed, 327 insertions(+)
create mode 100644 drivers/platform/x86/lenovo-wmi-gamezone.c
create mode 100644 drivers/platform/x86/lenovo-wmi.h
diff --git a/MAINTAINERS b/MAINTAINERS
index baf0eeb9a355..8f8a6aec6b92 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13034,6 +13034,13 @@ S: Maintained
W: http://legousb.sourceforge.net/
F: drivers/usb/misc/legousbtower.c
+LENOVO WMI drivers
+M: Derek J. Clark <derekjohn.clark@xxxxxxxxx>
+L: platform-driver-x86@xxxxxxxxxxxxxxx
+S: Maintained
+F: drivers/platform/x86/lenovo-wmi-gamezone.c
+F: drivers/platform/x86/lenovo-wmi.h
+
LETSKETCH HID TABLET DRIVER
M: Hans de Goede <hdegoede@xxxxxxxxxx>
L: linux-input@xxxxxxxxxxxxxxx
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 0258dd879d64..9a6ac7fdec9f 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -459,6 +459,17 @@ config IBM_RTL
state = 0 (BIOS SMIs on)
state = 1 (BIOS SMIs off)
+config LENOVO_WMI_GAMEZONE
+ tristate "Lenovo GameZone WMI Driver"
+ depends on ACPI_WMI
+ select ACPI_PLATFORM_PROFILE
+ help
+ Say Y here if you have a WMI aware Lenovo Legion device and would like to use the
+ platform-profile firmware interface.
+
+ To compile this driver as a module, choose M here: the module will
+ be called lenovo_wmi_gamezone.
Could it be that the resulting kernel module is actually named lenovo-wmi-gamezone?.
If yes then please adjust the config description.
the .o/.ko are named as you described with -, but lsmod lists them
with _ which is how most would interact with the driver if manually
loading or blocking it. I'll put whichever you think is most
appropriate.
I would prefer if you use the .ko names.
+
config IDEAPAD_LAPTOP
tristate "Lenovo IdeaPad Laptop Extras"
depends on ACPI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index e1b142947067..7cb29a480ed2 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_THINKPAD_LMI) += think-lmi.o
obj-$(CONFIG_YOGABOOK) += lenovo-yogabook.o
obj-$(CONFIG_YT2_1380) += lenovo-yoga-tab2-pro-1380-fastcharger.o
obj-$(CONFIG_LENOVO_WMI_CAMERA) += lenovo-wmi-camera.o
+obj-$(CONFIG_LENOVO_WMI_GAMEZONE) += lenovo-wmi-gamezone.o
# Intel
obj-y += intel/
diff --git a/drivers/platform/x86/lenovo-wmi-gamezone.c b/drivers/platform/x86/lenovo-wmi-gamezone.c
new file mode 100644
index 000000000000..da5e2bc41f39
--- /dev/null
+++ b/drivers/platform/x86/lenovo-wmi-gamezone.c
@@ -0,0 +1,203 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Lenovo GameZone WMI interface driver. The GameZone WMI interface provides
+ * platform profile and fan curve settings for devices that fall under the
+ * "Gaming Series" of Lenovo Legion devices.
+ *
+ * Copyright(C) 2024 Derek J. Clark <derekjohn.clark@xxxxxxxxx>
+ */
+
+#include <linux/platform_profile.h>
+#include "lenovo-wmi.h"
Please add the necessary includes here and do not rely on the header file to pull them in.
Ack
+
+#define LENOVO_GAMEZONE_GUID "887B54E3-DDDC-4B2C-8B88-68A26A8835D0"
+
+/* Method IDs */
+#define WMI_METHOD_ID_SMARTFAN_SUPP 43 /* IsSupportSmartFan */
+#define WMI_METHOD_ID_SMARTFAN_SET 44 /* SetSmartFanMode */
+#define WMI_METHOD_ID_SMARTFAN_GET 45 /* GetSmartFanMode */
+
+static DEFINE_MUTEX(call_mutex);
+
+static const struct wmi_device_id lenovo_wmi_gamezone_id_table[] = {
+ { LENOVO_GAMEZONE_GUID, NULL }, /* LENOVO_GAMEZONE_DATA */
+ {}
+};
Please move those device IDs closer to the driver struct which uses them.
Ack
+
+struct lenovo_wmi_gz_priv {
+ struct wmi_device *wdev;
+ enum platform_profile_option current_profile;
+ struct platform_profile_handler pprof;
+ bool platform_profile_support;
+};
+
+/* Platform Profile Methods */
+static int lenovo_wmi_gamezone_platform_profile_supported(
+ struct platform_profile_handler *pprof, int *supported)
Please use ./scripts/checkpatch --strict to catch any coding style violations like this one.
Ack. Sorry about that.
+{
+ struct lenovo_wmi_gz_priv *priv;
+
+ priv = container_of(pprof, struct lenovo_wmi_gz_priv, pprof);
Is there a reason why you are not passing priv as an argument? If no then please pass priv
as an argument so you can avoid having to use container_of().
+
+ guard(mutex)(&call_mutex);
Is there a technical reason why you have to use a mutex for WMI method access? If no then please remove
this mutex.
We weren't sure and figured you would know best practice. I'll remove them.
+ return lenovo_wmidev_evaluate_method_1(
+ priv->wdev, 0x0, WMI_METHOD_ID_SMARTFAN_SUPP, 0, supported);
+}
+
+static int
+lenovo_wmi_gamezone_profile_get(struct platform_profile_handler *pprof,
+ enum platform_profile_option *profile)
+{
+ struct lenovo_wmi_gz_priv *priv;
+ int sel_prof;
+ int err;
+
+ priv = container_of(pprof, struct lenovo_wmi_gz_priv, pprof);
+
+ guard(mutex)(&call_mutex);
+ err = lenovo_wmidev_evaluate_method_1(
+ priv->wdev, 0x0, WMI_METHOD_ID_SMARTFAN_GET, 0, &sel_prof);
+ if (err) {
+ pr_err("Error getting fan profile from WMI interface: %d\n",
+ err);
Please just return here without printing anything, userspace does not benefit from such
an error message which only states the obvious.
Ack all debug return messages.
+ return err;
+ }
+
+ switch (sel_prof) {
+ case SMARTFAN_MODE_QUIET:
+ *profile = PLATFORM_PROFILE_QUIET;
+ break;
+ case SMARTFAN_MODE_BALANCED:
+ *profile = PLATFORM_PROFILE_BALANCED;
+ break;
+ case SMARTFAN_MODE_PERFORMANCE:
+ *profile = PLATFORM_PROFILE_PERFORMANCE;
+ break;
+ case SMARTFAN_MODE_CUSTOM:
+ *profile = PLATFORM_PROFILE_CUSTOM;
+ break;
+ default:
+ return -EINVAL;
+ }
+ priv->current_profile = *profile;
Please remove this unused variable from priv.
Ack
+
+ return 0;
+}
+
+static int
+lenovo_wmi_gamezone_profile_set(struct platform_profile_handler *pprof,
+ enum platform_profile_option profile)
+{
+ struct lenovo_wmi_gz_priv *priv;
+ int sel_prof;
+ int err;
+
+ switch (profile) {
+ case PLATFORM_PROFILE_QUIET:
+ sel_prof = SMARTFAN_MODE_QUIET;
+ break;
+ case PLATFORM_PROFILE_BALANCED:
+ sel_prof = SMARTFAN_MODE_BALANCED;
+ break;
+ case PLATFORM_PROFILE_PERFORMANCE:
+ sel_prof = SMARTFAN_MODE_PERFORMANCE;
+ break;
+ case PLATFORM_PROFILE_CUSTOM:
+ sel_prof = SMARTFAN_MODE_CUSTOM;
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ priv = container_of(pprof, struct lenovo_wmi_gz_priv, pprof);
Please assign priv during declaration.
Ack
+
+ guard(mutex)(&call_mutex);
+ err = lenovo_wmidev_evaluate_method_1(
+ priv->wdev, 0x0, WMI_METHOD_ID_SMARTFAN_SET, sel_prof, NULL);
+ if (err) {
+ pr_err("Error setting fan profile on WMI interface: %u\n", err);
Again, this error message only states the obvious, please remove it.
+ return err;
+ }
+
+ priv->current_profile = profile;
+ return 0;
+}
+
+/* Driver Setup */
+static int platform_profile_setup(struct lenovo_wmi_gz_priv *priv)
+{
+ int supported;
+ int err;
+
+ err = lenovo_wmi_gamezone_platform_profile_supported(&priv->pprof,
+ &supported);
+ if (err) {
+ pr_err("Error checking platform profile support: %d\n", err);
+ return err;
Please use dev_err() here.
Ack
+ }
+
+ priv->platform_profile_support = supported;
The value of platform_profile_support is not used anywhere, please remove it.
+
+ if (!supported)
+ return -EOPNOTSUPP;
IMHO returning -ENODEV would make more sense here.
Ack
+
+ priv->pprof.name = "lenovo-wmi-gamezone";
+ priv->pprof.profile_get = lenovo_wmi_gamezone_profile_get;
+ priv->pprof.profile_set = lenovo_wmi_gamezone_profile_set;
+
+ set_bit(PLATFORM_PROFILE_QUIET, priv->pprof.choices);
+ set_bit(PLATFORM_PROFILE_BALANCED, priv->pprof.choices);
+ set_bit(PLATFORM_PROFILE_PERFORMANCE, priv->pprof.choices);
+ set_bit(PLATFORM_PROFILE_CUSTOM, priv->pprof.choices);
+
+ err = lenovo_wmi_gamezone_profile_get(&priv->pprof,
+ &priv->current_profile);
+ if (err) {
+ pr_err("Error getting current platform profile: %d\n", err);
+ return err;
+ }
Is there a technical reason for reading the current platform profile during device
initialization(? If no then please remove this call.
There isn't, just a misconception on my part. WIll remove.
+
+ guard(mutex)(&call_mutex);
+ err = platform_profile_register(&priv->pprof);
Using devm_platform_profile_register() would make sense here. This function was added very recently
so you have to base your patch series onto the for-next branch.
Ack
+ if (err) {
+ pr_err("Error registering platform profile: %d\n", err);
+ return err;
+ }
+
+ return 0;
+}
+
+static int lenovo_wmi_gamezone_probe(struct wmi_device *wdev,
+ const void *context)
+{
+ struct lenovo_wmi_gz_priv *priv;
+
+ priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->wdev = wdev;
Since you are using dev_get_drvdata(), you also need to use dev_set_drvdata() here, otherwise
dev_get_drvdata() will return no valid value.
Ack
+ return platform_profile_setup(priv);
+}
+
+static void lenovo_wmi_gamezone_remove(struct wmi_device *wdev)
+{
+ struct lenovo_wmi_gz_priv *priv = dev_get_drvdata(&wdev->dev);
+
+ guard(mutex)(&call_mutex);
+ platform_profile_remove(&priv->pprof);
+}
+
+static struct wmi_driver lenovo_wmi_gamezone_driver = {
+ .driver = { .name = "lenovo_wmi_gamezone" },
Please set ".probe_type = PROBE_PREFER_ASYNCHRONOUS" here.
Ack
Also does the selected fan profile remain the same after suspending or hibernating?
If no then please add the necessary PM callbacks to save/restore the fan profile
before suspend/after resume if necessary.
It remains the same after suspend, hibernate, reboot, and shutdown.
Nice, so no special pm handling is necessary here :).
Thanks,
Armin Wolf
+ .id_table = lenovo_wmi_gamezone_id_table,
+ .probe = lenovo_wmi_gamezone_probe,
+ .remove = lenovo_wmi_gamezone_remove,
Please set ".no_singleton = true" here.
Ack
+};
+
+module_wmi_driver(lenovo_wmi_gamezone_driver);
+
+MODULE_DEVICE_TABLE(wmi, lenovo_wmi_gamezone_id_table);
+MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@xxxxxxxxx>");
+MODULE_DESCRIPTION("Lenovo GameZone WMI Driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/lenovo-wmi.h b/drivers/platform/x86/lenovo-wmi.h
new file mode 100644
index 000000000000..8a302c6c47cb
--- /dev/null
+++ b/drivers/platform/x86/lenovo-wmi.h
@@ -0,0 +1,105 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * Lenovo Legion WMI interface driver. The Lenovo Legion WMI interface is
+ * broken up into multiple GUID interfaces that require cross-references
+ * between GUID's for some functionality. The "Custom Mode" interface is a
+ * legacy interface for managing and displaying CPU & GPU power and hwmon
+ * settings and readings. The "Other Mode" interface is a modern interface
+ * that replaces or extends the "Custom Mode" interface methods. The
+ * "GameZone" interface adds advanced features such as fan profiles and
+ * overclocking. The "Lighting" interface adds control of various status
+ * lights related to different hardware components. "Other Method" uses
+ * the data structs LENOVO_CAPABILITY_DATA_00, LENOVO_CAPABILITY_DATA_01
+ * and LENOVO_CAPABILITY_DATA_02 structs for capability information.
+ *
+ * Copyright(C) 2024 Derek J. Clark <derekjohn.clark@xxxxxxxxx>
+ *
+ */
+
+#define pr_fmt(fmt) "%s:%s: " fmt, KBUILD_MODNAME, __func__
+
+#ifndef _LENOVO_WMI_H_
+#define _LENOVO_WMI_H_
+
+#include <linux/mutex.h>
+#include <linux/types.h>
+#include <linux/wmi.h>
+
+/* Platform Profile Modes */
+#define SMARTFAN_MODE_QUIET 0x01
+#define SMARTFAN_MODE_BALANCED 0x02
+#define SMARTFAN_MODE_PERFORMANCE 0x03
+#define SMARTFAN_MODE_CUSTOM 0xFF
+
+struct wmi_method_args {
+ u32 arg0;
+ u32 arg1;
+};
+
+/* General Use functions */
+static int lenovo_wmidev_evaluate_method(struct wmi_device *wdev, u8 instance,
+ u32 method_id, struct acpi_buffer *in,
+ struct acpi_buffer *out)
+{
+ acpi_status status;
+
+ status = wmidev_evaluate_method(wdev, instance, method_id, in, out);
+
+ if (ACPI_FAILURE(status))
+ return -EIO;
+
+ return 0;
+};
+
+int lenovo_wmidev_evaluate_method_2(struct wmi_device *wdev, u8 instance,
+ u32 method_id, u32 arg0, u32 arg1,
+ u32 *retval);
+
+int lenovo_wmidev_evaluate_method_2(struct wmi_device *wdev, u8 instance,
+ u32 method_id, u32 arg0, u32 arg1,
+ u32 *retval)
+{
+ struct wmi_method_args args = { arg0, arg1 };
+ struct acpi_buffer input = { (acpi_size)sizeof(args), &args };
+ struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
+ union acpi_object *ret_obj = NULL;
+ int err;
Please order the variable declarations in reverse XMAS tree order.
Ack
+
+ err = lenovo_wmidev_evaluate_method(wdev, instance, method_id, &input,
+ &output);
+
+ if (err) {
+ pr_err("Attempt to get method value failed.\n");
Please remove any error messages in this part of the code, printing error messages should
ideally happen at the higher layers of the driver if necessary.
+ return err;
+ }
+
+ if (retval) {
+ ret_obj = (union acpi_object *)output.pointer;
+ if (!ret_obj) {
+ pr_err("Failed to get valid ACPI object from WMI interface\n");
+ return -EIO;
-ENODATA.
Ack
+ }
+ if (ret_obj->type != ACPI_TYPE_INTEGER) {
+ pr_err("WMI query returnd ACPI object with wrong type.\n");
+ kfree(ret_obj);
+ return -EIO;
-ENXIO.
Ack
+ }
+ *retval = (u32)ret_obj->integer.value;
+ }
+
+ kfree(ret_obj);
+
+ return 0;
+}
+
+int lenovo_wmidev_evaluate_method_1(struct wmi_device *wdev, u8 instance,
+ u32 method_id, u32 arg0, u32 *retval);
+
+int lenovo_wmidev_evaluate_method_1(struct wmi_device *wdev, u8 instance,
+ u32 method_id, u32 arg0, u32 *retval)
+{
+ return lenovo_wmidev_evaluate_method_2(wdev, instance, method_id, arg0,
+ 0, retval);
+}
+
+#endif /* !_LENOVO_WMI_H_ */
Thanks Armin,
Derek