Received-SPF: pass (google.com: domain of linux-kernel+bounces-201894-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Feedback-ID: i5ec1447f:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-497-g97f96844c-fm-20240526.001-g97f96844
Precedence: bulk
MIME-Version: 1.0
Message-Id: <4de768c5-aae5-4fda-a139-a8b73c8495a1@app.fastmail.com>
In-Reply-To: <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>
 <f126562f-54c8-de58-3f98-7375c129f66a@linux.intel.com>
Date: Wed, 05 Jun 2024 19:43:08 +1200
From: "Luke Jones" <luke@ljones.dev>
To: =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
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
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable


On Mon, 3 Jun 2024, at 10:29 PM, Ilpo J=C3=A4rvinen 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 VIVOB=
OOK
> > > > 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_a=
vailable
> > > > boolean stored in the driver struct is removed and
> > > > throttle_thermal_policy_dev is used in place (as on init it is z=
eroed).
> > > >=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/asus-wmi.c
> > > > index 3c61d75a3..1f54596ca 100644
> > > > --- a/drivers/platform/x86/asus-wmi.c
> > > > +++ b/drivers/platform/x86/asus-wmi.c
>=20
> > > > @@ -3747,7 +3753,10 @@ static ssize_t throttle_thermal_policy_st=
ore(struct device *dev,
> > > >  return count;
> > > >  }
> > > > =20
> > > > -// Throttle thermal policy: 0 - default, 1 - overboost, 2 - sil=
ent
> > > > +/*
> > > > + * Throttle thermal policy: 0 - default, 1 - overboost, 2 - sil=
ent
> > > > + * Throttle thermal policy vivobook : 0 - default, 1 - silent, =
2 - overboost, 3 - fullspeed
> > > > + */
> > >=20
> > > throttle_thermal_policy_write() always expects normal (non vivoboo=
k) values and
> > > then translates those to vivo values, so this comment is not corre=
ct.
> > >=20
> > > The only difference is that vivobook also has fullspeed, but the w=
ay userspace
> > > sees it 1/2 or silent/overspeed are not swapped, since the swappin=
g is taking
> > > care of in throttle_thermal_policy_write().
> > >=20
> > > Also the new fullspeed is not exported through the platform_profil=
e interface,
> > > for setting values this is somewhat ok, but fullspeed can be set t=
hrough
> > > 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(), o=
r 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 t=
he=20
> > driver, and most certainly won't be of value in the platform_profile.
> >=20
> > Otherwise, what is the status on this?=20
>=20
> Hi,
>=20
> I was expecting an update that addresses Hans' review comment.
>=20
> Luke, are you arguing that his comment is not valid and v3 is fine?
>=20
> (In any case, I've summoned v3 back from archive into active patches i=
n=20
> patchwork so this doesn't get forgotten if v4 is not needed).
>=20
> --=20
> i.

Hi.

I am saying I would like to see ASUS_THROTTLE_THERMAL_POLICY_FULLSPEED r=
emoved, or placed somewhere else such as in <sysfs>/asus-nb-wmi/hwmon/hw=
mon3/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 o=
f asus-wmi and the "features" such as this one to place them under firmw=
are_attributes class and begin deprecation of them in asus_wmi. From wha=
t 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 thrott=
le_thermal_policy completely to wholly favour the use of platform_profil=
e. It doesn't make all that much sense to have two different things mani=
pulating 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 tw=
eak.

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.