Re: [PATCH v6 03/12] platform/x86: alienware-wmi-wmax: Improve internal AWCC API

[Kurt Borja]

On Fri Mar 28, 2025 at 11:51 AM -03, Ilpo Järvinen wrote:
> On Thu, 13 Mar 2025, Kurt Borja wrote:
>
>> Inline all AWCC WMI helper methods and directly return the newly
>> introduced __awcc_wmi_command() helper to simplify implementation.
>> 
>> Drop awcc_thermal_control() in favor of awcc_op_activate_profile().
>> 
>> Add awcc_op_get_resource_id(), awcc_op_get_current_profile() and a new
>> failure code.
>> 
>> Reviewed-by: Armin Wolf <W_Armin@xxxxxx>
>> Signed-off-by: Kurt Borja <kuurtb@xxxxxxxxx>
>> ---
>>  drivers/platform/x86/dell/alienware-wmi-wmax.c | 150 +++++++++++++++----------
>>  1 file changed, 91 insertions(+), 59 deletions(-)
>> 
>> diff --git a/drivers/platform/x86/dell/alienware-wmi-wmax.c b/drivers/platform/x86/dell/alienware-wmi-wmax.c
>> index 80aefba5b22d6b4ac18aeb2ca356f8c911150abd..b9dbfdc8096c571722b3c7ac3e73989e235e2eb9 100644
>> --- a/drivers/platform/x86/dell/alienware-wmi-wmax.c
>> +++ b/drivers/platform/x86/dell/alienware-wmi-wmax.c
>> @@ -32,6 +32,7 @@
>>  #define AWCC_THERMAL_MODE_GMODE			0xAB
>>  
>>  #define AWCC_FAILURE_CODE			0xFFFFFFFF
>> +#define AWCC_FAILURE_CODE_2			0xFFFFFFFE
>>  #define AWCC_THERMAL_TABLE_MASK			GENMASK(7, 4)
>>  #define AWCC_THERMAL_MODE_MASK			GENMASK(3, 0)
>>  /* Some IDs have a BIT(8) flag that we ignore */
>> @@ -443,8 +444,7 @@ const struct attribute_group wmax_deepsleep_attribute_group = {
>>  };
>>  
>>  /*
>> - * Thermal Profile control
>> - *  - Provides thermal profile control through the Platform Profile API
>> + * AWCC Helpers
>>   */
>>  static bool is_awcc_thermal_profile_id(u8 code)
>>  {
>> @@ -463,72 +463,107 @@ static bool is_awcc_thermal_profile_id(u8 code)
>>  	return false;
>>  }
>>  
>> -static int awcc_thermal_information(struct wmi_device *wdev, u8 operation,
>> -				    u8 arg, u32 *out_data)
>> +static int __awcc_wmi_command(struct wmi_device *wdev, u32 method_id,
>> +			      struct wmax_u32_args *args, u32 *out)
>
> Why did you put __ into the name?
>
> Some people oppose __ prefix altogether, I don't entirely agree with their 
> stance but I don't really understand what the underscores here signify.
>
> Normally I see __ being used in three main cases:
> - non-__ variant does some locking around the call too the __ func (though 
>   many funcs use _locked postfix these days).
> - func is "dangerous" and caller has to really know what he/she is
>   doing / be careful for some reason.
> - non-__ variant exists and somebody cannot come up better name for the
>   internally called function (not very good use case, IMHO)

Oh - I didn't know. Andy also mentioned this prefix is used for
non-atomic versions of some functions like __set_bit().

>
> I don't see any of those apply here, this looks just an ordinary wrapper 
> function, thus the question.

I did it to indicate it's a "private" method, so it's not used directly.
But ofc that's not the convention here, so I'll drop it.

