Re: [PATCH v2] platform/x86: hp-wmi: fix platform profile values for Omen 16-wf1xxx

[Krishna Chomal <krishna.chomal108@gmail.com>]

On Tue, Dec 16, 2025 at 03:07:37PM +0200, Ilpo Järvinen wrote:
>> +enum hp_thermal_profile {
>> +	HP_THERMAL_PROFILE_PERFORMANCE			= 0x00,
>> +	HP_THERMAL_PROFILE_DEFAULT			= 0x01,
>> +	HP_THERMAL_PROFILE_COOL				= 0x02,
>> +	HP_THERMAL_PROFILE_QUIET			= 0x03,
>> +};
>> +
>> +struct thermal_profile_params {
>> +	u8 performance;
>> +	u8 balanced;
>> +	u8 eco;
>> +};
>
>How do you envision this being used on the get side? You choose not to
>change the get side, so you didn't run to that challenge. Currently
>at least platform_profile_omen_get_ec() is sort of cheating as it maps
>multiple variants without properly differentiation them.

The get side for victus_s devices as defined in platform_profile_victus_s_ops
is platform_profile_omen_get(). That function simply returns the value
stored in the global static var: active_platform_profile. As far as
platform_profile_omen_get_ec() is concerned, it being called:
1. In omen_powersource_event() and
2. In thermal_profile_setup()

However both of these are irrelevant for victus_s devices, as it is
guarded by a if(is_omen_thermal_profile()) block. So basically victus_s
devices never actually reach that function. I believe that is because it
internally calls omen_thermal_profile_get(), which itself uses ec_read
to read the current thermal profile. This functionality was added in
commit 4c51ba9af42df, back in the year 2021. Since then the EC layout,
and some WMI calls have been changed in HP laptops. In fact that is the
reason victus_s list exists in the first place.

After some basic EC-dumping from Windows, I know that the EC offset for
these newer boards should be 0x59. If you want, I can add this new EC
mapping too.

So basically for victus_s boards, at least as of now, there is no
get side to read the actual platform profile, hence no challenges faced
yet.

>Is "eco" hp specific terminology? (platform_profile_option doesn't use
>that terminology at all.)

Yes "eco" is hp-specific terminology, but you are right, I will change
it to "low_power" in V3 for consistency.

>Wouldn't we want to include struct platform_profile_ops * somewhere as
>well so this framework can extend beyond just victus_s? (I'm not sure if
>it should be in struct thermal_profile_params or if should be separate
>from that, this whole platform profile code is so hard to read it's hard
>to infer what are all the variations.)

That seems like a valid direction but given the current state of the
driver (with its many static vars), I would prefer to keep this patch
focused on fixing victus_s thermal profile values. I can look into a
more broader refactor later.

>>
>>  static int victus_s_gpu_thermal_profile_get(bool *ctgp_enable,
>> @@ -1672,25 +1708,33 @@ static int victus_s_set_cpu_pl1_pl2(u8 pl1, u8 pl2)
>>
>>  static int platform_profile_victus_s_set_ec(enum platform_profile_option profile)
>>  {
>> +	const struct dmi_system_id *id;
>> +	const struct thermal_profile_params *params;
>>  	bool gpu_ctgp_enable, gpu_ppab_enable;
>>  	u8 gpu_dstate; /* Test shows 1 = 100%, 2 = 50%, 3 = 25%, 4 = 12.5% */
>>  	int err, tp;
>>
>> +	id = dmi_first_match(victus_s_thermal_profile_boards);
>> +	if (!id)
>> +		return -ENODEV;
>> +
>> +	params = id->driver_data;
>
>We should do this once at init time. Then you can mark the dmi table(s)
>with __init.
>
>Normally the pointer would be stored into a priv struct (which this driver
>doesn't have yet but gazillions of static vars).

Yes I agree querying the dmi table for every profile change is very
suboptimal. I will add that in V3 with yet another static var :)