Re: [PATCH v4 1/1] platform/x86: asus-wmi: add support for vivobook fan profiles

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 7/31/24 11:06 PM, Luke Jones wrote:

...

>>>>> If Hans and Ilpo don't have any requirements then:
>>>>>
>>>>> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> 
> I have no issues with this patch and it needs to be merged before I submit a stream of work. My signed-off tag is above.

Reading back the comments about the function names I believe that
the asus_wmi_platform_profile_mode_[to|from]_vivo() names are fine.

And the rest of the patch looks good to me to:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

I have merged this into my review-hans branch now with one small change,
both asus_wmi_platform_profile_mode_[to|from]_vivo() ended in:

       return mode;

}

I have dropped the empty line after the return mode; statement
(in both functions) while merging this.

I've also dropped a spurious whitespace change (extra empty line)
added to asus_wmi_platform_profile_get().

Luke, please base your coming work on top of pdx86/review-hans.

Regards,

Hans


p.s.

One thing which I noticed while reviewing is this:

static int fan_curve_get_factory_default(struct asus_wmi *asus, u32 fan_dev)
{
        struct fan_curve_data *curves;
        u8 buf[FAN_CURVE_BUF_LEN];
        int err, fan_idx;
        u8 mode = 0;

        if (asus->throttle_thermal_policy_dev)
                mode = asus->throttle_thermal_policy_mode;
        /* DEVID_<C/G>PU_FAN_CURVE is switched for OVERBOOST vs SILENT */
        if (mode == 2)
                mode = 1;
        else if (mode == 1)
                mode = 2;

        err = asus_wmi_evaluate_method_buf(asus->dsts_id, fan_dev, mode, buf,
                                           FAN_CURVE_BUF_LEN);


Since asus->throttle_thermal_policy_mode stores the raw mode, which has
silent/overboost swapped for vivo notebooks I wonder if we should still
do the mode 1/2 swapping here when on a vivo notebook ?

I have a feeling the swapping here should not be done when one a vivo
notebook but I'm not sure.

If the swapping here should be skipped on Vivobooks please submit
a separate follow-up patch for that.

Regards,

Hans