Re: [PATCH v4 3/8] platform/x86: lenovo-wmi-other: Add lwmi_attr_id() function

[Derek J. Clark]

On March 14, 2026 6:08:55 PM PDT, Rong Zhang <i@xxxxxxxx> wrote:
>Hi Derek,
>
>On Thu, 2026-03-12 at 03:10 +0000, Derek J. Clark wrote:
>> Adds lwmi_attr_id() function. In the same vein as LWMI_ATTR_ID_FAN_RPM(),
>> but as a generic, to de-duplicate attribute_id assignment biolerplate.
>> 
>> Reviewed-by: Mark Pearson <mpearson-lenovo@xxxxxxxxx>
>> Signed-off-by: Derek J. Clark <derekjohn.clark@xxxxxxxxx>
>> ---
>> v4:
>>   - Switch from macro to static inline to preserve types.
>> ---
>>  drivers/platform/x86/lenovo/wmi-gamezone.h |  1 +
>>  drivers/platform/x86/lenovo/wmi-other.c    | 59 +++++++++++++---------
>>  2 files changed, 35 insertions(+), 25 deletions(-)
>> 
>> diff --git a/drivers/platform/x86/lenovo/wmi-gamezone.h b/drivers/platform/x86/lenovo/wmi-gamezone.h
>> index 6b163a5eeb95..ddb919cf6c36 100644
>> --- a/drivers/platform/x86/lenovo/wmi-gamezone.h
>> +++ b/drivers/platform/x86/lenovo/wmi-gamezone.h
>> @@ -10,6 +10,7 @@ enum gamezone_events_type {
>>  };
>>  
>>  enum thermal_mode {
>> +	LWMI_GZ_THERMAL_MODE_NONE =	   0x00,
>>  	LWMI_GZ_THERMAL_MODE_QUIET =	   0x01,
>>  	LWMI_GZ_THERMAL_MODE_BALANCED =	   0x02,
>>  	LWMI_GZ_THERMAL_MODE_PERFORMANCE = 0x03,
>> diff --git a/drivers/platform/x86/lenovo/wmi-other.c b/drivers/platform/x86/lenovo/wmi-other.c
>> index c1728c7c2957..9fff9c1f768c 100644
>> --- a/drivers/platform/x86/lenovo/wmi-other.c
>> +++ b/drivers/platform/x86/lenovo/wmi-other.c
>> @@ -73,10 +73,26 @@
>>  
>>  #define LWMI_FAN_DIV 100
>>  
>> -#define LWMI_ATTR_ID_FAN_RPM(x)						\
>> -	(FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, LWMI_DEVICE_ID_FAN) |	\
>> -	 FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, LWMI_FEATURE_ID_FAN_RPM) |	\
>> -	 FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, LWMI_FAN_ID(x)))
>> +/**
>> + * lwmi_attr_id() - Formats a capability data attribute ID
>> + * @dev_id: The u8 corresponding to the device ID.
>> + * @feat_id: The u8 corresponding to the feature ID on the device.
>> + * @mode_id: The u8 corresponding to the wmi-gamezone mode for set/get.
>> + * @type_id: The u8 corresponding to the sub-device.
>> + *
>> + * Return: u32.
>> + */
>> +static u32 lwmi_attr_id(u8 dev_id, u8 feat_id, u8 mode_id, u8 type_id)
>> +{
>> +	return (FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, dev_id)   |
>> +		FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, feat_id) |
>> +		FIELD_PREP(LWMI_ATTR_MODE_ID_MASK, mode_id) |
>> +		FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, type_id));
>> +}
>> +
>> +#define LWMI_ATTR_ID_FAN_RPM(x)                                   \
>> +	lwmi_attr_id(LWMI_DEVICE_ID_FAN, LWMI_FEATURE_ID_FAN_RPM, \
>> +		     LWMI_GZ_THERMAL_MODE_NONE, LWMI_FAN_ID(x))
>>  
>>  #define LWMI_OM_FW_ATTR_BASE_PATH "lenovo-wmi-other"
>>  #define LWMI_OM_HWMON_NAME "lenovo_wmi_other"
>> @@ -550,6 +566,8 @@ struct tunable_attr_01 {
>>  	u8 feature_id;
>>  	u8 device_id;
>>  	u8 type_id;
>> +	u8 cd_mode_id; /* mode arg for searching capdata */
>> +	u8 cv_mode_id; /* mode arg for set/get current_value */
>
>Adding them actually depends on [PATCH v4 4/8], otherwise you always
>get 0 when accessing them in this patch. There are potential
>regressions...
>
>
Hi Rong,

Good catch. I think they got moved during a rebase edit by mistake. Thanks.

- Derek

>>  };
>>  
>>  static struct tunable_attr_01 ppt_pl1_spl = {
>> @@ -715,12 +733,8 @@ static ssize_t attr_capdata01_show(struct kobject *kobj,
>>  	u32 attribute_id;
>>  	int value, ret;
>>  
>> -	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);
>> +	attribute_id = lwmi_attr_id(tunable_attr->device_id, tunable_attr->feature_id,
>> +				    LWMI_GZ_THERMAL_MODE_CUSTOM, tunable_attr->type_id);
>>  
>>  	ret = lwmi_cd01_get_data(priv->cd01_list, attribute_id, &capdata);
>>  	if (ret)
>> @@ -775,7 +789,6 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
>>  	struct wmi_method_args_32 args;
>>  	struct capdata01 capdata;
>>  	enum thermal_mode mode;
>> -	u32 attribute_id;
>>  	u32 value;
>>  	int ret;
>>  
>> @@ -786,13 +799,10 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
>>  	if (mode != LWMI_GZ_THERMAL_MODE_CUSTOM)
>>  		return -EBUSY;
>>  
>> -	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, mode) |
>> -		FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, tunable_attr->type_id);
>> +	args.arg0 = lwmi_attr_id(tunable_attr->device_id, tunable_attr->feature_id,
>> +				 mode, tunable_attr->type_id);
>>  
>> -	ret = lwmi_cd01_get_data(priv->cd01_list, attribute_id, &capdata);
>> +	ret = lwmi_cd01_get_data(priv->cd01_list, args.arg0, &capdata);
>>  	if (ret)
>>  		return ret;
>>  
>> @@ -803,7 +813,8 @@ static ssize_t attr_current_value_store(struct kobject *kobj,
>>  	if (value < capdata.min_value || value > capdata.max_value)
>>  		return -EINVAL;
>>  
>> -	args.arg0 = attribute_id;
>> +	args.arg0 = lwmi_attr_id(tunable_attr->device_id, tunable_attr->feature_id,
>> +				 tunable_attr->cv_mode_id, tunable_attr->type_id);
>
>...here...
>
>>  	args.arg1 = value;
>>  
>>  	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_SET,
>> @@ -837,7 +848,6 @@ static ssize_t attr_current_value_show(struct kobject *kobj,
>>  	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 ret;
>>  
>> @@ -845,13 +855,12 @@ static ssize_t attr_current_value_show(struct kobject *kobj,
>>  	if (ret)
>>  		return ret;
>>  
>> -	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, mode) |
>> -		FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, tunable_attr->type_id);
>> +	/* If "no-mode" is the supported mode, ensure we never send current mode */
>> +	if (tunable_attr->cv_mode_id == LWMI_GZ_THERMAL_MODE_NONE)
>> +		mode = tunable_attr->cv_mode_id;
>
>...and here.
>
>Thanks,
>Rong
>
>>  
>> -	args.arg0 = attribute_id;
>> +	args.arg0 = lwmi_attr_id(tunable_attr->device_id, tunable_attr->feature_id,
>> +				 mode, tunable_attr->type_id);
>>  
>>  	ret = lwmi_dev_evaluate_int(priv->wdev, 0x0, LWMI_FEATURE_VALUE_GET,
>>  				    (unsigned char *)&args, sizeof(args),