Re: [PATCH v3 2/4] platform/x86: Add Lenovo Gamezone WMI Driver

Armin Wolf
2025-03-08T00:42:00Z

Am 07.03.25 um 23:47 schrieb Armin Wolf:

> Am 25.02.25 um 22:59 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.
>>
>> Adds lenovo-wmi.h and lenovo-wmi.c which provide helper functions for
>> wmidev_evaluate_method as well as prototypes for exported functions.
>> v3:
>> - Use notifier chain to report platform profile changes to any
>>    subscribed drivers.
>> - Adds THERMAL_MODE_EVENT GUID and .notify function to trigger notifier
>>    chain.
>> - Adds support for Extreme Mode profile on supported hardware, as well
>>    as a DMI quirk table for some devices that report extreme mode
>> version
>>    support but so not have it fully implemented.
>> - Update to include recent changes to platform-profile.
>> v2:
>> - Use devm_kmalloc 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@gmail.com>
>> ---
>>   MAINTAINERS                                |   3 +
>>   drivers/platform/x86/Kconfig               |  16 +
>>   drivers/platform/x86/Makefile              |   2 +
>>   drivers/platform/x86/lenovo-wmi-gamezone.c | 374 +++++++++++++++++++++
>>   drivers/platform/x86/lenovo-wmi.c          |  77 +++++
>>   drivers/platform/x86/lenovo-wmi.h          |  62 ++++
>>   6 files changed, 534 insertions(+)
>>   create mode 100644 drivers/platform/x86/lenovo-wmi-gamezone.c
>>   create mode 100644 drivers/platform/x86/lenovo-wmi.c
>>   create mode 100644 drivers/platform/x86/lenovo-wmi.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index e20c32b3c480..cf7f4fce1a25 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -13157,6 +13157,9 @@ L: platform-driver-x86@vger.kernel.org
>>   S:    Maintained
>>   F:    Documentation/wmi/devices/lenovo-wmi-gamezone.rst
>>   F:    Documentation/wmi/devices/lenovo-wmi-other.rst
>> +F:    drivers/platform/x86/lenovo-wmi-gamezone.c
>> +F:    drivers/platform/x86/lenovo-wmi.c
>> +F:    drivers/platform/x86/lenovo-wmi.h
>>     LETSKETCH HID TABLET DRIVER
>>   M:    Hans de Goede <hdegoede@redhat.com>
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index 7e20a58861eb..875822e6bd65 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -459,6 +459,22 @@ config IBM_RTL
>>        state = 0 (BIOS SMIs on)
>>        state = 1 (BIOS SMIs off)
>>   +config LENOVO_WMI
>> +    tristate
>> +    depends on ACPI_WMI
>
> Please rename this module to LENOVO_WMI_HELPERS.
>
>> +
>> +config LENOVO_WMI_GAMEZONE
>> +    tristate "Lenovo GameZone WMI Driver"
>> +    depends on ACPI_WMI
>> +    select ACPI_PLATFORM_PROFILE
>> +    select LENOVO_WMI
>> +    help
>> +      Say Y here if you have a WMI aware Lenovo Legion device and
>> would like to use the
>> +      platform-profile firmware interface to manage power usage.
>> +
>> +      To compile this driver as a module, choose M here: the module
>> will
>> +      be called lenovo-wmi-gamezone.
>> +
>>   config IDEAPAD_LAPTOP
>>       tristate "Lenovo IdeaPad Laptop Extras"
>>       depends on ACPI
>> diff --git a/drivers/platform/x86/Makefile
>> b/drivers/platform/x86/Makefile
>> index 5f6307246e69..4a7b2d14eb82 100644
>> --- a/drivers/platform/x86/Makefile
>> +++ b/drivers/platform/x86/Makefile
>> @@ -67,7 +67,9 @@ obj-$(CONFIG_THINKPAD_ACPI)    += thinkpad_acpi.o
>>   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)    += lenovo-wmi.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..d5329fecba47
>> --- /dev/null
>> +++ b/drivers/platform/x86/lenovo-wmi-gamezone.c
>> @@ -0,0 +1,374 @@
>> +// 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@gmail.com>
>> + */
>> +
>> +#include "linux/container_of.h"
>> +#include "linux/printk.h"
>> +#include <linux/cleanup.h>
>> +#include <linux/dev_printk.h>
>> +#include <linux/dmi.h>
>> +#include <linux/list.h>
>> +#include <linux/notifier.h>
>> +#include <linux/platform_profile.h>
>> +#include <linux/types.h>
>> +#include <linux/wmi.h>
>> +#include "lenovo-wmi.h"
>> +
>> +/* Interface GUIDs */
>> +#define LENOVO_GAMEZONE_GUID "887B54E3-DDDC-4B2C-8B88-68A26A8835D0"
>> +#define THERMAL_MODE_EVENT_GUID "D320289E-8FEA-41E0-86F9-911D83151B5F"
>> +
>> +/* 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 */
>> +
>> +enum lenovo_wmi_gz_type {
>> +    GAMEZONE_FULL = 1,
>> +    THERMAL_MODE,
>> +};
>> +
>> +#define GAMEZONE_WMI_DEVICE(guid, type)                              \
>> +    .guid_string = (guid), .context = &(enum lenovo_wmi_gz_type) \
>> + {                                                            \
>> +        type                                                 \
>> +    }
>> +
>> +static BLOCKING_NOTIFIER_HEAD(gz_chain_head);
>> +static DEFINE_MUTEX(gz_chain_mutex);
>> +
>> +struct lenovo_wmi_gz_priv {
>> +    enum platform_profile_option current_profile;
>
> This variable is only assigned and never read, please remove it.
>
>> +    struct wmi_device *wdev;
>> +    bool extreme_supported;
>> +    struct device *ppdev; /*platform profile device */
>> +    enum lenovo_wmi_gz_type type;
>> +    struct blocking_notifier_head nhead;
>> +};
>> +
>> +struct quirk_entry {
>> +    bool extreme_supported;
>> +};
>> +
>> +static struct quirk_entry quirk_no_extreme_bug = {
>> +    .extreme_supported = false,
>> +};
>
> Can you make this const?
>
>> +
>> +/* Platform Profile Methods & Setup */
>> +static int
>> +lenovo_wmi_gz_platform_profile_supported(struct lenovo_wmi_gz_priv
>> *priv,
>> +                     int *supported)
>> +{
>> +    return lenovo_wmidev_evaluate_method_1(priv->wdev, 0x0,
>> +                           WMI_METHOD_ID_SMARTFAN_SUPP, 0, supported);
>> +}
>> +
>> +static int lenovo_wmi_gz_profile_get(struct device *dev,
>> +                     enum platform_profile_option *profile)
>> +{
>> +    struct lenovo_wmi_gz_priv *priv = dev_get_drvdata(dev);
>> +    int sel_prof;
>> +    int ret;
>> +
>> +    ret = lenovo_wmidev_evaluate_method_1(priv->wdev, 0x0,
>> +                          WMI_METHOD_ID_SMARTFAN_GET, 0, &sel_prof);
>> +    if (ret)
>> +        return ret;
>> +
>> +    switch (sel_prof) {
>> +    case SMARTFAN_MODE_QUIET:
>> +        *profile = PLATFORM_PROFILE_LOW_POWER;
>> +        break;
>> +    case SMARTFAN_MODE_BALANCED:
>> +        *profile = PLATFORM_PROFILE_BALANCED;
>> +        break;
>> +    case SMARTFAN_MODE_PERFORMANCE:
>> +        if (priv->extreme_supported) {
>> +            *profile = PLATFORM_PROFILE_BALANCED_PERFORMANCE;
>> +            break;
>> +        }
>> +        *profile = PLATFORM_PROFILE_PERFORMANCE;
>> +        break;
>> +    case SMARTFAN_MODE_EXTREME:
>> +        *profile = PLATFORM_PROFILE_PERFORMANCE;
>> +        break;
>> +    case SMARTFAN_MODE_CUSTOM:
>> +        *profile = PLATFORM_PROFILE_CUSTOM;
>> +        break;
>> +    default:
>> +        return -EINVAL;
>> +    }
>> +
>> +    priv->current_profile = *profile;
>> +
>> +    ret = blocking_notifier_call_chain(&gz_chain_head,
>> THERMAL_MODE_EVENT,
>> +                       &sel_prof);
>
> Is it really necessary to call the notifier here? AFAIK the notifier
> needs to be called
> only:
>
>  - when the platform profile was changed either by the user or the
> firmware.
>  - when a new notifier handler was registered so that the handler does
> not have to wait for the next user input
>
> Please only call the notifier in those two situations.

I thought about this a bit and i came to the conclusion that my proposal would not work. The problem is that if no
gamezone driver is available the notifier handler will not be called at all.

I instead suggest that you move the notifier inside the lenovo-wmi-other driver. Then the gamezone driver registers itself
with this notifier and supplies the current platform profile if the lenovo-wmi-other driver requests it. In this case
it would make sense to cache the current platform profile.

Thanks,
Armin Wolf

>
>> +    if (ret == NOTIFY_BAD)
>> +        pr_err("Failed to send notification to call chain for WMI
>> event %u\n",
>> +               priv->type);
>
> Use dev_err() here please.
>
>> +    return 0;
>> +}
>> +
>> +static int lenovo_wmi_gz_profile_set(struct device *dev,
>> +                     enum platform_profile_option profile)
>> +{
>> +    struct lenovo_wmi_gz_priv *priv = dev_get_drvdata(dev);
>> +    int sel_prof;
>> +    int ret;
>> +
>> +    switch (profile) {
>> +    case PLATFORM_PROFILE_LOW_POWER:
>> +        sel_prof = SMARTFAN_MODE_QUIET;
>> +        break;
>> +    case PLATFORM_PROFILE_BALANCED:
>> +        sel_prof = SMARTFAN_MODE_BALANCED;
>> +        break;
>> +    case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
>> +        sel_prof = SMARTFAN_MODE_PERFORMANCE;
>> +        break;
>> +    case PLATFORM_PROFILE_PERFORMANCE:
>> +        if (priv->extreme_supported) {
>> +            sel_prof = SMARTFAN_MODE_EXTREME;
>> +            break;
>> +        }
>> +        sel_prof = SMARTFAN_MODE_PERFORMANCE;
>> +        break;
>> +    case PLATFORM_PROFILE_CUSTOM:
>> +        sel_prof = SMARTFAN_MODE_CUSTOM;
>> +        break;
>> +    default:
>> +        return -EOPNOTSUPP;
>> +    }
>> +
>> +    ret = lenovo_wmidev_evaluate_method_1(priv->wdev, 0x0,
>> +                          WMI_METHOD_ID_SMARTFAN_SET, sel_prof, NULL);
>> +    if (ret)
>> +        return ret;
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct dmi_system_id fwbug_list[] = {
>> +    {
>> +        .ident = "Legion Go 8APU1",
>> +        .matches = {
>> +            DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>> +            DMI_MATCH(DMI_PRODUCT_VERSION, "Legion Go 8APU1"),
>> +        },
>> +        .driver_data = &quirk_no_extreme_bug,
>> +    },
>> +    {
>> +        .ident = "Legion Go S 8ARP1",
>> +        .matches = {
>> +            DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>> +            DMI_MATCH(DMI_PRODUCT_VERSION, "Legion Go S 8ARP1"),
>> +        },
>> +        .driver_data = &quirk_no_extreme_bug,
>> +    },
>> +    {
>> +        .ident = "Legion Go S 8APU1",
>> +        .matches = {
>> +            DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>> +            DMI_MATCH(DMI_PRODUCT_VERSION, "Legion Go S 8APU1"),
>> +        },
>> +        .driver_data = &quirk_no_extreme_bug,
>> +    },
>> +    {},
>> +
>> +};
>> +
>> +static bool extreme_supported(int profile_support_ver)
>> +{
>> +    const struct dmi_system_id *dmi_id;
>> +    struct quirk_entry *quirks;
>> +
>> +    if (profile_support_ver < 6)
>> +        return false;
>> +
>> +    dmi_id = dmi_first_match(fwbug_list);
>> +    if (!dmi_id)
>> +        return true;
>> +
>> +    quirks = dmi_id->driver_data;
>> +    return quirks->extreme_supported;
>> +}
>> +
>> +static int lenovo_wmi_platform_profile_probe(void *drvdata,
>> +                         unsigned long *choices)
>> +{
>> +    struct lenovo_wmi_gz_priv *priv = drvdata;
>> +    enum platform_profile_option profile;
>
> Unused variable, please remove.
>
>> +    int profile_support_ver;
>> +    int ret;
>> +
>> +    ret = lenovo_wmi_gz_platform_profile_supported(priv,
>> +                               &profile_support_ver);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (profile_support_ver < 1)
>> +        return -ENODEV;
>> +
>> +    priv->extreme_supported = extreme_supported(profile_support_ver);
>> +
>> +    set_bit(PLATFORM_PROFILE_LOW_POWER, choices);
>> +    set_bit(PLATFORM_PROFILE_BALANCED, choices);
>> +    set_bit(PLATFORM_PROFILE_PERFORMANCE, choices);
>> +    set_bit(PLATFORM_PROFILE_CUSTOM, choices);
>> +
>> +    if (priv->extreme_supported)
>> +        set_bit(PLATFORM_PROFILE_BALANCED_PERFORMANCE, choices);
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct platform_profile_ops
>> lenovo_wmi_gz_platform_profile_ops = {
>> +    .probe = lenovo_wmi_platform_profile_probe,
>> +    .profile_get = lenovo_wmi_gz_profile_get,
>> +    .profile_set = lenovo_wmi_gz_profile_set,
>> +};
>> +
>> +/* Notifier Methods */
>> +int lenovo_wmi_gz_register_notifier(struct notifier_block *nb)
>> +{
>> +    guard(mutex)(&gz_chain_mutex);
>
> The blocking notifier already does the locking itself. Please remove
> this mutex.
>
>> +    return blocking_notifier_chain_register(&gz_chain_head, nb);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(lenovo_wmi_gz_register_notifier, "GZ_WMI");
>
> Can you name the namespace similar to "LENOVO_GAMEZONE_WMI" please?
>
>> +
>> +int lenovo_wmi_gz_unregister_notifier(struct notifier_block *nb)
>> +{
>> +    guard(mutex)(&gz_chain_mutex);
>> +    return blocking_notifier_chain_unregister(&gz_chain_head, nb);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(lenovo_wmi_gz_unregister_notifier, "GZ_WMI");
>> +
>> +static void devm_lenovo_wmi_gz_unregister_notifier(void *data)
>> +{
>> +    struct notifier_block *nb = data;
>> +
>> +    lenovo_wmi_gz_unregister_notifier(nb);
>> +}
>> +
>> +int devm_lenovo_wmi_gz_register_notifier(struct device *dev,
>> +                     struct notifier_block *nb)
>> +{
>> +    int ret;
>> +
>> +    ret = lenovo_wmi_gz_register_notifier(nb);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    return devm_add_action_or_reset(dev,
>> devm_lenovo_wmi_gz_unregister_notifier, nb);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(devm_lenovo_wmi_gz_register_notifier, "GZ_WMI");
>> +
>> +/* Driver Methods */
>> +static void lenovo_wmi_gz_notify(struct wmi_device *wdev,
>> +                 union acpi_object *obj)
>> +{
>> +    struct lenovo_wmi_gz_priv *tm_priv = dev_get_drvdata(&wdev->dev);
>> +    struct lenovo_wmi_gz_priv *gz_priv =
>> +        container_of(&gz_chain_head, struct lenovo_wmi_gz_priv, nhead);
>
> I fear that this will not work because gz_chain_head is a global
> variable, not a field inside
> struct lenovo_wmi_gz_priv. Also this would crash the kernel should the
> main gamezone driver be
> unbound from its WMI device.
>
> I suggest you move the WMI driver for the WMI event into a separate
> module. Then you use another notifier
> inside the new module to allow the gamezone driver to listen for
> events. For example this separate WMI event driver
> could use the "val" argument inside blocking_notifier_call_chain() to
> specify the type of event (like THERMAL_MODE_CHANGED)
> and the "v" argument to pass a pointer to a u32 variable containing
> the new thermal mode.
>
> This also allows you to extend the separate WMI driver later to
> support more WMI event GUIDs.
>
>> +    int sel_prof;
>> +    int ret;
>> +
>> +    if (obj->type != ACPI_TYPE_INTEGER)
>> +        return;
>> +
>> +    switch (tm_priv->type) {
>> +    case THERMAL_MODE:
>> +        sel_prof = obj->integer.value;
>> +        break;
>> +    default:
>> +        return;
>> +    }
>> +
>> +    /* Update primary Gamezone instance */
>> +    switch (sel_prof) {
>> +    case SMARTFAN_MODE_QUIET:
>> +        gz_priv->current_profile = PLATFORM_PROFILE_LOW_POWER;
>> +        break;
>> +    case SMARTFAN_MODE_BALANCED:
>> +        gz_priv->current_profile = PLATFORM_PROFILE_BALANCED;
>> +        break;
>> +    case SMARTFAN_MODE_PERFORMANCE:
>> +        if (gz_priv->extreme_supported) {
>> +            gz_priv->current_profile =
>> +                PLATFORM_PROFILE_BALANCED_PERFORMANCE;
>> +            break;
>> +        }
>> +        gz_priv->current_profile = PLATFORM_PROFILE_PERFORMANCE;
>> +        break;
>> +    case SMARTFAN_MODE_EXTREME:
>> +        gz_priv->current_profile = PLATFORM_PROFILE_PERFORMANCE;
>> +        break;
>> +    case SMARTFAN_MODE_CUSTOM:
>> +        gz_priv->current_profile = PLATFORM_PROFILE_CUSTOM;
>> +        break;
>> +    default:
>> +        break;
>> +    }
>
> Please use platform_profile_notify() to notify userspace of the new
> platform profile settings.
>
>> +
>> +    ret = blocking_notifier_call_chain(&gz_chain_head,
>> THERMAL_MODE_EVENT,
>> +                       &sel_prof);
>> +    if (ret == NOTIFY_BAD)
>> +        pr_err("Failed to send notification to call chain for WMI
>> event %u\n",
>> +               tm_priv->type);
>> +}
>> +
>> +static int lenovo_wmi_gz_probe(struct wmi_device *wdev, const void
>> *context)
>> +{
>> +    struct lenovo_wmi_gz_priv *priv =
>> +        devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
>
> Please do the call to devm_kzalloc() on a separate line:
>
>     struct lenovo_wmi_gz_priv *priv;
>
>     priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
>
>> +
>> +    if (!priv)
>> +        return -ENOMEM;
>> +
>> +    if (!context)
>> +        return -EINVAL;
>> +
>> +    priv->wdev = wdev;
>> +    priv->type = *((enum lenovo_wmi_gz_type *)context);
>> +
>> +    dev_set_drvdata(&wdev->dev, priv);
>> +
>> +    if (priv->type != GAMEZONE_FULL)
>> +        return 0;
>> +
>> +    priv->nhead = gz_chain_head;
>> +    priv->ppdev = platform_profile_register(&wdev->dev,
>> "lenovo-wmi-gamezone",
>> +                        priv, &lenovo_wmi_gz_platform_profile_ops);
>
> Please check if platform_profile_register() was successful and return
> an error if not.
>
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct wmi_device_id lenovo_wmi_gz_id_table[] = {
>> +    { GAMEZONE_WMI_DEVICE(LENOVO_GAMEZONE_GUID, GAMEZONE_FULL) },
>> +    { GAMEZONE_WMI_DEVICE(THERMAL_MODE_EVENT_GUID, THERMAL_MODE) },
>> +    {}
>> +};
>> +
>> +static struct wmi_driver lenovo_wmi_gz_driver = {
>> +    .driver = {
>> +        .name = "lenovo_wmi_gamezone",
>> +        .probe_type = PROBE_PREFER_ASYNCHRONOUS,
>> +    },
>> +    .id_table = lenovo_wmi_gz_id_table,
>> +    .probe = lenovo_wmi_gz_probe,
>> +    .notify = lenovo_wmi_gz_notify,
>> +    .no_singleton = true,
>> +};
>> +
>> +module_wmi_driver(lenovo_wmi_gz_driver);
>> +
>> +MODULE_IMPORT_NS("LENOVO_WMI");
>> +MODULE_DEVICE_TABLE(wmi, lenovo_wmi_gz_id_table);
>> +MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@gmail.com>");
>> +MODULE_DESCRIPTION("Lenovo GameZone WMI Driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/platform/x86/lenovo-wmi.c
>> b/drivers/platform/x86/lenovo-wmi.c
>> new file mode 100644
>> index 000000000000..0de2c37e69bd
>> --- /dev/null
>> +++ b/drivers/platform/x86/lenovo-wmi.c
>> @@ -0,0 +1,77 @@
>> +// 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 Method"
>> 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 Method" 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 Mode" 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@gmail.com>
>> + *
>> + */
>> +
>> +#include <linux/wmi.h>
>> +#include "lenovo-wmi.h"
>> +
>> +/* wmidev_evaluate_method helper 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)
>> +{
>
> Please give this method a more descriptive name.
>
>> +    struct wmi_method_args args = { arg0, arg1 };
>> +    struct acpi_buffer input = { (acpi_size)sizeof(args), &args };
>
> Cast to acpi_size is unnecessary here.
>
>> +    struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
>> +    union acpi_object *ret_obj __free(kfree) = NULL;
>> +    int err;
>> +
>> +    err = lenovo_wmidev_evaluate_method(wdev, instance, method_id,
>> &input,
>> +                        &output);
>> +
>> +    if (err)
>> +        return err;
>> +
>> +    if (retval) {
>> +        ret_obj = (union acpi_object *)output.pointer;
>> +        if (!ret_obj)
>> +            return -ENODATA;
>> +
>> +        if (ret_obj->type != ACPI_TYPE_INTEGER)
>> +            return -ENXIO;
>> +
>> +        *retval = (u32)ret_obj->integer.value;
>> +    }
>> +
>> +    return 0;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(lenovo_wmidev_evaluate_method_2, "LENOVO_WMI");
>
> Can you please rename the namespace to "LENOVO_WMI_HELPERS"?
>
>> +
>> +int lenovo_wmidev_evaluate_method_1(struct wmi_device *wdev, u8
>> instance,
>> +                    u32 method_id, u32 arg0, u32 *retval)
>> +{
>
> Same as above.
>
>> +    return lenovo_wmidev_evaluate_method_2(wdev, instance,
>> method_id, arg0,
>> +                           0, retval);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(lenovo_wmidev_evaluate_method_1, "LENOVO_WMI");
>> +MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@gmail.com>");
>> +MODULE_DESCRIPTION("Lenovo WMI Common 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..113928b4fc0f
>> --- /dev/null
>> +++ b/drivers/platform/x86/lenovo-wmi.h
>> @@ -0,0 +1,62 @@
>> +/* 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 Method"
>> 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 Method" 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 Mode" 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@gmail.com>
>> + *
>> + */
>> +#include <linux/notifier.h>
>> +#include <linux/platform_profile.h>
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> This causes a build error on my machine. Please only use this macro
> inside source files
> for modules and not inside header files.
>
>> +
>> +#ifndef _LENOVO_WMI_H_
>> +#define _LENOVO_WMI_H_
>> +
>> +#include <linux/bits.h>
>> +#include <linux/types.h>
>> +#include <linux/wmi.h>
>> +
>> +struct wmi_method_args {
>> +    u32 arg0;
>> +    u32 arg1;
>> +};
>> +
>> +/* gamezone structs */
>> +enum thermal_mode {
>> +    SMARTFAN_MODE_QUIET = 0x01,
>> +    SMARTFAN_MODE_BALANCED = 0x02,
>> +    SMARTFAN_MODE_PERFORMANCE = 0x03,
>> +    SMARTFAN_MODE_EXTREME = 0xE0, /* Ver 6+ */
>> +    SMARTFAN_MODE_CUSTOM = 0xFF,
>> +};
>> +
>> +enum lenovo_wmi_action {
>> +    THERMAL_MODE_EVENT = 1,
>> +};
>> +
>> +/* wmidev_evaluate_method helper functions */
>> +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_1(struct wmi_device *wdev, u8
>> instance,
>> +                    u32 method_id, u32 arg0, u32 *retval);
>> +
>> +/* lenovo_wmi_gz_driver notifier functions */
>> +int lenovo_wmi_gz_notifier_call(struct notifier_block *nb, unsigned
>> long action,
>> +                enum platform_profile_option *profile);
>> +int lenovo_wmi_gz_register_notifier(struct notifier_block *nb);
>> +int lenovo_wmi_gz_unregister_notifier(struct notifier_block *nb);
>> +int devm_lenovo_wmi_gz_register_notifier(struct device *dev,
>> +                     struct notifier_block *nb);
>
> Can you please create a separate header file for each driver?
> Otherwise this header file
> will contain many different things from different drivers, which will
> maybe not even be
> available depending on the Kconfig settings.
>
> Thanks,
> Armin Wolf
>
>> +#endif /* !_LENOVO_WMI_H_ */