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

["Luke Jones" <luke@ljones.dev>]

On Mon, 3 Jun 2024, at 10:29 PM, Ilpo Järvinen wrote:
> On Mon, 3 Jun 2024, Luke Jones wrote:
> > On Mon, 29 Apr 2024, at 10:20 PM, Hans de Goede wrote:
> > > On 4/21/24 9:43 PM, Mohamed Ghanmi wrote:
> > > > Add support for vivobook fan profiles wmi call on the ASUS VIVOBOOK
> > > > to adjust power limits.
> > > > 
> > > > These fan profiles have a different device id than the ROG series
> > > > and different order. This reorders the existing modes and adds a new
> > > > full speed mode available on these laptops.
> > > > 
> > > > As part of keeping the patch clean the throttle_thermal_policy_available
> > > > boolean stored in the driver struct is removed and
> > > > throttle_thermal_policy_dev is used in place (as on init it is zeroed).
> > > > 
> > > > Signed-off-by: Mohamed Ghanmi <mohamed.ghanmi@supcom.tn>
> > > > Co-developed-by: Luke D. Jones <luke@ljones.dev>
> > > > Signed-off-by: Luke D. Jones <luke@ljones.dev>
> > > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > ---
> > > >  drivers/platform/x86/asus-wmi.c            | 93 ++++++++++++----------
> > > >  include/linux/platform_data/x86/asus-wmi.h |  1 +
> > > >  2 files changed, 51 insertions(+), 43 deletions(-)
> > > > 
> > > > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> > > > index 3c61d75a3..1f54596ca 100644
> > > > --- a/drivers/platform/x86/asus-wmi.c
> > > > +++ b/drivers/platform/x86/asus-wmi.c
> 
> > > > @@ -3747,7 +3753,10 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
> > > >  return count;
> > > >  }
> > > >  
> > > > -// Throttle thermal policy: 0 - default, 1 - overboost, 2 - silent
> > > > +/*
> > > > + * Throttle thermal policy: 0 - default, 1 - overboost, 2 - silent
> > > > + * Throttle thermal policy vivobook : 0 - default, 1 - silent, 2 - overboost, 3 - fullspeed
> > > > + */
> > > 
> > > throttle_thermal_policy_write() always expects normal (non vivobook) values and
> > > then translates those to vivo values, so this comment is not correct.
> > > 
> > > The only difference is that vivobook also has fullspeed, but the way userspace
> > > sees it 1/2 or silent/overspeed are not swapped, since the swapping is taking
> > > care of in throttle_thermal_policy_write().
> > > 
> > > Also the new fullspeed is not exported through the platform_profile interface,
> > > for setting values this is somewhat ok, but fullspeed can be set through
> > > sysfs, and this will then cause asus_wmi_platform_profile_get() to fail
> > > with -EINVAL, so this need to be fixed. Either map fullspeed to
> > > PLATFORM_PROFILE_PERFORMANCE in asus_wmi_platform_profile_get(), or add
> > > a new platform_profile value for this.
> > >
> > 
> > I would much prefer if "fullspeed" was not included at all unless it was 
> > an individual setting. It very rarely contributes anything good to the 
> > driver, and most certainly won't be of value in the platform_profile.
> > 
> > Otherwise, what is the status on this? 
> 
> Hi,
> 
> I was expecting an update that addresses Hans' review comment.
> 
> Luke, are you arguing that his comment is not valid and v3 is fine?
> 
> (In any case, I've summoned v3 back from archive into active patches in 
> patchwork so this doesn't get forgotten if v4 is not needed).
> 
> -- 
> i.

Hi.

I am saying I would like to see ASUS_THROTTLE_THERMAL_POLICY_FULLSPEED removed, or placed somewhere else such as in <sysfs>/asus-nb-wmi/hwmon/hwmon3/pwm1_enable.

It certainly shouldn't be included in platform_profile and I'm not keen on seeing it in thorttle_thermal_policy either.

A lot of this reasoning is now coming from the refactor I've just done of asus-wmi and the "features" such as this one to place them under firmware_attributes class and begin deprecation of them in asus_wmi. From what I've achieved so far with it it is much *much* more suitable than this ad-hoc style of adding features I've been doing here (I'll submit this work soon, it repalces the last patch series I did).

In light of the above I'm considering the possibility of removing throttle_thermal_policy completely to wholly favour the use of platform_profile. It doesn't make all that much sense to have two different things manipulating the same thing - and as such I don't think the "full speed fan" setting fits at all with platform_profile as it is not a performance tweak.

Mohamed, I would be happy to include the Vivo support so long as:
1. the fullspeed setting is removed
2. the modes map correctly to platform_profile

I hope this makes sense. Very sorry about the previous brief response (I was recovering from surgery trauma).

Cheers,
Luke.