Re: [PATCH] asus-wmi: Add support for platform_profile

[Luke Jones <luke@ljones.dev>]

On Fri, Aug 13 2021 at 09:03:04 +0200, Hans de Goede 
<hdegoede@redhat.com> wrote:
> Hi,
> 
> On 8/13/21 7:27 AM, Luke Jones wrote:
>>  I'm unsure of how to update the existing code for fn+f5 (next 
>> thermal profile) used by laptops like the TUF series that have 
>> keyboard over i2c. I was thinking of the following:
>> 
>>  static int throttle_thermal_policy_switch_next(struct asus_wmi 
>> *asus)
>>  {
>>  struct platform_profile_handler *handler = 
>> &asus->platform_profile_handler; // added
>>  u8 new_mode = asus->throttle_thermal_policy_mode + 1;
>> 
>>  if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
>>   new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
>> 
>>  // asus->throttle_thermal_policy_mode = new_mode;
>>  // return throttle_thermal_policy_write(asus);
>>  return handler->profile_set(&asus->platform_profile_handler, 
>> new_mode); // added
>>  }
>> 
>>  * two lines added, two commented
> 
> I was going to say it is best to just send a key-press event to 
> userspace
> (we can define a new EV_KEY_.... code for this) and then let userspace
> handle things. But I see that this is currently already handled by 
> the kernel,
> so that is not really an option.
> 
>>  I'm not able to test this though, and there are very few active 
>> people in my discord with TUF/i2c laptops to ask for testing also.
>> 
>>  My concern here is to get the platform_profile correctly updated. 
>> Simply updating asus->throttle_thermal_policy_mode isn't going to 
>> achieve what we'll want.
> 
> Right, there is no need to go through handler->profile_set() you have 
> defined
> profile_set yourself after all and it does not do anything different 
> then the
> old code you are replacing here.
> 
> The trick is to call platform_profile_notify() after 
> throttle_thermal_policy_write()
> this will let userspace, e.g. power-profiles-daemon know that the 
> profile has
> been changed and it will re-read it then, resulting in a call to
> handler->profile_get()
> 
> So the new throttle_thermal_policy_switch_next() would look like this:
> 
> static int throttle_thermal_policy_switch_next(struct asus_wmi *asus)
> {
>         u8 new_mode = asus->throttle_thermal_policy_mode + 1;
> 	int err; // new
> 
>         if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
>                 new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
> 
>         asus->throttle_thermal_policy_mode = new_mode;
> 
>         err = throttle_thermal_policy_write(asus); // changed
> 	if (err == 0)                              // new
> 		platform_profile_notify();         // new
> 
> 	return err;                                // new
> }
> 
> As you can see the only new thing here is the
> platform_profile_notify() call on a successful write,
> which is such a small change that I'm not overly worried about
> not being able to test this.
> 
> I hope this helps.
> 
> Regards,
> 
> Hans
> 
> 
>>  On Fri, Aug 13 2021 at 14:42:01 +1200, Luke D. Jones 
>> <luke@ljones.dev> wrote:
>>>  Add initial support for platform_profile where the support is
>>>  based on availability of ASUS_THROTTLE_THERMAL_POLICY.
>>> 
>>>  Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>>  ---
>>>   drivers/platform/x86/asus-wmi.c | 112 
>>> ++++++++++++++++++++++++++++++++
>>>   1 file changed, 112 insertions(+)
>>> 
>>>  diff --git a/drivers/platform/x86/asus-wmi.c 
>>> b/drivers/platform/x86/asus-wmi.c
>>>  index 90a6a0d00deb..62260043c15c 100644
>>>  --- a/drivers/platform/x86/asus-wmi.c
>>>  +++ b/drivers/platform/x86/asus-wmi.c
>>>  @@ -26,6 +26,7 @@
>>>   #include <linux/rfkill.h>
>>>   #include <linux/pci.h>
>>>   #include <linux/pci_hotplug.h>
>>>  +#include <linux/platform_profile.h>
>>>   #include <linux/power_supply.h>
>>>   #include <linux/hwmon.h>
>>>   #include <linux/hwmon-sysfs.h>
>>>  @@ -219,6 +220,9 @@ struct asus_wmi {
>>>       bool throttle_thermal_policy_available;
>>>       u8 throttle_thermal_policy_mode;
>>> 
>>>  +    struct platform_profile_handler platform_profile_handler;
>>>  +    bool platform_profile_support;
>>>  +
>>>       // The RSOC controls the maximum charging percentage.
>>>       bool battery_rsoc_available;
>>> 
>>>  @@ -2144,6 +2148,106 @@ static ssize_t 
>>> throttle_thermal_policy_store(struct device *dev,
>>>   // Throttle thermal policy: 0 - default, 1 - overboost, 2 - silent
>>>   static DEVICE_ATTR_RW(throttle_thermal_policy);
>>> 
>>>  +/* Platform profile 
>>> ***********************************************************/
>>>  +static int platform_profile_get(struct platform_profile_handler 
>>> *pprof,
>>>  +                enum platform_profile_option *profile)
>>>  +{
>>>  +    struct asus_wmi *asus;
>>>  +    int tp;
>>>  +
>>>  +    asus = container_of(pprof, struct asus_wmi, 
>>> platform_profile_handler);
>>>  +
>>>  +    /* All possible toggles like throttle_thermal_policy here */
>>>  +    if (asus->throttle_thermal_policy_available) {
>>>  +        tp = asus->throttle_thermal_policy_mode;
>>>  +    } else {
>>>  +        return -1;
>>>  +    }
>>>  +
>>>  +    if (tp < 0)
>>>  +        return tp;
>>>  +
>>>  +    switch (tp) {
>>>  +    case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
>>>  +        *profile = PLATFORM_PROFILE_BALANCED;
>>>  +        break;
>>>  +    case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
>>>  +        *profile = PLATFORM_PROFILE_PERFORMANCE;
>>>  +        break;
>>>  +    case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
>>>  +        *profile = PLATFORM_PROFILE_QUIET;
>>>  +        break;
>>>  +    default:
>>>  +        return -EINVAL;
>>>  +    }
>>>  +
>>>  +    return 0;
>>>  +}
>>>  +
>>>  +static int platform_profile_set(struct platform_profile_handler 
>>> *pprof,
>>>  +                enum platform_profile_option profile)
>>>  +{
>>>  +    struct asus_wmi *asus;
>>>  +    int tp;
>>>  +
>>>  +    asus = container_of(pprof, struct asus_wmi, 
>>> platform_profile_handler);
>>>  +
>>>  +    /* All possible toggles like throttle_thermal_policy here */
>>>  +    if (!asus->throttle_thermal_policy_available) {
>>>  +        return -1;
>>>  +    }
>>>  +
>>>  +    switch (profile) {
>>>  +    case PLATFORM_PROFILE_PERFORMANCE:
>>>  +        tp = ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST;
>>>  +        break;
>>>  +    case PLATFORM_PROFILE_BALANCED:
>>>  +        tp = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
>>>  +        break;
>>>  +    case PLATFORM_PROFILE_QUIET:
>>>  +        tp = ASUS_THROTTLE_THERMAL_POLICY_SILENT;
>>>  +        break;
>>>  +    default:
>>>  +        return -EOPNOTSUPP;
>>>  +    }
>>>  +
>>>  +    if (asus->throttle_thermal_policy_available) {
>>>  +        asus->throttle_thermal_policy_mode = tp;
>>>  +        return throttle_thermal_policy_write(asus);
>>>  +    }
>>>  +    return 0;
>>>  +}
>>>  +
>>>  +static int platform_profile_setup(struct asus_wmi *asus)
>>>  +{
>>>  +    int err;
>>>  +
>>>  +    if (asus->throttle_thermal_policy_available) {
>>>  +        pr_info("Using throttle_thermal_policy for 
>>> platform_profile support\n");
>>>  +    } else {
>>>  +        /*
>>>  +         * Not an error if a component platform_profile relies on 
>>> is unavailable
>>>  +         * so early return, skipping the setup of 
>>> platform_profile.
>>>  +        */
>>>  +        return 0;
>>>  +    }
>>>  +    asus->platform_profile_handler.profile_get = 
>>> platform_profile_get;
>>>  +    asus->platform_profile_handler.profile_set = 
>>> platform_profile_set;
>>>  +
>>>  +    set_bit(PLATFORM_PROFILE_QUIET, 
>>> asus->platform_profile_handler.choices);
>>>  +    set_bit(PLATFORM_PROFILE_BALANCED,
>>>  +        asus->platform_profile_handler.choices);
>>>  +    set_bit(PLATFORM_PROFILE_PERFORMANCE,
>>>  +        asus->platform_profile_handler.choices);
>>>  +
>>>  +    err = 
>>> platform_profile_register(&asus->platform_profile_handler);
>>>  +    if (err)
>>>  +        return err;
>>>  +
>>>  +    asus->platform_profile_support = true;
>>>  +    return 0;
>>>  +}
>>>  +
>>>   /* Backlight 
>>> ******************************************************************/
>>> 
>>>   static int read_backlight_power(struct asus_wmi *asus)
>>>  @@ -2904,6 +3008,10 @@ static int asus_wmi_add(struct 
>>> platform_device *pdev)
>>>       else
>>>           throttle_thermal_policy_set_default(asus);
>>> 
>>>  +    err = platform_profile_setup(asus);
>>>  +    if (err)
>>>  +        goto fail_platform_profile_setup;
>>>  +
>>>       err = panel_od_check_present(asus);
>>>       if (err)
>>>           goto fail_panel_od;
>>>  @@ -2993,6 +3101,7 @@ static int asus_wmi_add(struct 
>>> platform_device *pdev)
>>>       asus_wmi_sysfs_exit(asus->platform_device);
>>>   fail_sysfs:
>>>   fail_throttle_thermal_policy:
>>>  +fail_platform_profile_setup:
>>>   fail_fan_boost_mode:
>>>   fail_egpu_enable:
>>>   fail_dgpu_disable:
>>>  @@ -3017,6 +3126,9 @@ static int asus_wmi_remove(struct 
>>> platform_device *device)
>>>       asus_fan_set_auto(asus);
>>>       asus_wmi_battery_exit(asus);
>>> 
>>>  +    if (asus->platform_profile_support)
>>>  +        platform_profile_remove();
>>>  +
>>>       kfree(asus);
>>>       return 0;
>>>   }
>>>  --
>>>  2.31.1
>>> 
>> 
>> 
> 

Hi Hans,

Very helpful, thanks. I'd completely failed to notice 
platform_profile_notify in the platform_profile.h :| I've now put that 
in throttle_thermal_policy_write() just after sysfs_notify().

I will submit new version after completing other feedback.

Many Thanks,
Luke.