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

[Ilpo Järvinen]

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)

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

>  {
> -	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.

> +					 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.

> + * 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??

>  	*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;
> 
> 

-- 
 i.