Received-SPF: pass (google.com: domain of linux-kernel+bounces-198975-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Mon, 3 Jun 2024 13:29:21 +0300 (EEST)
To: Luke Jones <luke@ljones.dev>
cc: Hans de Goede <hdegoede@redhat.com>, 
    Mohamed Ghanmi <mohamed.ghanmi@supcom.tn>, corentin.chary@gmail.com, 
    platform-driver-x86@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 1/1] platform/x86: asus-wmi: add support for vivobook
 fan profiles
In-Reply-To: <aee09e9f-6269-43ef-b509-a9a7b5e1752f@app.fastmail.com>
Message-ID: <f126562f-54c8-de58-3f98-7375c129f66a@linux.intel.com>
References: <20240421194320.48258-1-mohamed.ghanmi@supcom.tn> <20240421194320.48258-2-mohamed.ghanmi@supcom.tn> <de8fcb82-3e08-41e6-b099-75df27c6df23@redhat.com> <aee09e9f-6269-43ef-b509-a9a7b5e1752f@app.fastmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="8323328-2015242583-1717410561=:1529"

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323328-2015242583-1717410561=:1529
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: QUOTED-PRINTABLE

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.
> > >=20
> > > 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.
> > >=20
> > > As part of keeping the patch clean the throttle_thermal_policy_availa=
ble
> > > boolean stored in the driver struct is removed and
> > > throttle_thermal_policy_dev is used in place (as on init it is zeroed=
).
> > >=20
> > > 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=C3=A4rvinen <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(-)
> > >=20
> > > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/a=
sus-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(s=
truct device *dev,
> > >  return count;
> > >  }
> > > =20
> > > -// 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 - o=
verboost, 3 - fullspeed
> > > + */
> >=20
> > throttle_thermal_policy_write() always expects normal (non vivobook) va=
lues and
> > then translates those to vivo values, so this comment is not correct.
> >=20
> > The only difference is that vivobook also has fullspeed, but the way us=
erspace
> > sees it 1/2 or silent/overspeed are not swapped, since the swapping is =
taking
> > care of in throttle_thermal_policy_write().
> >=20
> > Also the new fullspeed is not exported through the platform_profile int=
erface,
> > for setting values this is somewhat ok, but fullspeed can be set throug=
h
> > 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.
> >
>=20
> I would much prefer if "fullspeed" was not included at all unless it was=
=20
> an individual setting. It very rarely contributes anything good to the=20
> driver, and most certainly won't be of value in the platform_profile.
>=20
> Otherwise, what is the status on this?=20

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=20
patchwork so this doesn't get forgotten if v4 is not needed).

--=20
 i.
--8323328-2015242583-1717410561=:1529--