Message-Id: <aee09e9f-6269-43ef-b509-a9a7b5e1752f@app.fastmail.com>
Date: Mon, 03 Jun 2024 17:19:03 +1200
From: "Luke Jones" <luke@...nes.dev>
To: "Hans de Goede" <hdegoede@...hat.com>,
 "Mohamed Ghanmi" <mohamed.ghanmi@...com.tn>
Cc: corentin.chary@...il.com,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/1] platform/x86: asus-wmi: add support for vivobook fan
 profiles

On Mon, 29 Apr 2024, at 10:20 PM, Hans de Goede wrote:
> Hi,
> 
> 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@...com.tn>
> > Co-developed-by: Luke D. Jones <luke@...nes.dev>
> > Signed-off-by: Luke D. Jones <luke@...nes.dev>
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@...ux.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
> > @@ -97,6 +97,11 @@ module_param(fnlock_default, bool, 0444);
> >  #define ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST 1
> >  #define ASUS_THROTTLE_THERMAL_POLICY_SILENT 2
> >  
> > +#define ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO 0
> > +#define ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO 1
> > +#define ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO 2
> > +#define ASUS_THROTTLE_THERMAL_POLICY_FULLSPEED 3
> > +
> >  #define USB_INTEL_XUSB2PR 0xD0
> >  #define PCI_DEVICE_ID_INTEL_LYNXPOINT_LP_XHCI 0x9c31
> >  
> > @@ -293,8 +298,8 @@ struct asus_wmi {
> >  u32 kbd_rgb_dev;
> >  bool kbd_rgb_state_available;
> >  
> > - bool throttle_thermal_policy_available;
> >  u8 throttle_thermal_policy_mode;
> > + u32 throttle_thermal_policy_dev;
> >  
> >  bool cpu_fan_curve_available;
> >  bool gpu_fan_curve_available;
> > @@ -3152,7 +3157,7 @@ static int fan_curve_get_factory_default(struct asus_wmi *asus, u32 fan_dev)
> >  int err, fan_idx;
> >  u8 mode = 0;
> >  
> > - if (asus->throttle_thermal_policy_available)
> > + if (asus->throttle_thermal_policy_dev)
> >  mode = asus->throttle_thermal_policy_mode;
> >  /* DEVID_<C/G>PU_FAN_CURVE is switched for OVERBOOST vs SILENT */
> >  if (mode == 2)
> > @@ -3359,7 +3364,7 @@ static ssize_t fan_curve_enable_store(struct device *dev,
> >  * For machines with throttle this is the only way to reset fans
> >  * to default mode of operation (does not erase curve data).
> >  */
> > - if (asus->throttle_thermal_policy_available) {
> > + if (asus->throttle_thermal_policy_dev) {
> >  err = throttle_thermal_policy_write(asus);
> >  if (err)
> >  return err;
> > @@ -3576,8 +3581,8 @@ static const struct attribute_group asus_fan_curve_attr_group = {
> >  __ATTRIBUTE_GROUPS(asus_fan_curve_attr);
> >  
> >  /*
> > - * Must be initialised after throttle_thermal_policy_check_present() as
> > - * we check the status of throttle_thermal_policy_available during init.
> > + * Must be initialised after throttle_thermal_policy_dev is set as
> > + * we check the status of throttle_thermal_policy_dev during init.
> >   */
> >  static int asus_wmi_custom_fan_curve_init(struct asus_wmi *asus)
> >  {
> > @@ -3618,38 +3623,37 @@ static int asus_wmi_custom_fan_curve_init(struct asus_wmi *asus)
> >  }
> >  
> >  /* Throttle thermal policy ****************************************************/
> > -
> > -static int throttle_thermal_policy_check_present(struct asus_wmi *asus)
> > +static u8 throttle_thermal_policy_max_mode(struct asus_wmi *asus)
> >  {
> > - u32 result;
> > - int err;
> > -
> > - asus->throttle_thermal_policy_available = false;
> > -
> > - err = asus_wmi_get_devstate(asus,
> > -     ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY,
> > -     &result);
> > - if (err) {
> > - if (err == -ENODEV)
> > - return 0;
> > - return err;
> > - }
> > -
> > - if (result & ASUS_WMI_DSTS_PRESENCE_BIT)
> > - asus->throttle_thermal_policy_available = true;
> > -
> > - return 0;
> > + if (asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO)
> > + return ASUS_THROTTLE_THERMAL_POLICY_FULLSPEED;
> > + else
> > + return ASUS_THROTTLE_THERMAL_POLICY_SILENT;
> >  }
> >  
> >  static int throttle_thermal_policy_write(struct asus_wmi *asus)
> >  {
> > - int err;
> > - u8 value;
> > + u8 value = asus->throttle_thermal_policy_mode;
> >  u32 retval;
> > + bool vivo;
> > + int err;
> >  
> > - value = asus->throttle_thermal_policy_mode;
> > + vivo = asus->throttle_thermal_policy_dev == ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO;
> > + if (vivo) {
> > + switch (value) {
> > + case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
> > + value = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT_VIVO;
> > + break;
> > + case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
> > + value = ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST_VIVO;
> > + break;
> > + case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
> > + value = ASUS_THROTTLE_THERMAL_POLICY_SILENT_VIVO;
> > + break;
> > + }
> > + }
> >  
> > - err = asus_wmi_set_devstate(ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY,
> > + err = asus_wmi_set_devstate(asus->throttle_thermal_policy_dev,
> >      value, &retval);
> >  
> >  sysfs_notify(&asus->platform_device->dev.kobj, NULL,
> > @@ -3679,7 +3683,7 @@ static int throttle_thermal_policy_write(struct asus_wmi *asus)
> >  
> >  static int throttle_thermal_policy_set_default(struct asus_wmi *asus)
> >  {
> > - if (!asus->throttle_thermal_policy_available)
> > + if (!asus->throttle_thermal_policy_dev)
> >  return 0;
> >  
> >  asus->throttle_thermal_policy_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
> > @@ -3689,9 +3693,10 @@ static int throttle_thermal_policy_set_default(struct asus_wmi *asus)
> >  static int throttle_thermal_policy_switch_next(struct asus_wmi *asus)
> >  {
> >  u8 new_mode = asus->throttle_thermal_policy_mode + 1;
> > + u8 max_mode = throttle_thermal_policy_max_mode(asus);
> >  int err;
> >  
> > - if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
> > + if (new_mode > max_mode)
> >  new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
> >  
> >  asus->throttle_thermal_policy_mode = new_mode;
> > @@ -3722,6 +3727,7 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
> >      const char *buf, size_t count)
> >  {
> >  struct asus_wmi *asus = dev_get_drvdata(dev);
> > + u8 max_mode = throttle_thermal_policy_max_mode(asus);
> >  u8 new_mode;
> >  int result;
> >  int err;
> > @@ -3730,7 +3736,7 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
> >  if (result < 0)
> >  return result;
> >  
> > - if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
> > + if (new_mode > max_mode)
> >  return -EINVAL;
> >  
> >  asus->throttle_thermal_policy_mode = new_mode;
> > @@ -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? 

> 
> >  static DEVICE_ATTR_RW(throttle_thermal_policy);
> >  
> >  /* Platform profile ***********************************************************/
> > @@ -3813,7 +3822,7 @@ static int platform_profile_setup(struct asus_wmi *asus)
> >  * Not an error if a component platform_profile relies on is unavailable
> >  * so early return, skipping the setup of platform_profile.
> >  */
> > - if (!asus->throttle_thermal_policy_available)
> > + if (!asus->throttle_thermal_policy_dev)
> >  return 0;
> >  
> >  dev_info(dev, "Using throttle_thermal_policy for platform_profile support\n");
> > @@ -4228,7 +4237,7 @@ static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
> >  if (code == NOTIFY_KBD_FBM || code == NOTIFY_KBD_TTP) {
> >  if (asus->fan_boost_mode_available)
> >  fan_boost_mode_switch_next(asus);
> > - if (asus->throttle_thermal_policy_available)
> > + if (asus->throttle_thermal_policy_dev)
> >  throttle_thermal_policy_switch_next(asus);
> >  return;
> >  
> > @@ -4436,7 +4445,7 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
> >  else if (attr == &dev_attr_fan_boost_mode.attr)
> >  ok = asus->fan_boost_mode_available;
> >  else if (attr == &dev_attr_throttle_thermal_policy.attr)
> > - ok = asus->throttle_thermal_policy_available;
> > + ok = asus->throttle_thermal_policy_dev != 0;
> >  else if (attr == &dev_attr_ppt_pl2_sppt.attr)
> >  devid = ASUS_WMI_DEVID_PPT_PL2_SPPT;
> >  else if (attr == &dev_attr_ppt_pl1_spl.attr)
> > @@ -4745,16 +4754,15 @@ static int asus_wmi_add(struct platform_device *pdev)
> >  else if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_MODE2))
> >  asus->kbd_rgb_dev = ASUS_WMI_DEVID_TUF_RGB_MODE2;
> >  
> > + if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY))
> > + asus->throttle_thermal_policy_dev = ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY;
> > + else if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO))
> > + asus->throttle_thermal_policy_dev = ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO;
> > +
> >  err = fan_boost_mode_check_present(asus);
> >  if (err)
> >  goto fail_fan_boost_mode;
> >  
> > - err = throttle_thermal_policy_check_present(asus);
> > - if (err)
> > - goto fail_throttle_thermal_policy;
> > - else
> > - throttle_thermal_policy_set_default(asus);
> > -
> >  err = platform_profile_setup(asus);
> >  if (err)
> >  goto fail_platform_profile_setup;
> > @@ -4849,7 +4857,6 @@ static int asus_wmi_add(struct platform_device *pdev)
> >  fail_input:
> >  asus_wmi_sysfs_exit(asus->platform_device);
> >  fail_sysfs:
> > -fail_throttle_thermal_policy:
> >  fail_custom_fan_curve:
> >  fail_platform_profile_setup:
> >  if (asus->platform_profile_support)
> > diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> > index 3eb5cd677..982a63774 100644
> > --- a/include/linux/platform_data/x86/asus-wmi.h
> > +++ b/include/linux/platform_data/x86/asus-wmi.h
> > @@ -64,6 +64,7 @@
> >  #define ASUS_WMI_DEVID_SCREENPAD_LIGHT 0x00050032
> >  #define ASUS_WMI_DEVID_FAN_BOOST_MODE 0x00110018
> >  #define ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY 0x00120075
> > +#define ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO 0x00110019
> >  
> >  /* Misc */
> >  #define ASUS_WMI_DEVID_PANEL_OD 0x00050019
> 
> 

Powered by blists - more mailing lists