On 16/01/2025 5:33 PM, Ilpo Järvinen wrote:
>> +static int victus_s_gpu_thermal_profile_get(bool *ctgp_enable,
>> + bool *ppab_enable,
>> + u8 *dstate,
>> + u8 *gpu_slowdown_temp)
>> +{
>> + int ret;
>> + struct victus_gpu_power_modes gpu_power_modes;
>
> You could order the function local variables into reverse xmas tree order
> (from the longest lines to the shortest). If it doesn't make sense
> somewhere due to internal dependencies, it's not a big deal to diverge
> at times (but probably doesn't happen within the context of this patch).
Fixed for v5; I didn't know about that.
I found 3 others occurrences of the same thing to fix into my work, fixed them too.
>> +/* Note: HP_POWER_LIMIT_DEFAULT can be used to restore default PL1 and PL2 */
>> +static int victus_s_set_cpu_pl1_pl2(u8 pl1, u8 pl2)
>> +{
>> + int ret;
>> + struct victus_power_limits power_limits;
>> +
>> + power_limits.pl1 = pl1;
>> + power_limits.pl2 = pl2;
>> + power_limits.pl4 = HP_POWER_LIMIT_NO_CHANGE;
>> + power_limits.cpu_gpu_concurrent_limit = HP_POWER_LIMIT_NO_CHANGE;
>> +
>> + /* We need to know both PL1 and PL2 values in order to check them */
>> + if (pl1 == HP_POWER_LIMIT_NO_CHANGE || pl2 == HP_POWER_LIMIT_NO_CHANGE)
>> + return -EINVAL;
>> +
>> + /* PL2 is not supposed to be lower than PL1 */
>> + if (pl2 < pl1)
>> + return -EINVAL;
>
> Here too these validations could be done before assignments into
> power_limits.
This makes sense, I may even have seen it sooner. Fixed for v5.
> In the meantime, I've applied the large thermal profile refactoring series
> from Kurt which will cause some conflicts here so if you could in addition
> rebase this on top of what's in the review-ilpo-next branch please.
Sure, I used the following command to get it (I hope this is the good one):
git clone -b review-ilpo-next git://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git
Then I carefully managed to understand the changes, and adapt mines on top of the new hp-wmi.c file.
I also successfully ensured the updated resulting patch still completely works on my machine (as always before sending anything). I'm now noticing having the amd_pmf driver loaded doesn't conflict anymore with the registration of the hp-wmi driver as platform profile handler, which is perfect. (As for now, changing platform profiles through amd_pmf driver alone isn't doing anything visible yet).
I'm sending the updated v5.
Best regards,
Julien ROBIN
