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

[Ilpo Järvinen]

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@xxxxxxxxx>
> > > Co-developed-by: Luke D. Jones <luke@xxxxxxxxxx>
> > > Signed-off-by: Luke D. Jones <luke@xxxxxxxxxx>
> > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> > > ---
> > >  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.