Re: [PATCH v5 6/6] platform/x86: Add Lenovo Other Mode WMI Driver

[Derek J. Clark]

On Fri, Apr 11, 2025, 07:54 Derek John Clark <derekjohn.clark@xxxxxxxxx> 
wrote:

> On Tue, Apr 8, 2025 at 5:50 AM Ilpo Järvinen
> <ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
> >
> > On Mon, 7 Apr 2025, Derek J. Clark wrote:
> >
> > > Adds lenovo-wmi-other driver which provides the Lenovo "Other Mode" WMI
> > > interface that comes on some Lenovo "Gaming Series" hardware. Provides 
> a
> > > firmware-attributes class which enables the use of tunable knobs for 
> SPL,
> > > SPPT, and FPPT.
> > >
> > > Signed-off-by: Derek J. Clark <derekjohn.clark@xxxxxxxxx>
> > > ---
> > > v5:
> > > - Switch from locally storing capability data list to storing a pointer
> > >   from the capability data interface.
> > > - Add portion of Gamezone driver that relies on the exported functions
> > >   of this driver.
> > > - Properly initialize IDA and use it for naming the firmware-attributes
> > >   path.
> > > - Rename most defines to clearly indicate they are from this driver.
> > > - Misc fixes from v4 review.
> > > v4:
> > > - Treat Other Mode as a notifier chain head, use the notifier chain to
> > >   get the current mode from Gamezone.
> > > - Add header file for Other Mode specific structs and finctions.
> > > - Use component master bind to cache the capdata01 array locally.
> > > - Drop all reference to external driver private data structs.
> > > - Various fixes from review.
> > > v3:
> > > - Add notifier block and store result for getting the Gamezone 
> interface
> > >   profile changes.
> > > - Add driver as master component of capdata01 driver.
> > > - Use FIELD_PREP where appropriate.
> > > - Move macros and associated functions out of lemovo-wmi.h that are 
> only
> > >   used by this driver.
> > > 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 other_method_wmi to lenovo_wmi_om_priv and om_wmi to priv.
> > > - Use list to get the lenovo_wmi_om_priv instance in some macro
> > >   called functions as the data provided by the macros that use it
> > >   doesn't pass a member of the struct for use in container_of.
> > > - Do not rely on GameZone interface to grab the current fan mode.
> > > ---
> > >  MAINTAINERS                                |   2 +
> > >  drivers/platform/x86/Kconfig               |  15 +
> > >  drivers/platform/x86/Makefile              |   1 +
> > >  drivers/platform/x86/lenovo-wmi-gamezone.c |  35 ++
> > >  drivers/platform/x86/lenovo-wmi-other.c    | 677 +++++++++++++++++++++
> > >  drivers/platform/x86/lenovo-wmi-other.h    |  16 +
> > >  6 files changed, 746 insertions(+)
> > >  create mode 100644 drivers/platform/x86/lenovo-wmi-other.c
> > >  create mode 100644 drivers/platform/x86/lenovo-wmi-other.h
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 49deb8ea4ea7..0416afd997a0 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -13172,6 +13172,8 @@ F:    
> drivers/platform/x86/lenovo-wmi-gamezone.c
> > >  F:   drivers/platform/x86/lenovo-wmi-gamezone.h
> > >  F:   drivers/platform/x86/lenovo-wmi-helpers.c
> > >  F:   drivers/platform/x86/lenovo-wmi-helpers.h
> > > +F:   drivers/platform/x86/lenovo-wmi-other.c
> > > +F:   drivers/platform/x86/lenovo-wmi-other.h
> > >
> > >  LENOVO WMI HOTKEY UTILITIES DRIVER
> > >  M:   Jackie Dong <xy-jackie@xxxxxxx>
> > > diff --git a/drivers/platform/x86/Kconfig 
> b/drivers/platform/x86/Kconfig
> > > index aaa1a69c10ca..be5ab24960b5 100644
> > > --- a/drivers/platform/x86/Kconfig
> > > +++ b/drivers/platform/x86/Kconfig
> > > @@ -485,6 +485,21 @@ config LENOVO_WMI_DATA01
> > >       tristate
> > >       depends on ACPI_WMI
> > >
> > > +config LENOVO_WMI_TUNING
> > > +     tristate "Lenovo Other Mode WMI Driver"
> > > +     depends on ACPI_WMI
> > > +     select FW_ATTR_CLASS
> > > +     select LENOVO_WMI_DATA01
> > > +     select LENOVO_WMI_EVENTS
> > > +     select LENOVO_WMI_HELPERS
> > > +     help
> > > +       Say Y here if you have a WMI aware Lenovo Legion device and 
> would like to use the
> > > +       firmware_attributes API to control various tunable settings 
> typically exposed by
> > > +       Lenovo software in Windows.
> > > +
> > > +       To compile this driver as a module, choose M here: the module 
> will
> > > +       be called lenovo-wmi-other.
> > > +
> > >  config IDEAPAD_LAPTOP
> > >       tristate "Lenovo IdeaPad Laptop Extras"
> > >       depends on ACPI
> > > diff --git a/drivers/platform/x86/Makefile 
> b/drivers/platform/x86/Makefile
> > > index 60058b547de2..f3e64926a96b 100644
> > > --- a/drivers/platform/x86/Makefile
> > > +++ b/drivers/platform/x86/Makefile
> > > @@ -73,6 +73,7 @@ obj-$(CONFIG_LENOVO_WMI_DATA01)     += 
> lenovo-wmi-capdata01.o
> > >  obj-$(CONFIG_LENOVO_WMI_EVENTS)      += lenovo-wmi-events.o
> > >  obj-$(CONFIG_LENOVO_WMI_GAMEZONE)    += lenovo-wmi-gamezone.o
> > >  obj-$(CONFIG_LENOVO_WMI_HELPERS)     += lenovo-wmi-helpers.o
> > > +obj-$(CONFIG_LENOVO_WMI_TUNING)      += lenovo-wmi-other.o
> > >
> > >  # Intel
> > >  obj-y                                += intel/
> > > diff --git a/drivers/platform/x86/lenovo-wmi-gamezone.c 
> b/drivers/platform/x86/lenovo-wmi-gamezone.c
> > > index 2e6d5e7faadf..9c80561f931c 100644
> > > --- a/drivers/platform/x86/lenovo-wmi-gamezone.c
> > > +++ b/drivers/platform/x86/lenovo-wmi-gamezone.c
> > > @@ -22,6 +22,7 @@
> > >  #include "lenovo-wmi-events.h"
> > >  #include "lenovo-wmi-gamezone.h"
> > >  #include "lenovo-wmi-helpers.h"
> > > +#include "lenovo-wmi-other.h"
> > >
> > >  #define LENOVO_GAMEZONE_GUID "887B54E3-DDDC-4B2C-8B88-68A26A8835D0"
> > >
> > > @@ -49,6 +50,34 @@ static struct quirk_entry quirk_no_extreme_bug = {
> > >       .extreme_supported = false,
> > >  };
> > >
> > > +/**
> > > + * lwmi_gz_mode_call() - Call method for lenovo-wmi-other driver 
> notifier.
> > > + *
> > > + * @nb: The notifier_block registered to lenovo-wmi-other driver.
> > > + * @cmd: The event type.
> > > + * @data: Thermal mode enum pointer pointer for returning the thermal 
> mode.
> > > + *
> > > + * For LWMI_GZ_GET_THERMAL_MODE, retrieve the current thermal mode.
> > > + *
> > > + * Return: Notifier_block status.
> > > + */
> > > +static int lwmi_gz_mode_call(struct notifier_block *nb, unsigned long 
> cmd,
> > > +                          void *data)
> > > +{
> > > +     enum thermal_mode **mode = data;
> > > +     struct lwmi_gz_priv *priv;
> > > +
> > > +     priv = container_of(nb, struct lwmi_gz_priv, mode_nb);
> > > +
> > > +     switch (cmd) {
> > > +     case LWMI_GZ_GET_THERMAL_MODE:
> > > +             **mode = priv->current_mode;
> > > +             return NOTIFY_STOP;
> > > +     default:
> > > +             return NOTIFY_DONE;
> > > +     }
> > > +}
> > > +
> > >  /**
> > >   * lwmi_gz_event_call() - Call method for lenovo-wmi-events driver 
> notifier.
> > >   * block call chain.
> > > @@ -347,6 +376,11 @@ static int lwmi_gz_probe(struct wmi_device *wdev, 
> const void *context)
> > >       if (ret)
> > >               return ret;
> > >
> > > +     priv->mode_nb.notifier_call = lwmi_gz_mode_call;
> > > +     ret = devm_lwmi_om_register_notifier(&wdev->dev, &priv->mode_nb);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > >       return 0;
> > >  }
> > >
> > > @@ -369,6 +403,7 @@ module_wmi_driver(lwmi_gz_driver);
> > >
> > >  MODULE_IMPORT_NS("LENOVO_WMI_EVENTS");
> > >  MODULE_IMPORT_NS("LENOVO_WMI_HELPERS");
> > > +MODULE_IMPORT_NS("LENOVO_WMI_OTHER");
> > >  MODULE_DEVICE_TABLE(wmi, lwmi_gz_id_table);
> > >  MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@xxxxxxxxx>");
> > >  MODULE_DESCRIPTION("Lenovo GameZone WMI Driver");
> > > diff --git a/drivers/platform/x86/lenovo-wmi-other.c 
> b/drivers/platform/x86/lenovo-wmi-other.c
> > > new file mode 100644
> > > index 000000000000..342883a90270
> > > --- /dev/null
> > > +++ b/drivers/platform/x86/lenovo-wmi-other.c
> > > @@ -0,0 +1,677 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * Lenovo Other Mode WMI interface driver.
> > > + *
> > > + * This driver uses the fw_attributes class to expose the various WMI 
> functions
> > > + * provided by the "Other Mode" WMI interface. This enables CPU and 
> GPU power
> > > + * limit as well as various other attributes for devices that fall 
> under the
> > > + * "Gaming Series" of Lenovo laptop devices. Each attribute exposed 
> by the
> > > + * "Other Mode"" interface has a corresponding Capability Data struct 
> that
> > > + * allows the driver to probe details about the attribute such as if 
> it is
> > > + * supported by the hardware, the default_value, max_value, 
> min_value, and step
> > > + * increment.
> > > + *
> > > + * These attributes typically don't fit anywhere else in the sysfs 
> and are set
> > > + * in Windows using one of Lenovo's multiple user applications.
> > > + *
> > > + * Copyright(C) 2025 Derek J. Clark <derekjohn.clark@xxxxxxxxx>
> > > + */
> > > +
> > > +#include <linux/acpi.h>
> > > +#include <linux/bitfield.h>
> > > +#include <linux/cleanup.h>
> > > +#include <linux/component.h>
> > > +#include <linux/container_of.h>
> > > +#include <linux/device.h>
> > > +#include <linux/export.h>
> > > +#include <linux/gfp_types.h>
> > > +#include <linux/idr.h>
> > > +#include <linux/kobject.h>
> > > +#include <linux/module.h>
> > > +#include <linux/notifier.h>
> > > +#include <linux/platform_profile.h>
> > > +#include <linux/types.h>
> > > +#include <linux/wmi.h>
> > > +
> > > +#include "lenovo-wmi-capdata01.h"
> > > +#include "lenovo-wmi-events.h"
> > > +#include "lenovo-wmi-gamezone.h"
> > > +#include "lenovo-wmi-helpers.h"
> > > +#include "lenovo-wmi-other.h"
> > > +#include "firmware_attributes_class.h"
> > > +
> > > +#define LENOVO_OTHER_MODE_GUID "DC2A8805-3A8C-41BA-A6F7-092E0089CD3B"
> > > +
> > > +#define LWMI_DEVICE_ID_CPU 0x01
> > > +
> > > +#define LWMI_FEATURE_ID_CPU_SPPT 0x01
> > > +#define LWMI_FEATURE_ID_CPU_SPL 0x02
> > > +#define LWMI_FEATURE_ID_CPU_FPPT 0x03
> >
> > Align values in every define group.
> >
> > > +
> > > +#define LWMI_TYPE_ID_NONE 0x00
> > > +
> > > +#define LWMI_FEATURE_VALUE_GET 17
> > > +#define LWMI_FEATURE_VALUE_SET 18
> > > +
> > > +#define LWMI_ATTR_DEV_ID_MASK GENMASK(31, 24)
> > > +#define LWMI_ATTR_FEAT_ID_MASK GENMASK(23, 16)
> > > +#define LWMI_ATTR_MODE_ID_MASK GENMASK(15, 8)
> > > +#define LWMI_ATTR_TYPE_ID_MASK GENMASK(7, 0)
> > > +
> > > +#define LWMI_OM_FW_ATTR_BASE_PATH "lenovo-wmi-other"
> > > +
> > > +static BLOCKING_NOTIFIER_HEAD(om_chain_head);
> > > +static DEFINE_IDA(lwmi_om_ida);
> > > +
> > > +enum attribute_property {
> > > +     DEFAULT_VAL,
> > > +     MAX_VAL,
> > > +     MIN_VAL,
> > > +     STEP_VAL,
> > > +     SUPPORTED,
> > > +};
> > > +
> > > +struct lwmi_om_priv {
> > > +     struct component_master_ops *ops;
> > > +     /* *cd01_list is only valid after master bind and while 
> capdata01 exists */
> > > +     struct cd01_list *cd01_list;
> > > +     struct device *fw_attr_dev;
> > > +     struct kset *fw_attr_kset;
> > > +     struct notifier_block nb;
> > > +     struct wmi_device *wdev;
> > > +     int ida_id;
> > > +};
> > > +
> > > +struct tunable_attr_01 {
> > > +     struct capdata01 *capdata;
> > > +     struct device *dev;
> > > +     u32 feature_id;
> > > +     u32 device_id;
> > > +     u32 type_id;
> > > +};
> > > +
> > > +struct tunable_attr_01 ppt_pl1_spl = { .device_id = 
> LWMI_DEVICE_ID_CPU,
> > > +                                    .feature_id = 
> LWMI_FEATURE_ID_CPU_SPL,
> > > +                                    .type_id = LWMI_TYPE_ID_NONE };
> > > +struct tunable_attr_01 ppt_pl2_sppt = { .device_id = 
> LWMI_DEVICE_ID_CPU,
> > > +                                     .feature_id = 
> LWMI_FEATURE_ID_CPU_SPPT,
> > > +                                     .type_id = LWMI_TYPE_ID_NONE };
> > > +struct tunable_attr_01 ppt_pl3_fppt = { .device_id = 
> LWMI_DEVICE_ID_CPU,
> > > +                                     .feature_id = 
> LWMI_FEATURE_ID_CPU_FPPT,
> > > +                                     .type_id = LWMI_TYPE_ID_NONE };
> > > +
> > > +struct capdata01_attr_group {
> > > +     const struct attribute_group *attr_group;
> > > +     struct tunable_attr_01 *tunable_attr;
> > > +};
> > > +
> > > +/**
> > > + * lwmi_om_register_notifier() - Add a notifier to the blocking 
> notifier chain
> > > + * @nb: The notifier_block struct to register
> > > + *
> > > + * Call blocking_notifier_chain_register to register the notifier 
> block to the
> > > + * lenovo-wmi-other driver notifer chain.
> > > + *
> > > + * Return: 0 on success, %-EEXIST on error.
> > > + */
> > > +int lwmi_om_register_notifier(struct notifier_block *nb)
> > > +{
> > > +     return blocking_notifier_chain_register(&om_chain_head, nb);
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(lwmi_om_register_notifier, "LENOVO_WMI_OTHER");
> > > +
> > > +/**
> > > + * lwmi_om_unregister_notifier() - Remove a notifier from the 
> blocking notifier
> > > + * chain.
> > > + * @nb: The notifier_block struct to register
> > > + *
> > > + * Call blocking_notifier_chain_unregister to unregister the notifier 
> block from the
> > > + * lenovo-wmi-other driver notifer chain.
> > > + *
> > > + * Return: 0 on success, %-ENOENT on error.
> > > + */
> > > +int lwmi_om_unregister_notifier(struct notifier_block *nb)
> > > +{
> > > +     return blocking_notifier_chain_unregister(&om_chain_head, nb);
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(lwmi_om_unregister_notifier, "LENOVO_WMI_OTHER");
> > > +
> > > +/**
> > > + * devm_lwmi_om_unregister_notifier() - Remove a notifier from the 
> blocking
> > > + * notifier chain.
> > > + * @data: Void pointer to the notifier_block struct to register.
> > > + *
> > > + * Call lwmi_om_unregister_notifier to unregister the notifier block 
> from the
> > > + * lenovo-wmi-other driver notifer chain.
> > > + *
> > > + * Return: 0 on success, %-ENOENT on error.
> > > + */
> > > +static void devm_lwmi_om_unregister_notifier(void *data)
> > > +{
> > > +     struct notifier_block *nb = data;
> > > +
> > > +     lwmi_om_unregister_notifier(nb);
> > > +}
> > > +
> > > +/**
> > > + * devm_lwmi_om_register_notifier() - Add a notifier to the blocking 
> notifier
> > > + * chain.
> > > + * @dev: The parent device of the notifier_block struct.
> > > + * @nb: The notifier_block struct to register
> > > + *
> > > + * Call lwmi_om_register_notifier to register the notifier block to 
> the
> > > + * lenovo-wmi-other driver notifer chain. Then add 
> devm_lwmi_om_unregister_notifier
> > > + * as a device managed ation to automatically unregister the notifier 
> block
> > > + * upon parent device removal.
> > > + *
> > > + * Return: 0 on success, or on error.
> > > + */
> > > +int devm_lwmi_om_register_notifier(struct device *dev,
> > > +                                struct notifier_block *nb)
> > > +{
> > > +     int ret;
> > > +
> > > +     ret = lwmi_om_register_notifier(nb);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     return devm_add_action_or_reset(dev, 
> devm_lwmi_om_unregister_notifier,
> > > +                                     nb);
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(devm_lwmi_om_register_notifier, 
> "LENOVO_WMI_OTHER");
> > > +
> > > +/**
> > > + * lwmi_om_notifier_call() - Call functions for the notifier call 
> chain.
> > > + * @mode: Pointer to a thermal mode enum to retrieve the data from.
> > > + *
> > > + * Call blocking_notifier_call_chain to retrieve the thermal mode 
> from the
> > > + * lenovo-wmi-gamezone driver.
> > > + *
> > > + * Return: 0 on success, or on error.
> > > + */
> > > +static int lwmi_om_notifier_call(enum thermal_mode *mode)
> > > +{
> > > +     int ret;
> > > +
> > > +     ret = blocking_notifier_call_chain(&om_chain_head, 
> LWMI_GZ_GET_THERMAL_MODE, &mode);
> > > +     if ((ret & ~NOTIFY_STOP_MASK) != NOTIFY_OK)
> > > +             return -EINVAL;
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +/* Attribute Methods */
> > > +
> > > +/**
> > > + * int_type_show() - Emit the data type for an integer attribute
> > > + * @kobj: Pointer to the driver object.
> > > + * @kobj_attribute: Pointer to the attribute calling this function.
> > > + * @buf: The buffer to write to.
> > > + *
> > > + * Return: Number of characters written to buf.
> > > + */
> > > +static ssize_t int_type_show(struct kobject *kobj, struct 
> kobj_attribute *kattr,
> > > +                          char *buf)
> > > +{
> > > +     return sysfs_emit(buf, "integer\n");
> > > +}
> > > +
> > > +/**
> > > + * attr_capdata01_get - Get the data of the specified attribute
> > > + * @tunable_attr: The attribute to be populated.
> > > + *
> > > + * Retrieves the capability data 01 struct pointer for the given
> > > + * attribute for its specified thermal mode.
> > > + *
> > > + * Return: Either a pointer to capability data, or NULL.
> > > + */
> > > +static struct capdata01 *attr_capdata01_get_data(struct lwmi_om_priv 
> *priv,
> > > +                                              u32 attribute_id)
> > > +{
> > > +     int idx;
> > > +
> > > +     for (idx = 0; idx < priv->cd01_list->count; idx++) {
> >
> > The convention for loop variables that count from 0 upwards is to use
> > unsigned type.
> >
> > > +             if (priv->cd01_list->data[idx].id != attribute_id)
> > > +                     continue;
> > > +             return &priv->cd01_list->data[idx];
> > > +     }
> > > +     return NULL;
> > > +}
> > > +
> > > +/**
> > > + * attr_capdata01_show() - Get the value of the specified attribute 
> property
> > > + *
> > > + * @kobj: Pointer to the driver object.
> > > + * @kobj_attribute: Pointer to the attribute calling this function.
> > > + * @buf: The buffer to write to.
> > > + * @tunable_attr: The attribute to be read.
> > > + * @prop: The property of this attribute to be read.
> > > + *
> > > + * Retrieves the given property from the capability data 01 struct 
> for the
> > > + * specified attribute's "custom" thermal mode. This function is 
> intended
> > > + * to be generic so it can be called from any integer attributes 
> "_show"
> > > + * function.
> > > + *
> > > + * If the WMI is success the sysfs attribute is notified.
> >
> > Add comma after success.
> >
> > > + *
> > > + * Return: Either number of characters written to buf, or an error.
> > > + */
> > > +static ssize_t attr_capdata01_show(struct kobject *kobj,
> > > +                                struct kobj_attribute *kattr, char 
> *buf,
> > > +                                struct tunable_attr_01 *tunable_attr,
> > > +                                enum attribute_property prop)
> > > +{
> > > +     struct lwmi_om_priv *priv = dev_get_drvdata(tunable_attr->dev);
> > > +     struct capdata01 *capdata;
> > > +     u32 attribute_id;
> > > +     int value;
> > > +
> > > +     attribute_id =
> > > +             FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, 
> tunable_attr->device_id) |
> > > +             FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, 
> tunable_attr->feature_id) |
> > > +             FIELD_PREP(LWMI_ATTR_MODE_ID_MASK, 
> LWMI_GZ_THERMAL_MODE_CUSTOM) |
> > > +             FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, 
> tunable_attr->type_id);
> > > +
> > > +     capdata = attr_capdata01_get_data(priv, attribute_id);
> > > +
> > > +     if (!capdata)
> > > +             return -ENODEV;
> > > +
> > > +     switch (prop) {
> > > +     case DEFAULT_VAL:
> > > +             value = capdata->default_value;
> > > +             break;
> > > +     case MAX_VAL:
> > > +             value = capdata->max_value;
> > > +             break;
> > > +     case MIN_VAL:
> > > +             value = capdata->min_value;
> > > +             break;
> > > +     case STEP_VAL:
> > > +             value = capdata->step;
> > > +             break;
> > > +     default:
> > > +             return -EINVAL;
> > > +     }
> > > +     return sysfs_emit(buf, "%d\n", value);
> > > +}
> > > +
> > > +/**
> > > + * att_current_value_store() - Set the current value of the given 
> attribute
> > > + * @kobj: Pointer to the driver object.
> > > + * @kobj_attribute: Pointer to the attribute calling this function.
> > > + * @buf: The buffer to read from, this is parsed to `int` type.
> > > + * @count: Required by sysfs attribute macros, pass in from the 
> callee attr.
> > > + * @tunable_attr: The attribute to be stored.
> > > + *
> > > + * Sets the value of the given attribute when operating under the 
> "custom"
> > > + * smartfan profile. The current smartfan profile is retrieved from 
> the
> > > + * lenovo-wmi-gamezone driver and error is returned if the result is 
> not
> > > + * "custom". This function is intended to be generic so it can be 
> called from
> > > + * any integer attribute's "_store" function. The integer to be sent 
> to the WMI
> > > + * method is range checked and an error is returned if out of range.
> > > + *
> > > + * If the value is valid and WMI is success, then the sysfs attribute 
> is
> > > + * notified.
> > > + *
> > > + * Return: Either count, or an error.
> > > + */
> > > +static ssize_t attr_current_value_store(struct kobject *kobj,
> > > +                                     struct kobj_attribute *kattr,
> > > +                                     const char *buf, size_t count,
> > > +                                     struct tunable_attr_01 
> *tunable_attr)
> > > +{
> > > +     struct lwmi_om_priv *priv = dev_get_drvdata(tunable_attr->dev);
> > > +     struct wmi_method_args_32 args;
> > > +     struct capdata01 *capdata;
> > > +     enum thermal_mode mode;
> > > +     u32 attribute_id;
> > > +     u32 value;
> > > +     int err;
> > > +
> > > +     err = lwmi_om_notifier_call(&mode);
> > > +     if (err)
> > > +             return err;
> > > +
> > > +     if (mode != LWMI_GZ_THERMAL_MODE_CUSTOM)
> > > +             return -EBUSY;
> > > +
> > > +     attribute_id =
> > > +             FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, 
> tunable_attr->device_id) |
> >
> > Just combine this to the previous line and realign the ones below.
> >
> > > +             FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, 
> tunable_attr->feature_id) |
> > > +             FIELD_PREP(LWMI_ATTR_MODE_ID_MASK, mode) |
> > > +             FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, 
> tunable_attr->type_id);
> > > +
> > > +     capdata = attr_capdata01_get_data(priv, attribute_id);
> > > +
> > > +     if (!capdata)
> > > +             return -ENODEV;
> > > +
> > > +     err = kstrtouint(buf, 10, &value);
> > > +     if (err)
> > > +             return err;
> > > +
> > > +     if (value < capdata->min_value || value > capdata->max_value)
> > > +             return -EINVAL;
> > > +
> > > +     args.arg0 = attribute_id;
> > > +     args.arg1 = value;
> > > +
> > > +     err = lwmi_dev_evaluate_int(priv->wdev, 0x0, 
> LWMI_FEATURE_VALUE_SET,
> > > +                                 (unsigned char *)&args, 
> sizeof(args), NULL);
> > > +
> > > +     if (err)
> > > +             return err;
> > > +
> > > +     return count;
> > > +};
> > > +
> > > +/**
> > > + * attr_current_value_show() - Get the current value of the given 
> attribute
> > > + * @kobj: Pointer to the driver object.
> > > + * @kobj_attribute: Pointer to the attribute calling this function.
> > > + * @buf: The buffer to write to.
> > > + * @tunable_attr: The attribute to be read.
> > > + *
> > > + * Retrieves the value of the given attribute for the current 
> smartfan profile.
> > > + * The current smartfan profile is retrieved from the 
> lenovo-wmi-gamezone driver.
> > > + * This function is intended to be generic so it can be called from 
> any integer
> > > + * attribute's "_show" function.
> > > + *
> > > + * If the WMI is success the sysfs attribute is notified.
> > > + *
> > > + * Return: Either number of characters written to buf, or an error.
> > > + */
> > > +static ssize_t attr_current_value_show(struct kobject *kobj,
> > > +                                    struct kobj_attribute *kattr, 
> char *buf,
> > > +                                    struct tunable_attr_01 
> *tunable_attr)
> > > +{
> > > +     struct lwmi_om_priv *priv = dev_get_drvdata(tunable_attr->dev);
> > > +     struct wmi_method_args_32 args;
> > > +     enum thermal_mode mode;
> > > +     u32 attribute_id;
> > > +     int retval;
> > > +     int err;
> > > +
> > > +     err = lwmi_om_notifier_call(&mode);
> > > +     if (err)
> > > +             return err;
> > > +
> > > +     attribute_id =
> > > +             FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, 
> tunable_attr->device_id) |
> >
> > Ditto.
> >
> > > +             FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, 
> tunable_attr->feature_id) |
> > > +             FIELD_PREP(LWMI_ATTR_MODE_ID_MASK, mode) |
> > > +             FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, 
> tunable_attr->type_id);
> > > +
> > > +     args.arg0 = attribute_id;
> > > +
> > > +     err = lwmi_dev_evaluate_int(priv->wdev, 0x0, 
> LWMI_FEATURE_VALUE_GET,
> > > +                                 (unsigned char *)&args, sizeof(args),
> > > +                                 &retval);
> > > +
> > > +     if (err)
> > > +             return err;
> > > +
> > > +     return sysfs_emit(buf, "%d\n", retval);
> > > +}
> > > +
> > > +/* Lenovo WMI Other Mode Attribute macros */
> > > +#define __LWMI_ATTR_RO(_func, _name)                                  
> \
> > > +     {                                                             \
> > > +             .attr = { .name = __stringify(_name), .mode = 0444 }, \
> > > +             .show = _func##_##_name##_show,                       \
> > > +     }
> > > +
> > > +#define __LWMI_ATTR_RO_AS(_name, _show)                              
>  \
> > > +     {                                                             \
> > > +             .attr = { .name = __stringify(_name), .mode = 0444 }, \
> > > +             .show = _show,                                        \
> > > +     }
> > > +
> > > +#define __LWMI_ATTR_RW(_func, _name) \
> > > +     __ATTR(_name, 0644, _func##_##_name##_show, 
> _func##_##_name##_store)
> > > +
> > > +/* Shows a formatted static variable */
> > > +#define __LWMI_ATTR_SHOW_FMT(_prop, _attrname, _fmt, _val)            
>          \
> > > +     static ssize_t _attrname##_##_prop##_show(                      
>        \
> > > +             struct kobject *kobj, struct kobj_attribute *kattr, char 
> *buf) \
> > > +     {                                                                
>       \
> > > +             return sysfs_emit(buf, _fmt, _val);                      
>       \
> > > +     }                                                                
>       \
> > > +     static struct kobj_attribute attr_##_attrname##_##_prop =        
>       \
> > > +             __LWMI_ATTR_RO(_attrname, _prop)
> > > +
> > > +/* Attribute current value read/write */
> > > +#define __LWMI_TUNABLE_CURRENT_VALUE_CAP01(_attrname)                
>           \
> > > +     static ssize_t _attrname##_current_value_store(                  
>       \
> > > +             struct kobject *kobj, struct kobj_attribute *kattr,      
>       \
> > > +             const char *buf, size_t count)                          
>        \
> > > +     {                                                                
>       \
> > > +             return attr_current_value_store(kobj, kattr, buf, 
> count,       \
> > > +                                             &_attrname);            
>        \
> > > +     }                                                                
>       \
> > > +     static ssize_t _attrname##_current_value_show(                  
>        \
> > > +             struct kobject *kobj, struct kobj_attribute *kattr, char 
> *buf) \
> > > +     {                                                                
>       \
> > > +             return attr_current_value_show(kobj, kattr, buf, 
> &_attrname);  \
> > > +     }                                                                
>       \
> > > +     static struct kobj_attribute attr_##_attrname##_current_value =  
>       \
> > > +             __LWMI_ATTR_RW(_attrname, current_value)
> > > +
> > > +/* Attribute property read only */
> > > +#define __LWMI_TUNABLE_RO_CAP01(_prop, _attrname, _prop_type)        
>           \
> > > +     static ssize_t _attrname##_##_prop##_show(                      
>        \
> > > +             struct kobject *kobj, struct kobj_attribute *kattr, char 
> *buf) \
> > > +     {                                                                
>       \
> > > +             return attr_capdata01_show(kobj, kattr, buf, 
> &_attrname,       \
> > > +                                        _prop_type);                  
>       \
> > > +     }                                                                
>       \
> > > +     static struct kobj_attribute attr_##_attrname##_##_prop =        
>       \
> > > +             __LWMI_ATTR_RO(_attrname, _prop)
> > > +
> > > +#define LWMI_ATTR_GROUP_TUNABLE_CAP01(_attrname, _fsname, _dispname)  
>     \
> > > +     __LWMI_TUNABLE_CURRENT_VALUE_CAP01(_attrname);                  
>   \
> > > +     __LWMI_TUNABLE_RO_CAP01(default_value, _attrname, DEFAULT_VAL);  
>  \
> > > +     __LWMI_ATTR_SHOW_FMT(display_name, _attrname, "%s\n", 
> _dispname); \
> > > +     __LWMI_TUNABLE_RO_CAP01(max_value, _attrname, MAX_VAL);          
>  \
> > > +     __LWMI_TUNABLE_RO_CAP01(min_value, _attrname, MIN_VAL);          
>  \
> > > +     __LWMI_TUNABLE_RO_CAP01(scalar_increment, _attrname, STEP_VAL);  
>  \
> > > +     static struct kobj_attribute attr_##_attrname##_type =          
>   \
> > > +             __LWMI_ATTR_RO_AS(type, int_type_show);                  
>  \
> > > +     static struct attribute *_attrname##_attrs[] = {                
>   \
> > > +             &attr_##_attrname##_current_value.attr,                  
>  \
> > > +             &attr_##_attrname##_default_value.attr,                  
>  \
> > > +             &attr_##_attrname##_display_name.attr,                  
>   \
> > > +             &attr_##_attrname##_max_value.attr,                      
>  \
> > > +             &attr_##_attrname##_min_value.attr,                      
>  \
> > > +             &attr_##_attrname##_scalar_increment.attr,              
>   \
> > > +             &attr_##_attrname##_type.attr,                          
>   \
> > > +             NULL,                                                    
>  \
> > > +     };                                                              
>   \
> > > +     static const struct attribute_group _attrname##_attr_group = {  
>   \
> > > +             .name = _fsname, .attrs = _attrname##_attrs              
>  \
> > > +     }
> > > +
> > > +LWMI_ATTR_GROUP_TUNABLE_CAP01(ppt_pl1_spl, "ppt_pl1_spl",
> > > +                           "Set the CPU sustained power limit");
> > > +LWMI_ATTR_GROUP_TUNABLE_CAP01(ppt_pl2_sppt, "ppt_pl2_sppt",
> > > +                           "Set the CPU slow package power tracking 
> limit");
> > > +LWMI_ATTR_GROUP_TUNABLE_CAP01(ppt_pl3_fppt, "ppt_pl3_fppt",
> > > +                           "Set the CPU fast package power tracking 
> limit");
> > > +
> > > +static struct capdata01_attr_group cd01_attr_groups[] = {
> > > +     { &ppt_pl1_spl_attr_group, &ppt_pl1_spl },
> > > +     { &ppt_pl2_sppt_attr_group, &ppt_pl2_sppt },
> > > +     { &ppt_pl3_fppt_attr_group, &ppt_pl3_fppt },
> > > +     {},
> > > +};
> > > +
> > > +/**
> > > + * lwmi_om_fw_attr_add() - Register all firmware_attributes_class 
> members
> > > + * @priv: The Other Mode driver data.
> > > + *
> > > + * Return: Either 0, or an error.
> > > + */
> > > +static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv)
> > > +{
> > > +     unsigned int i;
> > > +     int err;
> > > +
> > > +     priv->ida_id = ida_alloc(&lwmi_om_ida, GFP_KERNEL);
> > > +     if (priv->ida_id < 0)
> > > +             return priv->ida_id;
> > > +
> > > +     priv->fw_attr_dev = device_create(&firmware_attributes_class, 
> NULL,
> > > +                                       MKDEV(0, 0), NULL, "%s-%u",
> > > +                                       LWMI_OM_FW_ATTR_BASE_PATH,
> > > +                                       priv->ida_id);
> > > +     if (IS_ERR(priv->fw_attr_dev)) {
> >
> > Add include for IS_ERR().
> >
> > > +             err = PTR_ERR(priv->fw_attr_dev);
> > > +             goto err_free_ida;
> > > +     }
> > > +
> > > +     priv->fw_attr_kset = kset_create_and_add("attributes", NULL, 
> &priv->fw_attr_dev->kobj);
> > > +     if (!priv->fw_attr_kset) {
> > > +             err = -ENOMEM;
> > > +             goto err_destroy_classdev;
> > > +     }
> > > +
> > > +     for (i = 0; i < ARRAY_SIZE(cd01_attr_groups) - 1; i++) {
> >
> > Add include for ARRAY_SIZE().
> >
> > > +             err = sysfs_create_group(&priv->fw_attr_kset->kobj, 
> cd01_attr_groups[i].attr_group);
> > > +             if (err)
> > > +                     goto err_remove_groups;
> > > +
> > > +             cd01_attr_groups[i].tunable_attr->dev = &priv->wdev->dev;
> > > +     }
> > > +     return 0;
> > > +
> > > +err_remove_groups:
> > > +     while (i--)
> > > +             sysfs_remove_group(&priv->fw_attr_kset->kobj, 
> cd01_attr_groups[i].attr_group);
> > > +
> > > +     kset_unregister(priv->fw_attr_kset);
> > > +
> > > +err_destroy_classdev:
> > > +     device_unregister(priv->fw_attr_dev);
> > > +
> > > +err_free_ida:
> > > +     ida_free(&lwmi_om_ida, priv->ida_id);
> > > +     return err;
> > > +}
> > > +
> > > +/**
> > > + * lwmi_om_fw_attr_remove() - Unregister all capability data 
> attribute groups
> > > + * @priv: the lenovo-wmi-other driver data.
> > > + */
> > > +static void lwmi_om_fw_attr_remove(struct lwmi_om_priv *priv)
> > > +{
> > > +     for (unsigned int i = 0; i < ARRAY_SIZE(cd01_attr_groups) - 1; 
> i++)
> > > +             sysfs_remove_group(&priv->fw_attr_kset->kobj, 
> cd01_attr_groups[i].attr_group);
> > > +
> > > +     kset_unregister(priv->fw_attr_kset);
> > > +     device_unregister(priv->fw_attr_dev);
> > > +}
> > > +
> > > +/**
> > > + * lwmi_om_master_bind() - Bind all components of the other mode 
> driver
> > > + * @dev: The lenovo-wmi-other driver basic device.
> > > + *
> > > + * Call component_bind_all to bind the lenovo-wmi-capdata01 driver to 
> the
> > > + * lenovo-wmi-other master driver. On success, assign the capability 
> data 01
> > > + * list pointer to the driver data struct for later access. This 
> pointer
> > > + * is only valid while the capdata01 interface exists. Finally, 
> register all
> > > + * firmware attribute groups.
> > > + *
> > > + * Return: 0 on success, or an error.
> > > + */
> > > +static int lwmi_om_master_bind(struct device *dev)
> > > +{
> > > +     struct lwmi_om_priv *priv = dev_get_drvdata(dev);
> > > +     struct cd01_list *tmp_list;
> > > +     int ret;
> > > +
> > > +     ret = component_bind_all(dev, &tmp_list);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     priv->cd01_list = tmp_list;
> > > +
> > > +     if (!priv->cd01_list)
> > > +             return -ENODEV;
> > > +
> > > +     return lwmi_om_fw_attr_add(priv);
> > > +}
> > > +
> > > +/**
> > > + * lwmi_om_master_unbind() - Unbind all components of the other mode 
> driver
> > > + * @dev: The lenovo-wmi-other driver basic device
> > > + *
> > > + * Unregister all capability data attribute groups. Then call
> > > + * component_unbind_all to unbind the lenovo-wmi-capdata01 driver 
> from the
> > > + * lenovo-wmi-other master driver. Finally, free the IDA for this 
> device.
> > > + */
> > > +static void lwmi_om_master_unbind(struct device *dev)
> > > +{
> > > +     struct lwmi_om_priv *priv = dev_get_drvdata(dev);
> > > +
> > > +     lwmi_om_fw_attr_remove(priv);
> > > +     component_unbind_all(dev, NULL);
> > > +}
> > > +
> > > +static const struct component_master_ops lwmi_om_master_ops = {
> > > +     .bind = lwmi_om_master_bind,
> > > +     .unbind = lwmi_om_master_unbind,
> > > +};
> > > +
> > > +static int lwmi_other_probe(struct wmi_device *wdev, const void 
> *context)
> > > +{
> > > +     struct component_match *master_match = NULL;
> > > +     struct lwmi_om_priv *priv;
> > > +
> > > +     priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
> > > +     if (!priv)
> > > +             return -ENOMEM;
> > > +
> > > +     priv->wdev = wdev;
> > > +     dev_set_drvdata(&wdev->dev, priv);
> > > +
> > > +     component_match_add(&wdev->dev, &master_match, lwmi_cd01_match, 
> NULL);
> > > +     if (IS_ERR(master_match))
> > > +             return PTR_ERR(master_match);
> > > +
> > > +     return component_master_add_with_match(&wdev->dev, 
> &lwmi_om_master_ops,
> > > +                                            master_match);
> > > +}
> > > +
> > > +static void lwmi_other_remove(struct wmi_device *wdev)
> > > +{
> > > +     struct lwmi_om_priv *priv = dev_get_drvdata(&wdev->dev);
> > > +
> > > +     component_master_del(&wdev->dev, &lwmi_om_master_ops);
> > > +     ida_free(&lwmi_om_ida, priv->ida_id);
> > > +}
> > > +
> > > +static const struct wmi_device_id lwmi_other_id_table[] = {
> > > +     { LENOVO_OTHER_MODE_GUID, NULL },
> > > +     {}
> > > +};
> > > +
> > > +static struct wmi_driver lwmi_other_driver = {
> > > +     .driver = {
> > > +             .name = "lenovo_wmi_other",
> > > +             .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> > > +     },
> > > +     .id_table = lwmi_other_id_table,
> > > +     .probe = lwmi_other_probe,
> > > +     .remove = lwmi_other_remove,
> > > +     .no_singleton = true,
> > > +};
> > > +
> > > +module_wmi_driver(lwmi_other_driver);
> > > +
> > > +MODULE_IMPORT_NS("LENOVO_WMI_CD01");
> > > +MODULE_IMPORT_NS("LENOVO_WMI_HELPERS");
> > > +MODULE_DEVICE_TABLE(wmi, lwmi_other_id_table);
> > > +MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@xxxxxxxxx>");
> > > +MODULE_DESCRIPTION("Lenovo Other Mode WMI Driver");
> > > +MODULE_LICENSE("GPL");
> > > diff --git a/drivers/platform/x86/lenovo-wmi-other.h 
> b/drivers/platform/x86/lenovo-wmi-other.h
> > > new file mode 100644
> > > index 000000000000..49bc3521e184
> > > --- /dev/null
> > > +++ b/drivers/platform/x86/lenovo-wmi-other.h
> > > @@ -0,0 +1,16 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +
> > > +/* Copyright(C) 2025 Derek J. Clark <derekjohn.clark@xxxxxxxxx> */
> > > +
> > > +#ifndef _LENOVO_WMI_OTHER_H_
> > > +#define _LENOVO_WMI_OTHER_H_
> > > +
> > > +struct device;
> > > +struct notifier_block;
> > > +
> > > +int lwmi_om_register_notifier(struct notifier_block *nb);
> > > +int lwmi_om_unregister_notifier(struct notifier_block *nb);
> > > +int devm_lwmi_om_register_notifier(struct device *dev,
> > > +                                struct notifier_block *nb);
> > > +
> > > +#endif /* !_LENOVO_WMI_OTHER_H_ */
> >
> > Overall, the series still has lots of nits still to address (please look
> > for similar cases I've marked as I surely didn't mark each instance).
> > But logicwise, the code is easy to read, feels understandable, and I
> > couldn't find any big issues. Good work so far! :-)
> >
> > --
> >  i.
> >
> Thanks Ilpo, I'll take a look at these in the coming days-.
>
> There is something that was just brought up to me from Xino at Lenovo
> after some internal testing that I wasn't aware of, and it needs to be
> addressed. I want to discuss a way ahead before I add it to v6.
>
> Essentially, there was an assumption that the capability data is
> static once the device initializes, which is why back in v1 it was
> discussed to cache the data. This assumption was wrong; the capability
> data for max value changes depending on if AC or DC is being used to
> power the device, with the DC values being lower. The BIOS will
> automatically throttle any values that exceed the DC limits upon a
> change in status, so we don't need to account for this event in other
> mode directly, but we do need to re-cache the data blocks when there
> is an ACPI_AC_NOTIFY_STATUS event. This will allow us to both report
> accurate information, and prevent setting values that exceed the power
> draw limit of the battery after the event, which could be possible.
>
> My plan is (in Capability data driver):
>
> - Use the _setup function in _probe to call two new functions that are
> essentially the current _setup broken apart. The first (_allocate)
> will only run during _setup and will get the wmi block device count
> and allocate the list struct, and the second (_cache)  will loop
> through all the data blocks and copy the data to the list struct.
> - Subscribe to the register_acpi_notifier with a new notifier block in
> _probe, and in a new notify_call function run the _cache function upon
> ACPI_AC_NOTIFY_STATUS events.
>
> The main problem I see is that we will need to guard the access to the
> cd01_list now, as it could have async read/write. Since we are passing
> a pointer to that data to another driver I see two possible solutions
> to that:
> - Share a mutex between the two drivers.
> - Do all lookups of the capability data in the capability data driver
> and pass pointers back to the other mode driver.
>
> I personally prefer option 2. I can pass the existing list pointer to
> an exported function from the capability data driver and use its
> global mutex to iterate through it. I could also change the pointer
> passed in _bind to a device pointer, then pass it back in the exported
> function and use dev_get_data to access the private drvdata in the
> capability data driver, but Armin was against this previously so I'm
> unsure about it. I think the example i915 driver does store pointers
> to devices so I'm unsure why this wouldn't be preferable now, as it
> would prevent access to the data outside a guard in all contexts.
>
> As far as I'm aware, the only way to share a mutex would be to point
> to the private data between the two drivers, but I fully admit I could
> be wrong about that. I'm waiting for comments before proceeding
> further.
>
> Thank you,
> - Derek
>

Ilpo/Armin,

Any thoughts on this? I've successfully tested passing the device pointer for capability data to other mode and retrieving the accurate data when needed; re-caching on ACPI AC change events. It seems to work well this way and I'm able to contain the mutex in a single driver this way. I'd like a bit more than tacit approval of the concept before submitting the next version to avoid more unnecessary iterations if possible.

Thanks,
Derek