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@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. 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.
