Re: [PATCH v5 2/2] platform/x86: hp-wmi: Add EC offsets to read Victus S thermal profile

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

On Thu, Jan 15, 2026 at 03:26:45PM +0200, Ilpo Järvinen wrote:
[snip]
>> +static int platform_profile_victus_s_get_ec(enum platform_profile_option *profile)
>> +{
>> +	int ret, i;
>> +	bool current_ctgp_state, current_ppab_state;
>> +	u8 current_dstate, current_gpu_slowdown_temp, tp;
>> +	static const u8 tp_ec_offsets[2] = { HP_OMEN_EC_THERMAL_PROFILE_OFFSET,
>> +					     HP_VICTUS_S_EC_THERMAL_PROFILE_OFFSET };
>> +
>> +	/*
>> +	 * Victus S devices have more than 1 EC layouts, hence we cannot directly
>> +	 * call omen_thermal_profile_get() like other platform_profile_*_get_ec()
>> +	 * variants, since it would only resolve to that 1 type of board. Hence
>> +	 * we iteratively query a set of candidates: tp_ec_offsets[] until we
>> +	 * find a valid thermal profile.
>> +	 */
>> +	for (i = 0 ; i < ARRAY_SIZE(tp_ec_offsets) ; i++) {
>> +		ret = ec_read(tp_ec_offsets[i], &tp);
>
>I'm not so sure about this. Reading EC offsets and hoping we find the
>correct one doesn't sound the best idea. I'd prefer we store the
>information like we already do for thermal profiles. Unless there's some
>other way to detect which layout it is?

I explored the Omen Gaming Hub (OGH) behavior on Windows to see if a WMI
query exists for readback. OGH appears to default to "Balanced" on first
run and tracks state via a profile.json file on the disk. Deleting this
file causes the app to lose the current state, suggesting that there is no
official WMI readback query. By implementing EC reads, the driver can
actually remain more consistent with the real hardware state than the
offcial software.

I agree that iterative EC reads are not ideal. However, since these two
offsets (0x95 and 0x59) cover all (or almost all) known Victus/Omen layouts,
the risk of "hoping" is low.

Storing them at compile time in the victus_s array as a part of
.driver_data is indeed the best thing. But since we do not know what EC
layout is followed by the existing boards in the array, we can take a
hybrid approach here:
1. I (and subsequent additions) will store their EC offset in the
    .driver_data field struct.
2. For already existing boards we will perform this iterative probe once
    during init, and store it somewhere common.
3. Then platform_profile_victus_s_get_ec() can simply use this "definite"
    offset to perform the EC read.

>
>FYI, I took the first patch of this series again into the review-ilpo-next
>branch as it seems uncontested and fixes a problem in the existing code.
>
>-- 
> i.
>

Thank you :)