Re: [PATCH v2 02/10] platform/x86: alienware-wmi-wmax: Refactor is_awcc_thermal_mode()

[Kurt Borja]

Hi Ilpo,

On Tue Mar 4, 2025 at 10:02 AM -05, Ilpo Järvinen wrote:
> On Tue, 25 Feb 2025, Kurt Borja wrote:
>
>> Refactor is_awcc_thermal_mode() to use FIELD_GET() instead of bitwise
>> operations. Drop the check for BIT(8) sensor flag and rename it to
>> is_awcc_thermal_profile_id().
>> 
>> Signed-off-by: Kurt Borja <kuurtb@xxxxxxxxx>
>> Reviewed-by: Armin Wolf <W_Armin@xxxxxx>
>> ---
>>  .../platform/x86/dell/alienware-wmi-wmax.c    | 31 ++++++++++---------
>>  1 file changed, 16 insertions(+), 15 deletions(-)
>> 
>> diff --git a/drivers/platform/x86/dell/alienware-wmi-wmax.c b/drivers/platform/x86/dell/alienware-wmi-wmax.c
>> index ed70e12d73d7..7f0aa88221d6 100644
>> --- a/drivers/platform/x86/dell/alienware-wmi-wmax.c
>> +++ b/drivers/platform/x86/dell/alienware-wmi-wmax.c
>> @@ -34,7 +34,7 @@
>>  #define AWCC_FAILURE_CODE			0xFFFFFFFF
>>  #define AWCC_THERMAL_TABLE_MASK			GENMASK(7, 4)
>>  #define AWCC_THERMAL_MODE_MASK			GENMASK(3, 0)
>> -#define AWCC_SENSOR_ID_MASK			BIT(8)
>> +#define AWCC_RESOURCE_ID_MASK			GENMASK(7, 0)
>>  
>>  static bool force_platform_profile;
>>  module_param_unsafe(force_platform_profile, bool, 0);
>> @@ -168,8 +168,8 @@ enum AWCC_GAME_SHIFT_STATUS_OPERATIONS {
>>  };
>>  
>>  enum AWCC_THERMAL_TABLES {
>> -	AWCC_THERMAL_TABLE_LEGACY		= 0x90,
>> -	AWCC_THERMAL_TABLE_USTT			= 0xA0,
>> +	AWCC_THERMAL_TABLE_LEGACY		= 0x9,
>> +	AWCC_THERMAL_TABLE_USTT			= 0xA,
>>  };
>>  
>>  enum awcc_thermal_profile {
>> @@ -445,20 +445,18 @@ const struct attribute_group wmax_deepsleep_attribute_group = {
>>   * Thermal Profile control
>>   *  - Provides thermal profile control through the Platform Profile API
>>   */
>> -static bool is_awcc_thermal_mode(u32 code)
>> +static bool is_awcc_thermal_profile_id(u8 code)
>>  {
>> -	if (code & AWCC_SENSOR_ID_MASK)
>> -		return false;
>> +	u8 table = FIELD_GET(AWCC_THERMAL_TABLE_MASK, code);
>> +	u8 mode = FIELD_GET(AWCC_THERMAL_MODE_MASK, code);
>>  
>> -	if ((code & AWCC_THERMAL_MODE_MASK) >= AWCC_PROFILE_LAST)
>> +	if (mode >= AWCC_PROFILE_LAST)
>>  		return false;
>>  
>> -	if ((code & AWCC_THERMAL_TABLE_MASK) == AWCC_THERMAL_TABLE_LEGACY &&
>> -	    (code & AWCC_THERMAL_MODE_MASK) >= AWCC_PROFILE_LEGACY_QUIET)
>> +	if (table == AWCC_THERMAL_TABLE_LEGACY && mode >= AWCC_PROFILE_LEGACY_QUIET)
>>  		return true;
>>  
>> -	if ((code & AWCC_THERMAL_TABLE_MASK) == AWCC_THERMAL_TABLE_USTT &&
>> -	    (code & AWCC_THERMAL_MODE_MASK) <= AWCC_PROFILE_USTT_LOW_POWER)
>> +	if (table == AWCC_THERMAL_TABLE_USTT && mode <= AWCC_PROFILE_USTT_LOW_POWER)
>>  		return true;
>>  
>>  	return false;
>> @@ -548,7 +546,7 @@ static int awcc_platform_profile_get(struct device *dev,
>>  		return 0;
>>  	}
>>  
>> -	if (!is_awcc_thermal_mode(out_data))
>> +	if (!is_awcc_thermal_profile_id(out_data))
>>  		return -ENODATA;
>>  
>>  	out_data &= AWCC_THERMAL_MODE_MASK;
>> @@ -597,6 +595,7 @@ static int awcc_platform_profile_probe(void *drvdata, unsigned long *choices)
>>  	u32 first_mode;
>>  	u32 out_data;
>>  	int ret;
>> +	u8 id;
>>  
>>  	ret = awcc_thermal_information(priv->wdev, AWCC_OP_GET_SYSTEM_DESCRIPTION,
>>  				       0, (u32 *) &sys_desc);
>> @@ -615,12 +614,14 @@ static int awcc_platform_profile_probe(void *drvdata, unsigned long *choices)
>>  		if (ret == -EBADRQC)
>>  			break;
>>  
>> -		if (!is_awcc_thermal_mode(out_data))
>> +		/* Some IDs have a BIT(8) flag that should be ignored */
>
> I find the place of this comment odd. If you want to keep it, it should be 
> next to the GENMASK() as in this place I'm trying to find the code that 
> implements what you commented but its nowhere near here.

I placed it here because the FIELD_GET bellow implicitly ignores the
flag. But I agree, I'll move it next to the GENMASK definition.

-- 
 ~ Kurt

>
>> +		id = FIELD_GET(AWCC_RESOURCE_ID_MASK, out_data);
>> +		if (!is_awcc_thermal_profile_id(id))
>>  			continue;
>>  
>> -		mode = out_data & AWCC_THERMAL_MODE_MASK;
>> +		mode = FIELD_GET(AWCC_THERMAL_MODE_MASK, id);
>>  		profile = awcc_mode_to_platform_profile[mode];
>> -		priv->supported_thermal_profiles[profile] = out_data;
>> +		priv->supported_thermal_profiles[profile] = id;
>>  
>>  		set_bit(profile, choices);
>>  	}
>>