On Fri, Jan 10, 2025 at 1:33 PM Derek John Clark <derekjohn.clark@xxxxxxxxx> wrote: > > 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. > > > > + > > > 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. > > > > + .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 > Armin, After looking at this again, args is used by output so it needs to be before it. I'll swap input and output unless you have another suggestion. Cheers, Derek > > > + > > > + 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
