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

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

[Mohamed Ghanmi]

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

Changelog:
- V1
  - fixes grammar in description
  - reorders macros
  - adds throttle_thermal_policy_max_mode() helper function
- V2
  - removes unnecessary braces in throttle_thermal_policy_max_mode()
- V3
  - fixes a use before declaration compilation error caused by reordering of local
    variables in throttle_thermal_policy_store() function

 drivers/platform/x86/asus-wmi.c            | 93 ++++++++++++----------
 include/linux/platform_data/x86/asus-wmi.h |  1 +
 2 files changed, 51 insertions(+), 43 deletions(-)

-- 
2.44.0

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

[Mohamed Ghanmi]

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@supcom.tn>
Co-developed-by: Luke D. Jones <luke@ljones.dev>
Signed-off-by: Luke D. Jones <luke@ljones.dev>
Reviewed-by: Ilpo Järvinen <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(-)

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
+ */
 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
-- 
2.44.0

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

[Hans de Goede]

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@supcom.tn>
> Co-developed-by: Luke D. Jones <luke@ljones.dev>
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> Reviewed-by: Ilpo Järvinen <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(-)
> 
> 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.

Regards,

Hans




>  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

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

[Luke Jones]

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@supcom.tn>
> > Co-developed-by: Luke D. Jones <luke@ljones.dev>
> > Signed-off-by: Luke D. Jones <luke@ljones.dev>
> > Reviewed-by: Ilpo Järvinen <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(-)
> > 
> > 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
> 
> 

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3172 bytes --]

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@supcom.tn>
> > > Co-developed-by: Luke D. Jones <luke@ljones.dev>
> > > Signed-off-by: Luke D. Jones <luke@ljones.dev>
> > > Reviewed-by: Ilpo Järvinen <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(-)
> > > 
> > > 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.

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

[Luke Jones]

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@supcom.tn>
> > > > Co-developed-by: Luke D. Jones <luke@ljones.dev>
> > > > Signed-off-by: Luke D. Jones <luke@ljones.dev>
> > > > Reviewed-by: Ilpo Järvinen <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(-)
> > > > 
> > > > 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.

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

[Devin Bayer]

Hello Mohamed et al,

On 05/06/2024 09.43, Luke Jones wrote:
> 
> 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).

I recently created my own patch to toggle this control[^1]. I thought
it was an alternative fan_boost_mode because the dev_id's were one apart. But with
more testing I've realized it also controls the GPU power and supporting the standard
platform_profile is better, so I prefer this patch.

I tested it and it works. However, it has a couple issues:

1. This dev_id isn't Vivobook specific. My Zenbook UX3404VC (2023) has this control.

2. The Zenbook only supports values 0-2 (standard, quiet and performance).
   Calling the method with 3 causes the KEYBOARD_KEY event to fire instead of
   adjusting the GPU power and fan speed.

So I would suggest to remove the fullspeed setting and rename the constants,
perhaps to ASUS_THROTTLE_THERMAL_POLICY2.

Mohamed, if you are busy I could also create a new version of this patch with
the requested fixes.

I wonder if the existing fan_boost_mode should also be considered a platform_profile?

~ Dev

[^1]: https://lore.kernel.org/platform-driver-x86/20240620082223.20178-1-dev@doubly.so/

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

[Luke Jones]

On Fri, 21 Jun 2024, at 9:43 PM, Devin Bayer wrote:
> Hello Mohamed et al,
> 
> On 05/06/2024 09.43, Luke Jones wrote:
> > 
> > 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).
> 
> I recently created my own patch to toggle this control[^1]. I thought
> it was an alternative fan_boost_mode because the dev_id's were one apart. But with
> more testing I've realized it also controls the GPU power and supporting the standard
> platform_profile is better, so I prefer this patch.
> 
> I tested it and it works. However, it has a couple issues:
> 
> 1. This dev_id isn't Vivobook specific. My Zenbook UX3404VC (2023) has this control.

I'm not sure what else to call it. "thermal_throttle_alt" or otherwise, I don't know. The intention should be clear even if only in a comment.

> 2. The Zenbook only supports values 0-2 (standard, quiet and performance).
>    Calling the method with 3 causes the KEYBOARD_KEY event to fire instead of
>    adjusting the GPU power and fan speed.

I linked you to v4 of this patch in one of my responses. The link again is https://lore.kernel.org/platform-driver-x86/20240609144849.2532-1-mohamed.ghanmi@supcom.tn/T/#mcd18e74676084e21d5c15af84bc08d8c6b375fb9

> So I would suggest to remove the fullspeed setting and rename the constants,
> perhaps to ASUS_THROTTLE_THERMAL_POLICY2.
> 
> Mohamed, if you are busy I could also create a new version of this patch with
> the requested fixes.

As mentioned it is done in v4.

> 
> I wonder if the existing fan_boost_mode should also be considered a platform_profile?

No. It tends to be only fans, and usually fullspeed. Platform_profile is intended to control platform related variables. I would also be curious if you tested without the PPD (daemon) as recent versions may also control the energy performance preference and that will skew your results.

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

[Devin Bayer]

On 21/06/2024 12.26, Luke Jones wrote:
> 
>> I tested it and it works. However, it has a couple issues:
>>
>> 1. This dev_id isn't Vivobook specific. My Zenbook UX3404VC (2023) has this control.
> 
> I'm not sure what else to call it. "thermal_throttle_alt" or otherwise, I don't know. The intention should be clear even if only in a comment.

Okay, I thought Vivo might be little confusing and the "2" suffix would
match the ASUS_WMI_DEVID_MINI_LED_MODE2 convention.

>> 2. The Zenbook only supports values 0-2 (standard, quiet and performance).
>>    Calling the method with 3 causes the KEYBOARD_KEY event to fire instead of
>>    adjusting the GPU power and fan speed.
> 
> I linked you to v4 of this patch in one of my responses. The link again is https://lore.kernel.org/platform-driver-x86/20240609144849.2532-1-mohamed.ghanmi@supcom.tn/T/#mcd18e74676084e21d5c15af84bc08d8c6b375fb9

Ah, sorry. I see v4 of the patch is working fine.

>> I wonder if the existing fan_boost_mode should also be considered a platform_profile?
> 
> No. It tends to be only fans, and usually fullspeed. Platform_profile is intended to control platform related variables. I would also be curious if you tested without the PPD (daemon) as recent versions may also control the energy performance preference and that will skew your results.

I don't use the PPD so tested without it.

BTW, FAN_BOOST_MODE doesn't have a FULLSPEED constant, so maybe you were
thinking of FAN_CTRL.

I don't have a 0x00110018 device so I can't test that but just FYI, the
reason I thought the 0x00110019 device as related are the similar
dev_ids and it calls the ACPI method name FANL to write the EC variable
QFAN.

~ Dev