>
>>  {
>> -	struct wmax_u32_args in_args = {
>> +	int ret;
>> +
>> +	ret = alienware_wmi_command(wdev, method_id, args, sizeof(*args), out);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (*out == AWCC_FAILURE_CODE || *out == AWCC_FAILURE_CODE_2)
>> +		return -EBADRQC;
>> +
>> +	return 0;
>> +}
>> +
>> +static inline int awcc_thermal_information(struct wmi_device *wdev, u8 operation,
>> +					   u8 arg, u32 *out)
>> +{
>> +	struct wmax_u32_args args = {
>>  		.operation = operation,
>>  		.arg1 = arg,
>>  		.arg2 = 0,
>>  		.arg3 = 0,
>>  	};
>> -	int ret;
>>  
>> -	ret = alienware_wmi_command(wdev, AWCC_METHOD_THERMAL_INFORMATION,
>> -				    &in_args, sizeof(in_args), out_data);
>> -	if (ret < 0)
>> -		return ret;
>> -
>> -	if (*out_data == AWCC_FAILURE_CODE)
>> -		return -EBADRQC;
>> -
>> -	return 0;
>> +	return __awcc_wmi_command(wdev, AWCC_METHOD_THERMAL_INFORMATION, &args, out);
>>  }
>>  
>> -static int awcc_thermal_control(struct wmi_device *wdev, u8 profile)
>> +static inline int awcc_game_shift_status(struct wmi_device *wdev, u8 operation,
>
> As a general rule, don't add inline to .c files, we leave figuring 
> inlining out to the compiler.

Good to know, thanks!

>
>> +					 u32 *out)
>>  {
>> -	struct wmax_u32_args in_args = {
>> -		.operation = AWCC_OP_ACTIVATE_PROFILE,
>> -		.arg1 = profile,
>> -		.arg2 = 0,
>> -		.arg3 = 0,
>> -	};
>> -	u32 out_data;
>> -	int ret;
>> -
>> -	ret = alienware_wmi_command(wdev, AWCC_METHOD_THERMAL_CONTROL,
>> -				    &in_args, sizeof(in_args), &out_data);
>> -	if (ret)
>> -		return ret;
>> -
>> -	if (out_data == AWCC_FAILURE_CODE)
>> -		return -EBADRQC;
>> -
>> -	return 0;
>> -}
>> -
>> -static int awcc_game_shift_status(struct wmi_device *wdev, u8 operation,
>> -				  u32 *out_data)
>> -{
>> -	struct wmax_u32_args in_args = {
>> +	struct wmax_u32_args args = {
>>  		.operation = operation,
>>  		.arg1 = 0,
>>  		.arg2 = 0,
>>  		.arg3 = 0,
>>  	};
>> -	int ret;
>>  
>> -	ret = alienware_wmi_command(wdev, AWCC_METHOD_GAME_SHIFT_STATUS,
>> -				    &in_args, sizeof(in_args), out_data);
>> -	if (ret < 0)
>> -		return ret;
>> -
>> -	if (*out_data == AWCC_FAILURE_CODE)
>> -		return -EOPNOTSUPP;
>> -
>> -	return 0;
>> +	return __awcc_wmi_command(wdev, AWCC_METHOD_GAME_SHIFT_STATUS, &args, out);
>>  }
>>  
>> +/**
>> + * awcc_op_get_resource_id - Get the resource ID at a given index
>> + * @wdev: AWCC WMI device
>> + * @index: Index
>> + * @out: Value returned by the WMI call
>> + *
>> + * Get the resource ID at a given index. Resource IDs are listed in the
>
> Use @index to refer to the argument.

Ack

>
>> + * following order:
>> + *
>> + *	- Fan IDs
>> + *	- Sensor IDs
>> + *	- Unknown IDs
>> + *	- Thermal Profile IDs
>> + *
>> + * The total number of IDs of a given type can be obtained with
>> + * AWCC_OP_GET_SYSTEM_DESCRIPTION.
>> + *
>> + * Return: 0 on success, -errno on failure
>> + */
>> +static inline int awcc_op_get_resource_id(struct wmi_device *wdev, u8 index, u32 *out)
>> +{
>> +	struct wmax_u32_args args = {
>> +		.operation = AWCC_OP_GET_RESOURCE_ID,
>> +		.arg1 = index,
>> +		.arg2 = 0,
>> +		.arg3 = 0,
>> +	};
>> +
>> +	return __awcc_wmi_command(wdev, AWCC_METHOD_THERMAL_INFORMATION, &args, out);
>> +}
>> +
>> +static inline int awcc_op_get_current_profile(struct wmi_device *wdev, u32 *out)
>> +{
>> +	struct wmax_u32_args args = {
>> +		.operation = AWCC_OP_GET_CURRENT_PROFILE,
>> +		.arg1 = 0,
>> +		.arg2 = 0,
>> +		.arg3 = 0,
>> +	};
>> +
>> +	return __awcc_wmi_command(wdev, AWCC_METHOD_THERMAL_INFORMATION, &args, out);
>> +}
>> +
>> +static inline int awcc_op_activate_profile(struct wmi_device *wdev, u8 profile)
>> +{
>> +	struct wmax_u32_args args = {
>> +		.operation = AWCC_OP_ACTIVATE_PROFILE,
>> +		.arg1 = profile,
>> +		.arg2 = 0,
>> +		.arg3 = 0,
>> +	};
>> +	u32 out;
>> +
>> +	return __awcc_wmi_command(wdev, AWCC_METHOD_THERMAL_CONTROL, &args, &out);
>> +}
>> +
>> +/*
>> + * Thermal Profile control
>> + *  - Provides thermal profile control through the Platform Profile API
>> + */
>>  static int awcc_platform_profile_get(struct device *dev,
>>  				     enum platform_profile_option *profile)
>>  {
>> @@ -536,10 +571,8 @@ static int awcc_platform_profile_get(struct device *dev,
>>  	u32 out_data;
>>  	int ret;
>>  
>> -	ret = awcc_thermal_information(priv->wdev, AWCC_OP_GET_CURRENT_PROFILE,
>> -				       0, &out_data);
>> -
>> -	if (ret < 0)
>> +	ret = awcc_op_get_current_profile(priv->wdev, &out_data);
>> +	if (ret)
>>  		return ret;
>>  
>>  	if (out_data == AWCC_THERMAL_MODE_GMODE) {
>> @@ -550,7 +583,7 @@ static int awcc_platform_profile_get(struct device *dev,
>>  	if (!is_awcc_thermal_profile_id(out_data))
>>  		return -ENODATA;
>>  
>> -	out_data &= AWCC_THERMAL_MODE_MASK;
>> +	out_data = FIELD_GET(AWCC_THERMAL_MODE_MASK, out_data);
>
> Should this be part of the earlier patch??

I think I'll leave it in this patch, but I'll move it to
awcc_op_get_resource_id().

-- 
 ~ Kurt

>
>>  	*profile = awcc_mode_to_platform_profile[out_data];
>>  
>>  	return 0;
>> @@ -583,8 +616,8 @@ static int awcc_platform_profile_set(struct device *dev,
>>  		}
>>  	}
>>  
>> -	return awcc_thermal_control(priv->wdev,
>> -				    priv->supported_thermal_profiles[profile]);
>> +	return awcc_op_activate_profile(priv->wdev,
>> +					priv->supported_thermal_profiles[profile]);
>>  }
>>  
>>  static int awcc_platform_profile_probe(void *drvdata, unsigned long *choices)
>> @@ -606,8 +639,7 @@ static int awcc_platform_profile_probe(void *drvdata, unsigned long *choices)
>>  	first_mode = sys_desc[0] + sys_desc[1];
>>  
>>  	for (u32 i = 0; i < sys_desc[3]; i++) {
>> -		ret = awcc_thermal_information(priv->wdev, AWCC_OP_GET_RESOURCE_ID,
>> -					       i + first_mode, &out_data);
>> +		ret = awcc_op_get_resource_id(priv->wdev, i + first_mode, &out_data);
>>  
>>  		if (ret == -EIO)
>>  			return ret;
>> 
>>