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

From: Kurt Borja
Date: Fri Mar 28 2025 - 17:10:51 EST


On Fri Mar 28, 2025 at 11:17 AM -03, Ilpo Järvinen wrote:
> On Thu, 13 Mar 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().
>>
>> Reviewed-by: Armin Wolf <W_Armin@xxxxxx>
>> Signed-off-by: Kurt Borja <kuurtb@xxxxxxxxx>
>> ---
>> drivers/platform/x86/dell/alienware-wmi-wmax.c | 33 +++++++++++++-------------
>> 1 file changed, 17 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell/alienware-wmi-wmax.c b/drivers/platform/x86/dell/alienware-wmi-wmax.c
>> index ed70e12d73d7fe5d89f3364c5367820bf47e3c1e..80aefba5b22d6b4ac18aeb2ca356f8c911150abd 100644
>> --- a/drivers/platform/x86/dell/alienware-wmi-wmax.c
>> +++ b/drivers/platform/x86/dell/alienware-wmi-wmax.c
>> @@ -34,7 +34,8 @@
>> #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)
>
> I'd prefer leaving this in place even if it now becomes unused, or
> actually I suggest you move it before the GENMASKs to keep the values in
> order, and leave an empty row between failure code and these id related
> masks.

Sure, I'll move it to the top.

>
>> +/* Some IDs have a BIT(8) flag that we ignore */
>> +#define AWCC_RESOURCE_ID_MASK GENMASK(7, 0)
>
> (AWCC_THERMAL_TABLE_MASK | AWCC_THERMAL_MODE_MASK) ?

I prefer not to go for this because AWCC_RESOURCE_ID_MASK will be used
for all IDs in the following patches, not just thermal ones.

I'm doing this because I found that Alienware Aurora desktops also used
the BIT(8) flag for thermal profile IDs, not just sensors.

>
>> static bool force_platform_profile;
>> module_param_unsafe(force_platform_profile, bool, 0);
>> @@ -168,8 +169,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 +446,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)
>> + u8 table = FIELD_GET(AWCC_THERMAL_TABLE_MASK, code);
>> + u8 mode = FIELD_GET(AWCC_THERMAL_MODE_MASK, code);
>> +
>> + if (mode >= AWCC_PROFILE_LAST)
>> return false;
>>
>> - if ((code & AWCC_THERMAL_MODE_MASK) >= 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 +547,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 +596,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 +615,13 @@ static int awcc_platform_profile_probe(void *drvdata, unsigned long *choices)
>> if (ret == -EBADRQC)
>> break;
>>
>> - if (!is_awcc_thermal_mode(out_data))
>> + 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);
>> }
>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>

Thanks!

--
~ Kurt