[PATCH v2 0/9] asus-wmi: add new features, clean up, fixes

[PATCH v2 0/9] asus-wmi: add new features, clean up, fixes

[Luke D. Jones]

My appologies for the rapid v2. I hadn't run checkpatch like I usually do and
when I did it showed some improvements.

This patch series touches quite a few things along with adding support for some
new features.

- Add support for mini-LED on 2024 ROG lpatops
- Add support for the gpu MUX WMI call on Vivobook laptops
- Add support for the POST boot sound on ROG laptops
- Add support for MCU power-save (ROG Ally only, saves more power on suspend)
- Store written values for ppt_* features
- Small formatting cleanup
- Small fixes to cleanup struct holes found with pahole

Obsoletes:
- https://lore.kernel.org/all/20240320011442.11608-1-luke@ljones.dev/
- https://lore.kernel.org/all/20240310065408.63703-1-luke@ljones.dev/
- https://lore.kernel.org/all/20240310061715.16531-1-luke@ljones.dev/
- https://lore.kernel.org/all/20240310055312.11293-1-luke@ljones.dev/
- https://lore.kernel.org/all/20240310233722.30884-1-luke@ljones.dev/

Changelog:
- V1
  - Mini-LED: use asus_wmi_get_devstate() and not asus_wmi_get_devstate_simple()
  - Fix dates in Documentation/ABI/testing/sysfs-platform-asus-wmi
  - Remove <name>_available bools and rely on devid for:
    - gpu_mux
    - mini_led
    - kbd_rgb (TUF RGB LED)
- V2
  - Fix formating on select if/else blocks shown by checkpatch.pl

Luke D. Jones (9):
  platform/x86: asus-wmi: add support for 2024 ROG Mini-LED
  platform/x86: asus-wmi: add support for Vivobook GPU MUX
  platform/x86: asus-wmi: add support variant of TUF RGB
  platform/x86: asus-wmi: support toggling POST sound
  platform/x86: asus-wmi: store a min default for ppt options
  platform/x86: asus-wmi: adjust formatting of ppt-<name>() functions
  platform/x86: asus-wmi: ROG Ally increase wait time, allow MCU
    powersave
  platform/x86: asus-wmi: Add support for MCU powersave
  platform/x86: asus-wmi: cleanup main struct to avoid some holes

 .../ABI/testing/sysfs-platform-asus-wmi       |  26 ++
 drivers/platform/x86/asus-wmi.c               | 393 ++++++++++++++----
 include/linux/platform_data/x86/asus-wmi.h    |   6 +
 3 files changed, 338 insertions(+), 87 deletions(-)

-- 
2.44.0

[PATCH v2 1/9] platform/x86: asus-wmi: add support for 2024 ROG Mini-LED

[Luke D. Jones]

Support the 2024 mini-led backlight and adjust the related functions
to select the relevant dev-id. Also add `available_mini_led_mode` to the
platform sysfs since the available mini-led levels can be different.

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 .../ABI/testing/sysfs-platform-asus-wmi       |  8 ++
 drivers/platform/x86/asus-wmi.c               | 80 ++++++++++++++++---
 include/linux/platform_data/x86/asus-wmi.h    |  1 +
 3 files changed, 78 insertions(+), 11 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index 8a7e25bde085..ef1ac1a20a71 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -126,6 +126,14 @@ Description:
 		Change the mini-LED mode:
 			* 0 - Single-zone,
 			* 1 - Multi-zone
+			* 2 - Multi-zone strong (available on newer generation mini-led)
+
+What:		/sys/devices/platform/<platform>/available_mini_led_mode
+Date:		Apr 2024
+KernelVersion:	6.10
+Contact:	"Luke Jones" <luke@ljones.dev>
+Description:
+		List the available mini-led modes.
 
 What:		/sys/devices/platform/<platform>/ppt_pl1_spl
 Date:		Jun 2023
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 3f07bbf809ef..2330f02ff76f 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -288,7 +288,7 @@ struct asus_wmi {
 	bool battery_rsoc_available;
 
 	bool panel_overdrive_available;
-	bool mini_led_mode_available;
+	u32 mini_led_dev_id;
 
 	struct hotplug_slot hotplug_slot;
 	struct mutex hotplug_lock;
@@ -2108,13 +2108,30 @@ static ssize_t mini_led_mode_show(struct device *dev,
 				   struct device_attribute *attr, char *buf)
 {
 	struct asus_wmi *asus = dev_get_drvdata(dev);
-	int result;
+	u32 value;
 
-	result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_MINI_LED_MODE);
-	if (result < 0)
-		return result;
+	asus_wmi_get_devstate(asus, asus->mini_led_dev_id, &value);
+	value = value & 0x03; // only 3 modes on 2024 version
 
-	return sysfs_emit(buf, "%d\n", result);
+	/* Remap the mode values to match previous generation mini-led.
+	 * Some BIOSes return -19 instead of 2, which is "mini-LED off", this
+	 * appears to be a  BIOS bug.
+	 */
+	if (asus->mini_led_dev_id == ASUS_WMI_DEVID_MINI_LED_MODE2) {
+		switch (value) {
+		case 0:
+			value = 1;
+			break;
+		case 1:
+			value = 2;
+			break;
+		case 2:
+			value = 0;
+		}
+	} else if (value < 0) {
+		return value;
+	}
+	return sysfs_emit(buf, "%d\n", value);
 }
 
 static ssize_t mini_led_mode_store(struct device *dev,
@@ -2130,11 +2147,28 @@ static ssize_t mini_led_mode_store(struct device *dev,
 	if (result)
 		return result;
 
-	if (mode > 1)
+	if (mode > 1 && asus->mini_led_dev_id == ASUS_WMI_DEVID_MINI_LED_MODE)
 		return -EINVAL;
+	if (mode > 2 && asus->mini_led_dev_id == ASUS_WMI_DEVID_MINI_LED_MODE2)
+		return -EINVAL;
+	/*
+	 * Remap the mode values so expected behaviour is the same as the last
+	 * generation of mini-LED
+	 */
+	if (asus->mini_led_dev_id == ASUS_WMI_DEVID_MINI_LED_MODE2) {
+		switch (mode) {
+		case 0:
+			mode = 2;
+			break;
+		case 1:
+			mode = 0;
+			break;
+		case 2:
+			mode = 1;
+		}
+	}
 
-	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_MINI_LED_MODE, mode, &result);
-
+	err = asus_wmi_set_devstate(asus->mini_led_dev_id, mode, &result);
 	if (err) {
 		pr_warn("Failed to set mini-LED: %d\n", err);
 		return err;
@@ -2151,6 +2185,23 @@ static ssize_t mini_led_mode_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(mini_led_mode);
 
+static ssize_t available_mini_led_mode_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	switch (asus->mini_led_dev_id) {
+	case ASUS_WMI_DEVID_MINI_LED_MODE:
+		return sysfs_emit(buf, "0 1\n");
+	case ASUS_WMI_DEVID_MINI_LED_MODE2:
+		return sysfs_emit(buf, "0 1 2\n");
+	}
+
+	return sysfs_emit(buf, "0\n");
+}
+
+static DEVICE_ATTR_RO(available_mini_led_mode);
+
 /* Quirks *********************************************************************/
 
 static void asus_wmi_set_xusb2pr(struct asus_wmi *asus)
@@ -4139,6 +4190,7 @@ static struct attribute *platform_attributes[] = {
 	&dev_attr_nv_temp_target.attr,
 	&dev_attr_panel_od.attr,
 	&dev_attr_mini_led_mode.attr,
+	&dev_attr_available_mini_led_mode.attr,
 	NULL
 };
 
@@ -4191,7 +4243,9 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
 	else if (attr == &dev_attr_panel_od.attr)
 		ok = asus->panel_overdrive_available;
 	else if (attr == &dev_attr_mini_led_mode.attr)
-		ok = asus->mini_led_mode_available;
+		ok = asus->mini_led_dev_id != 0;
+	else if (attr == &dev_attr_available_mini_led_mode.attr)
+		ok = asus->mini_led_dev_id != 0;
 
 	if (devid != -1)
 		ok = !(asus_wmi_get_devstate_simple(asus, devid) < 0);
@@ -4444,10 +4498,14 @@ static int asus_wmi_add(struct platform_device *pdev)
 	asus->nv_dyn_boost_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_NV_DYN_BOOST);
 	asus->nv_temp_tgt_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_NV_THERM_TARGET);
 	asus->panel_overdrive_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PANEL_OD);
-	asus->mini_led_mode_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE);
 	asus->ally_mcu_usb_switch = acpi_has_method(NULL, ASUS_USB0_PWR_EC0_CSEE)
 						&& dmi_match(DMI_BOARD_NAME, "RC71L");
 
+	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE))
+		asus->mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE;
+	else if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE2))
+		asus->mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE2;
+
 	err = fan_boost_mode_check_present(asus);
 	if (err)
 		goto fail_fan_boost_mode;
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index ab1c7deff118..9cadce10ad9a 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -71,6 +71,7 @@
 #define ASUS_WMI_DEVID_LID_FLIP		0x00060062
 #define ASUS_WMI_DEVID_LID_FLIP_ROG	0x00060077
 #define ASUS_WMI_DEVID_MINI_LED_MODE	0x0005001E
+#define ASUS_WMI_DEVID_MINI_LED_MODE2	0x0005002E
 
 /* Storage */
 #define ASUS_WMI_DEVID_CARDREADER	0x00080013
-- 
2.44.0

Re: [PATCH v2 1/9] platform/x86: asus-wmi: add support for 2024 ROG Mini-LED

[Ilpo Järvinen]

On Tue, 2 Apr 2024, Luke D. Jones wrote:

> Support the 2024 mini-led backlight and adjust the related functions
> to select the relevant dev-id. Also add `available_mini_led_mode` to the
> platform sysfs since the available mini-led levels can be different.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>  .../ABI/testing/sysfs-platform-asus-wmi       |  8 ++
>  drivers/platform/x86/asus-wmi.c               | 80 ++++++++++++++++---
>  include/linux/platform_data/x86/asus-wmi.h    |  1 +
>  3 files changed, 78 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> index 8a7e25bde085..ef1ac1a20a71 100644
> --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> @@ -126,6 +126,14 @@ Description:
>  		Change the mini-LED mode:
>  			* 0 - Single-zone,
>  			* 1 - Multi-zone
> +			* 2 - Multi-zone strong (available on newer generation mini-led)
> +
> +What:		/sys/devices/platform/<platform>/available_mini_led_mode
> +Date:		Apr 2024
> +KernelVersion:	6.10
> +Contact:	"Luke Jones" <luke@ljones.dev>
> +Description:
> +		List the available mini-led modes.
>  
>  What:		/sys/devices/platform/<platform>/ppt_pl1_spl
>  Date:		Jun 2023
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 3f07bbf809ef..2330f02ff76f 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -288,7 +288,7 @@ struct asus_wmi {
>  	bool battery_rsoc_available;
>  
>  	bool panel_overdrive_available;
> -	bool mini_led_mode_available;
> +	u32 mini_led_dev_id;
>  
>  	struct hotplug_slot hotplug_slot;
>  	struct mutex hotplug_lock;
> @@ -2108,13 +2108,30 @@ static ssize_t mini_led_mode_show(struct device *dev,
>  				   struct device_attribute *attr, char *buf)
>  {
>  	struct asus_wmi *asus = dev_get_drvdata(dev);
> -	int result;
> +	u32 value;
>  
> -	result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_MINI_LED_MODE);
> -	if (result < 0)
> -		return result;
> +	asus_wmi_get_devstate(asus, asus->mini_led_dev_id, &value);

Error handling missing.

> +	value = value & 0x03; // only 3 modes on 2024 version

Add #define XX GENMASK(1, 0) for this.

>  
> -	return sysfs_emit(buf, "%d\n", result);
> +	/* Remap the mode values to match previous generation mini-led.
> +	 * Some BIOSes return -19 instead of 2, which is "mini-LED off", this
> +	 * appears to be a  BIOS bug.

Is this comment still 100% valid now or should it be removed completely? 
There's no handling for -19 in ASUS_WMI_DEVID_MINI_LED_MODE2 block?

There's also a double space in the comment.

> +	 */
> +	if (asus->mini_led_dev_id == ASUS_WMI_DEVID_MINI_LED_MODE2) {
> +		switch (value) {
> +		case 0:
> +			value = 1;
> +			break;
> +		case 1:
> +			value = 2;
> +			break;
> +		case 2:
> +			value = 0;

Add break here too.

These literals 0-2 should be named with #defines as it would make the code 
readable, current way of the mapping between literal numbers 
unintelligible magic.

> +		}
> +	} else if (value < 0) {

This will never be true because value is u32 and also because of & 0x03 
even if you'd change the type.

I don't quite follow what you're trying to do here. Why this only applies 
to cases != ASUS_WMI_DEVID_MINI_LED_MODE2?

> +		return value;
> +	}
> +	return sysfs_emit(buf, "%d\n", value);
>  }
>  
>  static ssize_t mini_led_mode_store(struct device *dev,
> @@ -2130,11 +2147,28 @@ static ssize_t mini_led_mode_store(struct device *dev,
>  	if (result)
>  		return result;
>  
> -	if (mode > 1)
> +	if (mode > 1 && asus->mini_led_dev_id == ASUS_WMI_DEVID_MINI_LED_MODE)
>  		return -EINVAL;
> +	if (mode > 2 && asus->mini_led_dev_id == ASUS_WMI_DEVID_MINI_LED_MODE2)
> +		return -EINVAL;
> +	/*
> +	 * Remap the mode values so expected behaviour is the same as the last
> +	 * generation of mini-LED

Missing .

-- 
 i.

> +	 */
> +	if (asus->mini_led_dev_id == ASUS_WMI_DEVID_MINI_LED_MODE2) {
> +		switch (mode) {
> +		case 0:
> +			mode = 2;
> +			break;
> +		case 1:
> +			mode = 0;
> +			break;
> +		case 2:
> +			mode = 1;
> +		}
> +	}
>  
> -	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_MINI_LED_MODE, mode, &result);
> -
> +	err = asus_wmi_set_devstate(asus->mini_led_dev_id, mode, &result);
>  	if (err) {
>  		pr_warn("Failed to set mini-LED: %d\n", err);
>  		return err;
> @@ -2151,6 +2185,23 @@ static ssize_t mini_led_mode_store(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(mini_led_mode);
>  
> +static ssize_t available_mini_led_mode_show(struct device *dev,
> +				  struct device_attribute *attr, char *buf)
> +{
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +	switch (asus->mini_led_dev_id) {
> +	case ASUS_WMI_DEVID_MINI_LED_MODE:
> +		return sysfs_emit(buf, "0 1\n");
> +	case ASUS_WMI_DEVID_MINI_LED_MODE2:
> +		return sysfs_emit(buf, "0 1 2\n");
> +	}
> +
> +	return sysfs_emit(buf, "0\n");
> +}
> +
> +static DEVICE_ATTR_RO(available_mini_led_mode);
> +
>  /* Quirks *********************************************************************/
>  
>  static void asus_wmi_set_xusb2pr(struct asus_wmi *asus)
> @@ -4139,6 +4190,7 @@ static struct attribute *platform_attributes[] = {
>  	&dev_attr_nv_temp_target.attr,
>  	&dev_attr_panel_od.attr,
>  	&dev_attr_mini_led_mode.attr,
> +	&dev_attr_available_mini_led_mode.attr,
>  	NULL
>  };
>  
> @@ -4191,7 +4243,9 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
>  	else if (attr == &dev_attr_panel_od.attr)
>  		ok = asus->panel_overdrive_available;
>  	else if (attr == &dev_attr_mini_led_mode.attr)
> -		ok = asus->mini_led_mode_available;
> +		ok = asus->mini_led_dev_id != 0;
> +	else if (attr == &dev_attr_available_mini_led_mode.attr)
> +		ok = asus->mini_led_dev_id != 0;
>  
>  	if (devid != -1)
>  		ok = !(asus_wmi_get_devstate_simple(asus, devid) < 0);
> @@ -4444,10 +4498,14 @@ static int asus_wmi_add(struct platform_device *pdev)
>  	asus->nv_dyn_boost_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_NV_DYN_BOOST);
>  	asus->nv_temp_tgt_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_NV_THERM_TARGET);
>  	asus->panel_overdrive_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PANEL_OD);
> -	asus->mini_led_mode_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE);
>  	asus->ally_mcu_usb_switch = acpi_has_method(NULL, ASUS_USB0_PWR_EC0_CSEE)
>  						&& dmi_match(DMI_BOARD_NAME, "RC71L");
>  
> +	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE))
> +		asus->mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE;
> +	else if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE2))
> +		asus->mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE2;
> +
>  	err = fan_boost_mode_check_present(asus);
>  	if (err)
>  		goto fail_fan_boost_mode;
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index ab1c7deff118..9cadce10ad9a 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -71,6 +71,7 @@
>  #define ASUS_WMI_DEVID_LID_FLIP		0x00060062
>  #define ASUS_WMI_DEVID_LID_FLIP_ROG	0x00060077
>  #define ASUS_WMI_DEVID_MINI_LED_MODE	0x0005001E
> +#define ASUS_WMI_DEVID_MINI_LED_MODE2	0x0005002E
>  
>  /* Storage */
>  #define ASUS_WMI_DEVID_CARDREADER	0x00080013
> 

Re: [PATCH v2 1/9] platform/x86: asus-wmi: add support for 2024 ROG Mini-LED

[Luke Jones]

On Tuesday, 2 April 2024 11:43:25 PM NZDT Ilpo Järvinen wrote:
> On Tue, 2 Apr 2024, Luke D. Jones wrote:
> > Support the 2024 mini-led backlight and adjust the related functions
> > to select the relevant dev-id. Also add `available_mini_led_mode` to the
> > platform sysfs since the available mini-led levels can be different.
> > 
> > Signed-off-by: Luke D. Jones <luke@ljones.dev>
> > ---
> > 
> >  .../ABI/testing/sysfs-platform-asus-wmi       |  8 ++
> >  drivers/platform/x86/asus-wmi.c               | 80 ++++++++++++++++---
> >  include/linux/platform_data/x86/asus-wmi.h    |  1 +
> >  3 files changed, 78 insertions(+), 11 deletions(-)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> > b/Documentation/ABI/testing/sysfs-platform-asus-wmi index
> > 8a7e25bde085..ef1ac1a20a71 100644
> > --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> > +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> > 
> > @@ -126,6 +126,14 @@ Description:
> >  		Change the mini-LED mode:
> >  			* 0 - Single-zone,
> >  			* 1 - Multi-zone
> > 
> > +			* 2 - Multi-zone strong (available on newer 
generation mini-led)
> > +
> > +What:		/sys/devices/platform/<platform>/available_mini_led_mode
> > +Date:		Apr 2024
> > +KernelVersion:	6.10
> > +Contact:	"Luke Jones" <luke@ljones.dev>
> > +Description:
> > +		List the available mini-led modes.
> > 
> >  What:		/sys/devices/platform/<platform>/ppt_pl1_spl
> >  Date:		Jun 2023
> > 
> > diff --git a/drivers/platform/x86/asus-wmi.c
> > b/drivers/platform/x86/asus-wmi.c index 3f07bbf809ef..2330f02ff76f 100644
> > --- a/drivers/platform/x86/asus-wmi.c
> > +++ b/drivers/platform/x86/asus-wmi.c
> > @@ -288,7 +288,7 @@ struct asus_wmi {
> > 
> >  	bool battery_rsoc_available;
> >  	
> >  	bool panel_overdrive_available;
> > 
> > -	bool mini_led_mode_available;
> > +	u32 mini_led_dev_id;
> > 
> >  	struct hotplug_slot hotplug_slot;
> >  	struct mutex hotplug_lock;
> > 
> > @@ -2108,13 +2108,30 @@ static ssize_t mini_led_mode_show(struct device
> > *dev,> 
> >  				   struct device_attribute *attr, 
char *buf)
> >  
> >  {
> >  
> >  	struct asus_wmi *asus = dev_get_drvdata(dev);
> > 
> > -	int result;
> > +	u32 value;
> > 
> > -	result = asus_wmi_get_devstate_simple(asus,
> > ASUS_WMI_DEVID_MINI_LED_MODE); -	if (result < 0)
> > -		return result;
> > +	asus_wmi_get_devstate(asus, asus->mini_led_dev_id, &value);
> 
> Error handling missing.

I omitted it because other uses of this function also ignore it. Might need to 
do another patch to add handling everywhere else.

For now I will add here, and later in the week fix the other cases.

> 
> > +	value = value & 0x03; // only 3 modes on 2024 version
> 
> Add #define XX GENMASK(1, 0) for this.

Done. I should have done so before, I know.
There are also spots where a mask is explicitly 0xFFFF while there is a define 
for that also. Maybe another patch to do here?

> 
> > -	return sysfs_emit(buf, "%d\n", result);
> > +	/* Remap the mode values to match previous generation mini-led.
> > +	 * Some BIOSes return -19 instead of 2, which is "mini-LED off", 
this
> > +	 * appears to be a  BIOS bug.
> 
> Is this comment still 100% valid now or should it be removed completely?
> There's no handling for -19 in ASUS_WMI_DEVID_MINI_LED_MODE2 block?
> 
> There's also a double space in the comment.

Yay, adhd coding :)
Done.

> 
> > +	 */
> > +	if (asus->mini_led_dev_id == ASUS_WMI_DEVID_MINI_LED_MODE2) {
> > +		switch (value) {
> > +		case 0:
> > +			value = 1;
> > +			break;
> > +		case 1:
> > +			value = 2;
> > +			break;
> > +		case 2:
> > +			value = 0;
> 
> Add break here too.

Wasn't sure it needed it due to being last statement? In either case, done.

> 
> These literals 0-2 should be named with #defines as it would make the code
> readable, current way of the mapping between literal numbers
> unintelligible magic.

Honestly with the block below it reversing them I thought it was easier. I'll 
try and come up with some decent define names.

> 
> > +		}
> > +	} else if (value < 0) {
> 
> This will never be true because value is u32 and also because of & 0x03
> even if you'd change the type.
> 
> I don't quite follow what you're trying to do here. Why this only applies
> to cases != ASUS_WMI_DEVID_MINI_LED_MODE2?

Ah.. yes I'll write that comment much better, and the defines hopefully help

> 
> > +		return value;
> > +	}
> > +	return sysfs_emit(buf, "%d\n", value);
> > 
> >  }
> >  
> >  static ssize_t mini_led_mode_store(struct device *dev,
> > 
> > @@ -2130,11 +2147,28 @@ static ssize_t mini_led_mode_store(struct device
> > *dev,> 
> >  	if (result)
> >  	
> >  		return result;
> > 
> > -	if (mode > 1)
> > +	if (mode > 1 && asus->mini_led_dev_id == 
ASUS_WMI_DEVID_MINI_LED_MODE)
> > 
> >  		return -EINVAL;
> > 
> > +	if (mode > 2 && asus->mini_led_dev_id == 
ASUS_WMI_DEVID_MINI_LED_MODE2)
> > +		return -EINVAL;
> > +	/*
> > +	 * Remap the mode values so expected behaviour is the same as the 
last
> > +	 * generation of mini-LED
> 
> Missing .

Done

I think everything is in good shape now. Thank you so much for your thorough 
reviews.

> 
> > +	 */
> > +	if (asus->mini_led_dev_id == ASUS_WMI_DEVID_MINI_LED_MODE2) {
> > +		switch (mode) {
> > +		case 0:
> > +			mode = 2;
> > +			break;
> > +		case 1:
> > +			mode = 0;
> > +			break;
> > +		case 2:
> > +			mode = 1;
> > +		}
> > +	}
> > 
> > -	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_MINI_LED_MODE, mode,
> > &result);
> > -
> > +	err = asus_wmi_set_devstate(asus->mini_led_dev_id, mode, &result);
> > 
> >  	if (err) {
> >  	
> >  		pr_warn("Failed to set mini-LED: %d\n", err);
> >  		return err;
> > 
> > @@ -2151,6 +2185,23 @@ static ssize_t mini_led_mode_store(struct device
> > *dev,> 
> >  }
> >  static DEVICE_ATTR_RW(mini_led_mode);
> > 
> > +static ssize_t available_mini_led_mode_show(struct device *dev,
> > +				  struct device_attribute *attr, 
char *buf)
> > +{
> > +	struct asus_wmi *asus = dev_get_drvdata(dev);
> > +
> > +	switch (asus->mini_led_dev_id) {
> > +	case ASUS_WMI_DEVID_MINI_LED_MODE:
> > +		return sysfs_emit(buf, "0 1\n");
> > +	case ASUS_WMI_DEVID_MINI_LED_MODE2:
> > +		return sysfs_emit(buf, "0 1 2\n");
> > +	}
> > +
> > +	return sysfs_emit(buf, "0\n");
> > +}
> > +
> > +static DEVICE_ATTR_RO(available_mini_led_mode);
> > +
> > 
> >  /* Quirks
> >  *********************************************************************/
> >  
> >  static void asus_wmi_set_xusb2pr(struct asus_wmi *asus)
> > 
> > @@ -4139,6 +4190,7 @@ static struct attribute *platform_attributes[] = {
> > 
> >  	&dev_attr_nv_temp_target.attr,
> >  	&dev_attr_panel_od.attr,
> >  	&dev_attr_mini_led_mode.attr,
> > 
> > +	&dev_attr_available_mini_led_mode.attr,
> > 
> >  	NULL
> >  
> >  };
> > 
> > @@ -4191,7 +4243,9 @@ static umode_t asus_sysfs_is_visible(struct kobject
> > *kobj,> 
> >  	else if (attr == &dev_attr_panel_od.attr)
> >  	
> >  		ok = asus->panel_overdrive_available;
> >  	
> >  	else if (attr == &dev_attr_mini_led_mode.attr)
> > 
> > -		ok = asus->mini_led_mode_available;
> > +		ok = asus->mini_led_dev_id != 0;
> > +	else if (attr == &dev_attr_available_mini_led_mode.attr)
> > +		ok = asus->mini_led_dev_id != 0;
> > 
> >  	if (devid != -1)
> >  	
> >  		ok = !(asus_wmi_get_devstate_simple(asus, devid) < 0);
> > 
> > @@ -4444,10 +4498,14 @@ static int asus_wmi_add(struct platform_device
> > *pdev)> 
> >  	asus->nv_dyn_boost_available = asus_wmi_dev_is_present(asus,
> >  	ASUS_WMI_DEVID_NV_DYN_BOOST); asus->nv_temp_tgt_available =
> >  	asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_NV_THERM_TARGET);
> >  	asus->panel_overdrive_available = asus_wmi_dev_is_present(asus,
> >  	ASUS_WMI_DEVID_PANEL_OD);> 
> > -	asus->mini_led_mode_available = asus_wmi_dev_is_present(asus,
> > ASUS_WMI_DEVID_MINI_LED_MODE);> 
> >  	asus->ally_mcu_usb_switch = acpi_has_method(NULL,
> >  	ASUS_USB0_PWR_EC0_CSEE)
> >  	
> >  						&& 
dmi_match(DMI_BOARD_NAME, "RC71L");
> > 
> > +	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE))
> > +		asus->mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE;
> > +	else if (asus_wmi_dev_is_present(asus, 
ASUS_WMI_DEVID_MINI_LED_MODE2))
> > +		asus->mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE2;
> > +
> > 
> >  	err = fan_boost_mode_check_present(asus);
> >  	if (err)
> >  	
> >  		goto fail_fan_boost_mode;
> > 
> > diff --git a/include/linux/platform_data/x86/asus-wmi.h
> > b/include/linux/platform_data/x86/asus-wmi.h index
> > ab1c7deff118..9cadce10ad9a 100644
> > --- a/include/linux/platform_data/x86/asus-wmi.h
> > +++ b/include/linux/platform_data/x86/asus-wmi.h
> > @@ -71,6 +71,7 @@
> > 
> >  #define ASUS_WMI_DEVID_LID_FLIP		0x00060062
> >  #define ASUS_WMI_DEVID_LID_FLIP_ROG	0x00060077
> >  #define ASUS_WMI_DEVID_MINI_LED_MODE	0x0005001E
> > 
> > +#define ASUS_WMI_DEVID_MINI_LED_MODE2	0x0005002E
> > 
> >  /* Storage */
> >  #define ASUS_WMI_DEVID_CARDREADER	0x00080013

[PATCH v2 2/9] platform/x86: asus-wmi: add support for Vivobook GPU MUX

[Luke D. Jones]

Add support for the Vivobook dgpu MUX available on the ASUS Viviobook
and some of the other ranges (Zen).

This MUX functions exactly the same as the existing ROG MUX support so
the existing functionality now detects which MUX is available and uses
that for control.

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 drivers/platform/x86/asus-wmi.c            | 22 +++++++++++++---------
 include/linux/platform_data/x86/asus-wmi.h |  1 +
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 2330f02ff76f..797531300138 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -259,7 +259,7 @@ struct asus_wmi {
 	bool egpu_enable_available;
 	bool egpu_connect_available;
 	bool dgpu_disable_available;
-	bool gpu_mux_mode_available;
+	u32 gpu_mux_dev;
 
 	/* Tunables provided by ASUS for gaming laptops */
 	bool ppt_pl2_sppt_available;
@@ -682,8 +682,8 @@ static ssize_t dgpu_disable_store(struct device *dev,
 	if (disable > 1)
 		return -EINVAL;
 
-	if (asus->gpu_mux_mode_available) {
-		result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_GPU_MUX);
+	if (asus->gpu_mux_dev) {
+		result = asus_wmi_get_devstate_simple(asus, asus->gpu_mux_dev);
 		if (result < 0)
 			/* An error here may signal greater failure of GPU handling */
 			return result;
@@ -748,8 +748,8 @@ static ssize_t egpu_enable_store(struct device *dev,
 		return err;
 	}
 
-	if (asus->gpu_mux_mode_available) {
-		result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_GPU_MUX);
+	if (asus->gpu_mux_dev) {
+		result = asus_wmi_get_devstate_simple(asus, asus->gpu_mux_dev);
 		if (result < 0) {
 			/* An error here may signal greater failure of GPU handling */
 			pr_warn("Failed to get gpu mux status: %d\n", result);
@@ -802,7 +802,7 @@ static ssize_t gpu_mux_mode_show(struct device *dev,
 	struct asus_wmi *asus = dev_get_drvdata(dev);
 	int result;
 
-	result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_GPU_MUX);
+	result = asus_wmi_get_devstate_simple(asus, asus->gpu_mux_dev);
 	if (result < 0)
 		return result;
 
@@ -848,7 +848,7 @@ static ssize_t gpu_mux_mode_store(struct device *dev,
 		}
 	}
 
-	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_GPU_MUX, optimus, &result);
+	err = asus_wmi_set_devstate(asus->gpu_mux_dev, optimus, &result);
 	if (err) {
 		dev_err(dev, "Failed to set GPU MUX mode: %d\n", err);
 		return err;
@@ -4221,7 +4221,7 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
 	else if (attr == &dev_attr_dgpu_disable.attr)
 		ok = asus->dgpu_disable_available;
 	else if (attr == &dev_attr_gpu_mux_mode.attr)
-		ok = asus->gpu_mux_mode_available;
+		ok = asus->gpu_mux_dev != 0;
 	else if (attr == &dev_attr_fan_boost_mode.attr)
 		ok = asus->fan_boost_mode_available;
 	else if (attr == &dev_attr_throttle_thermal_policy.attr)
@@ -4487,7 +4487,6 @@ static int asus_wmi_add(struct platform_device *pdev)
 	asus->egpu_enable_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_EGPU);
 	asus->egpu_connect_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_EGPU_CONNECTED);
 	asus->dgpu_disable_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_DGPU);
-	asus->gpu_mux_mode_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_GPU_MUX);
 	asus->kbd_rgb_mode_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_MODE);
 	asus->kbd_rgb_state_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_STATE);
 	asus->ppt_pl2_sppt_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PPT_PL2_SPPT);
@@ -4506,6 +4505,11 @@ static int asus_wmi_add(struct platform_device *pdev)
 	else if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE2))
 		asus->mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE2;
 
+	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_GPU_MUX))
+		asus->gpu_mux_dev = ASUS_WMI_DEVID_GPU_MUX;
+	else if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_GPU_MUX_VIVO))
+		asus->gpu_mux_dev = ASUS_WMI_DEVID_GPU_MUX_VIVO;
+
 	err = fan_boost_mode_check_present(asus);
 	if (err)
 		goto fail_fan_boost_mode;
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 9cadce10ad9a..b48b024dd844 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -128,6 +128,7 @@
 
 /* gpu mux switch, 0 = dGPU, 1 = Optimus */
 #define ASUS_WMI_DEVID_GPU_MUX		0x00090016
+#define ASUS_WMI_DEVID_GPU_MUX_VIVO	0x00090026
 
 /* TUF laptop RGB modes/colours */
 #define ASUS_WMI_DEVID_TUF_RGB_MODE	0x00100056
-- 
2.44.0

Re: [PATCH v2 2/9] platform/x86: asus-wmi: add support for Vivobook GPU MUX

[Ilpo Järvinen]

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

On Tue, 2 Apr 2024, Luke D. Jones wrote:

> Add support for the Vivobook dgpu MUX available on the ASUS Viviobook
> and some of the other ranges (Zen).
> 
> This MUX functions exactly the same as the existing ROG MUX support so
> the existing functionality now detects which MUX is available and uses
> that for control.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

[PATCH v2 3/9] platform/x86: asus-wmi: add support variant of TUF RGB

[Luke D. Jones]

Adds support for a second TUF RGB wmi call that some versions of the TUF
laptop come with. Also adjusts existing support to select whichever is
available.

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 drivers/platform/x86/asus-wmi.c            | 13 +++++++++----
 include/linux/platform_data/x86/asus-wmi.h |  1 +
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 797531300138..b5bd42e5f4ef 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -270,7 +270,7 @@ struct asus_wmi {
 	bool nv_dyn_boost_available;
 	bool nv_temp_tgt_available;
 
-	bool kbd_rgb_mode_available;
+	u32 kbd_rgb_dev;
 	bool kbd_rgb_state_available;
 
 	bool throttle_thermal_policy_available;
@@ -870,6 +870,7 @@ static ssize_t kbd_rgb_mode_store(struct device *dev,
 				 struct device_attribute *attr,
 				 const char *buf, size_t count)
 {
+	struct asus_wmi *asus = dev_get_drvdata(dev);
 	u32 cmd, mode, r, g, b, speed;
 	int err;
 
@@ -906,7 +907,7 @@ static ssize_t kbd_rgb_mode_store(struct device *dev,
 		speed = 0xeb;
 	}
 
-	err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS, ASUS_WMI_DEVID_TUF_RGB_MODE,
+	err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS, asus->kbd_rgb_dev,
 			cmd | (mode << 8) | (r << 16) | (g << 24), b | (speed << 8), NULL);
 	if (err)
 		return err;
@@ -1549,7 +1550,7 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
 {
 	int rv = 0, num_rgb_groups = 0, led_val;
 
-	if (asus->kbd_rgb_mode_available)
+	if (asus->kbd_rgb_dev)
 		kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_mode_group;
 	if (asus->kbd_rgb_state_available)
 		kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_state_group;
@@ -4487,7 +4488,6 @@ static int asus_wmi_add(struct platform_device *pdev)
 	asus->egpu_enable_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_EGPU);
 	asus->egpu_connect_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_EGPU_CONNECTED);
 	asus->dgpu_disable_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_DGPU);
-	asus->kbd_rgb_mode_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_MODE);
 	asus->kbd_rgb_state_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_STATE);
 	asus->ppt_pl2_sppt_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PPT_PL2_SPPT);
 	asus->ppt_pl1_spl_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PPT_PL1_SPL);
@@ -4510,6 +4510,11 @@ static int asus_wmi_add(struct platform_device *pdev)
 	else if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_GPU_MUX_VIVO))
 		asus->gpu_mux_dev = ASUS_WMI_DEVID_GPU_MUX_VIVO;
 
+	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_MODE))
+		asus->kbd_rgb_dev = ASUS_WMI_DEVID_TUF_RGB_MODE;
+	else if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_MODE2))
+		asus->kbd_rgb_dev = ASUS_WMI_DEVID_TUF_RGB_MODE2;
+
 	err = fan_boost_mode_check_present(asus);
 	if (err)
 		goto fail_fan_boost_mode;
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index b48b024dd844..3e9a01467c67 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -132,6 +132,7 @@
 
 /* TUF laptop RGB modes/colours */
 #define ASUS_WMI_DEVID_TUF_RGB_MODE	0x00100056
+#define ASUS_WMI_DEVID_TUF_RGB_MODE2	0x0010005A
 
 /* TUF laptop RGB power/state */
 #define ASUS_WMI_DEVID_TUF_RGB_STATE	0x00100057
-- 
2.44.0

Re: [PATCH v2 3/9] platform/x86: asus-wmi: add support variant of TUF RGB

[Ilpo Järvinen]

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

On Tue, 2 Apr 2024, Luke D. Jones wrote:

> Adds support for a second TUF RGB wmi call that some versions of the TUF
> laptop come with. Also adjusts existing support to select whichever is
> available.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

[PATCH v2 4/9] platform/x86: asus-wmi: support toggling POST sound

[Luke D. Jones]

Add support for toggling the BIOS POST sound on some ASUS laptops.

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 .../ABI/testing/sysfs-platform-asus-wmi       |  9 ++++
 drivers/platform/x86/asus-wmi.c               | 51 +++++++++++++++++++
 include/linux/platform_data/x86/asus-wmi.h    |  3 ++
 3 files changed, 63 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index ef1ac1a20a71..41b92e53e88a 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -194,3 +194,12 @@ Contact:	"Luke Jones" <luke@ljones.dev>
 Description:
 		Set the target temperature limit of the Nvidia dGPU:
 			* min=75, max=87
+
+What:		/sys/devices/platform/<platform>/boot_sound
+Date:		Apr 2024
+KernelVersion:	6.10
+Contact:	"Luke Jones" <luke@ljones.dev>
+Description:
+		Set if the BIOS POST sound is played on boot.
+			* 0 - False,
+			* 1 - True
\ No newline at end of file
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index b5bd42e5f4ef..b795f9c1941f 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -2104,6 +2104,54 @@ static ssize_t panel_od_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(panel_od);
 
+/* Bootup sound ***************************************************************/
+
+static ssize_t boot_sound_show(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+	int result;
+
+	result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_BOOT_SOUND);
+	if (result < 0)
+		return result;
+
+	return sysfs_emit(buf, "%d\n", result);
+}
+
+static ssize_t boot_sound_store(struct device *dev,
+			      struct device_attribute *attr,
+			      const char *buf, size_t count)
+{
+	int result, err;
+	u32 snd;
+
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	result = kstrtou32(buf, 10, &snd);
+	if (result)
+		return result;
+
+	if (snd > 1)
+		return -EINVAL;
+
+	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_BOOT_SOUND, snd, &result);
+	if (err) {
+		pr_warn("Failed to set boot sound: %d\n", err);
+		return err;
+	}
+
+	if (result > 1) {
+		pr_warn("Failed to set panel boot sound (result): 0x%x\n", result);
+		return -EIO;
+	}
+
+	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "boot_sound");
+
+	return count;
+}
+static DEVICE_ATTR_RW(boot_sound);
+
 /* Mini-LED mode **************************************************************/
 static ssize_t mini_led_mode_show(struct device *dev,
 				   struct device_attribute *attr, char *buf)
@@ -4189,6 +4237,7 @@ static struct attribute *platform_attributes[] = {
 	&dev_attr_ppt_platform_sppt.attr,
 	&dev_attr_nv_dynamic_boost.attr,
 	&dev_attr_nv_temp_target.attr,
+	&dev_attr_boot_sound.attr,
 	&dev_attr_panel_od.attr,
 	&dev_attr_mini_led_mode.attr,
 	&dev_attr_available_mini_led_mode.attr,
@@ -4241,6 +4290,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
 		ok = asus->nv_dyn_boost_available;
 	else if (attr == &dev_attr_nv_temp_target.attr)
 		ok = asus->nv_temp_tgt_available;
+	else if (attr == &dev_attr_boot_sound.attr)
+		devid = ASUS_WMI_DEVID_BOOT_SOUND;
 	else if (attr == &dev_attr_panel_od.attr)
 		ok = asus->panel_overdrive_available;
 	else if (attr == &dev_attr_mini_led_mode.attr)
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 3e9a01467c67..3eb5cd6773ad 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -137,6 +137,9 @@
 /* TUF laptop RGB power/state */
 #define ASUS_WMI_DEVID_TUF_RGB_STATE	0x00100057
 
+/* Bootup sound control */
+#define ASUS_WMI_DEVID_BOOT_SOUND	0x00130022
+
 /* DSTS masks */
 #define ASUS_WMI_DSTS_STATUS_BIT	0x00000001
 #define ASUS_WMI_DSTS_UNKNOWN_BIT	0x00000002
-- 
2.44.0

Re: [PATCH v2 4/9] platform/x86: asus-wmi: support toggling POST sound

[Ilpo Järvinen]

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

On Tue, 2 Apr 2024, Luke D. Jones wrote:

> Add support for toggling the BIOS POST sound on some ASUS laptops.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>  .../ABI/testing/sysfs-platform-asus-wmi       |  9 ++++
>  drivers/platform/x86/asus-wmi.c               | 51 +++++++++++++++++++
>  include/linux/platform_data/x86/asus-wmi.h    |  3 ++
>  3 files changed, 63 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> index ef1ac1a20a71..41b92e53e88a 100644
> --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> @@ -194,3 +194,12 @@ Contact:	"Luke Jones" <luke@ljones.dev>
>  Description:
>  		Set the target temperature limit of the Nvidia dGPU:
>  			* min=75, max=87
> +
> +What:		/sys/devices/platform/<platform>/boot_sound
> +Date:		Apr 2024
> +KernelVersion:	6.10
> +Contact:	"Luke Jones" <luke@ljones.dev>
> +Description:
> +		Set if the BIOS POST sound is played on boot.
> +			* 0 - False,
> +			* 1 - True
> \ No newline at end of file

Add the last newline please.

After fixing that, please add:

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


-- 
 i.

Re: [PATCH v2 4/9] platform/x86: asus-wmi: support toggling POST sound

[Luke Jones]

On Tuesday, 2 April 2024 11:47:08 PM NZDT Ilpo Järvinen wrote:
> On Tue, 2 Apr 2024, Luke D. Jones wrote:
> > Add support for toggling the BIOS POST sound on some ASUS laptops.
> > 
> > Signed-off-by: Luke D. Jones <luke@ljones.dev>
> > ---
> > 
> >  .../ABI/testing/sysfs-platform-asus-wmi       |  9 ++++
> >  drivers/platform/x86/asus-wmi.c               | 51 +++++++++++++++++++
> >  include/linux/platform_data/x86/asus-wmi.h    |  3 ++
> >  3 files changed, 63 insertions(+)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> > b/Documentation/ABI/testing/sysfs-platform-asus-wmi index
> > ef1ac1a20a71..41b92e53e88a 100644
> > --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> > +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> > @@ -194,3 +194,12 @@ Contact:	"Luke Jones" <luke@ljones.dev>
> > 
> >  Description:
> >  		Set the target temperature limit of the Nvidia dGPU:
> >  			* min=75, max=87
> > 
> > +
> > +What:		/sys/devices/platform/<platform>/boot_sound
> > +Date:		Apr 2024
> > +KernelVersion:	6.10
> > +Contact:	"Luke Jones" <luke@ljones.dev>
> > +Description:
> > +		Set if the BIOS POST sound is played on boot.
> > +			* 0 - False,
> > +			* 1 - True
> > \ No newline at end of file
> 
> Add the last newline please.

Editor removed :(

> 
> After fixing that, please add:
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Done and done.

[PATCH v2 5/9] platform/x86: asus-wmi: store a min default for ppt options

[Luke D. Jones]

Laptops with any of the ppt or nv tunables default to the minimum setting
on boot so we can safely assume a stored value is correct.

This patch adds storing of those values in the local struct, and enables
reading of those values back. To prevent creating a series of byte holes
in the struct the "<name>_available" bool is removed and
`asus_sysfs_is_visible()` uses the `ASUS_WMI_DEVID_<name>` directly.

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 drivers/platform/x86/asus-wmi.c | 127 +++++++++++++++++++++++++-------
 1 file changed, 99 insertions(+), 28 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index b795f9c1941f..c80afa385532 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -262,13 +262,13 @@ struct asus_wmi {
 	u32 gpu_mux_dev;
 
 	/* Tunables provided by ASUS for gaming laptops */
-	bool ppt_pl2_sppt_available;
-	bool ppt_pl1_spl_available;
-	bool ppt_apu_sppt_available;
-	bool ppt_plat_sppt_available;
-	bool ppt_fppt_available;
-	bool nv_dyn_boost_available;
-	bool nv_temp_tgt_available;
+	u32 ppt_pl2_sppt;
+	u32 ppt_pl1_spl;
+	u32 ppt_apu_sppt;
+	u32 ppt_platform_sppt;
+	u32 ppt_fppt;
+	u32 nv_dynamic_boost;
+	u32 nv_temp_target;
 
 	u32 kbd_rgb_dev;
 	bool kbd_rgb_state_available;
@@ -1020,11 +1020,21 @@ static ssize_t ppt_pl2_sppt_store(struct device *dev,
 		return -EIO;
 	}
 
+	asus->ppt_pl2_sppt = value;
 	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "ppt_pl2_sppt");
 
 	return count;
 }
-static DEVICE_ATTR_WO(ppt_pl2_sppt);
+
+static ssize_t ppt_pl2_sppt_show(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%d\n", asus->ppt_pl2_sppt);
+}
+static DEVICE_ATTR_RW(ppt_pl2_sppt);
 
 /* Tunable: PPT, Intel=PL1, AMD=SPL ******************************************/
 static ssize_t ppt_pl1_spl_store(struct device *dev,
@@ -1054,11 +1064,20 @@ static ssize_t ppt_pl1_spl_store(struct device *dev,
 		return -EIO;
 	}
 
+	asus->ppt_pl1_spl = value;
 	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "ppt_pl1_spl");
 
 	return count;
 }
-static DEVICE_ATTR_WO(ppt_pl1_spl);
+static ssize_t ppt_pl1_spl_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%d\n", asus->ppt_pl1_spl);
+}
+static DEVICE_ATTR_RW(ppt_pl1_spl);
 
 /* Tunable: PPT APU FPPT ******************************************************/
 static ssize_t ppt_fppt_store(struct device *dev,
@@ -1088,11 +1107,21 @@ static ssize_t ppt_fppt_store(struct device *dev,
 		return -EIO;
 	}
 
+	asus->ppt_fppt = value;
 	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "ppt_fpu_sppt");
 
 	return count;
 }
-static DEVICE_ATTR_WO(ppt_fppt);
+
+static ssize_t ppt_fppt_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%d\n", asus->ppt_fppt);
+}
+static DEVICE_ATTR_RW(ppt_fppt);
 
 /* Tunable: PPT APU SPPT *****************************************************/
 static ssize_t ppt_apu_sppt_store(struct device *dev,
@@ -1122,11 +1151,21 @@ static ssize_t ppt_apu_sppt_store(struct device *dev,
 		return -EIO;
 	}
 
+	asus->ppt_apu_sppt = value;
 	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "ppt_apu_sppt");
 
 	return count;
 }
-static DEVICE_ATTR_WO(ppt_apu_sppt);
+
+static ssize_t ppt_apu_sppt_show(struct device *dev,
+			     struct device_attribute *attr,
+			     char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%d\n", asus->ppt_apu_sppt);
+}
+static DEVICE_ATTR_RW(ppt_apu_sppt);
 
 /* Tunable: PPT platform SPPT ************************************************/
 static ssize_t ppt_platform_sppt_store(struct device *dev,
@@ -1156,11 +1195,21 @@ static ssize_t ppt_platform_sppt_store(struct device *dev,
 		return -EIO;
 	}
 
+	asus->ppt_platform_sppt = value;
 	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "ppt_platform_sppt");
 
 	return count;
 }
-static DEVICE_ATTR_WO(ppt_platform_sppt);
+
+static ssize_t ppt_platform_sppt_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%d\n", asus->ppt_platform_sppt);
+}
+static DEVICE_ATTR_RW(ppt_platform_sppt);
 
 /* Tunable: NVIDIA dynamic boost *********************************************/
 static ssize_t nv_dynamic_boost_store(struct device *dev,
@@ -1190,11 +1239,21 @@ static ssize_t nv_dynamic_boost_store(struct device *dev,
 		return -EIO;
 	}
 
+	asus->nv_dynamic_boost = value;
 	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "nv_dynamic_boost");
 
 	return count;
 }
-static DEVICE_ATTR_WO(nv_dynamic_boost);
+
+static ssize_t nv_dynamic_boost_show(struct device *dev,
+				      struct device_attribute *attr,
+				      char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%d\n", asus->nv_dynamic_boost);
+}
+static DEVICE_ATTR_RW(nv_dynamic_boost);
 
 /* Tunable: NVIDIA temperature target ****************************************/
 static ssize_t nv_temp_target_store(struct device *dev,
@@ -1224,11 +1283,21 @@ static ssize_t nv_temp_target_store(struct device *dev,
 		return -EIO;
 	}
 
+	asus->nv_temp_target = value;
 	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "nv_temp_target");
 
 	return count;
 }
-static DEVICE_ATTR_WO(nv_temp_target);
+
+static ssize_t nv_temp_target_show(struct device *dev,
+				     struct device_attribute *attr,
+				     char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%d\n", asus->nv_temp_target);
+}
+static DEVICE_ATTR_RW(nv_temp_target);
 
 /* Battery ********************************************************************/
 
@@ -4277,19 +4346,19 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
 	else if (attr == &dev_attr_throttle_thermal_policy.attr)
 		ok = asus->throttle_thermal_policy_available;
 	else if (attr == &dev_attr_ppt_pl2_sppt.attr)
-		ok = asus->ppt_pl2_sppt_available;
+		devid = ASUS_WMI_DEVID_PPT_PL2_SPPT;
 	else if (attr == &dev_attr_ppt_pl1_spl.attr)
-		ok = asus->ppt_pl1_spl_available;
+		devid = ASUS_WMI_DEVID_PPT_PL1_SPL;
 	else if (attr == &dev_attr_ppt_fppt.attr)
-		ok = asus->ppt_fppt_available;
+		devid = ASUS_WMI_DEVID_PPT_FPPT;
 	else if (attr == &dev_attr_ppt_apu_sppt.attr)
-		ok = asus->ppt_apu_sppt_available;
+		devid = ASUS_WMI_DEVID_PPT_APU_SPPT;
 	else if (attr == &dev_attr_ppt_platform_sppt.attr)
-		ok = asus->ppt_plat_sppt_available;
+		devid = ASUS_WMI_DEVID_PPT_PLAT_SPPT;
 	else if (attr == &dev_attr_nv_dynamic_boost.attr)
-		ok = asus->nv_dyn_boost_available;
+		devid = ASUS_WMI_DEVID_NV_DYN_BOOST;
 	else if (attr == &dev_attr_nv_temp_target.attr)
-		ok = asus->nv_temp_tgt_available;
+		devid = ASUS_WMI_DEVID_NV_THERM_TARGET;
 	else if (attr == &dev_attr_boot_sound.attr)
 		devid = ASUS_WMI_DEVID_BOOT_SOUND;
 	else if (attr == &dev_attr_panel_od.attr)
@@ -4535,18 +4604,20 @@ static int asus_wmi_add(struct platform_device *pdev)
 	if (err)
 		goto fail_platform;
 
+	/* ensure defaults for tunables */
+	asus->ppt_pl2_sppt = 5;
+	asus->ppt_pl1_spl = 5;
+	asus->ppt_apu_sppt = 5;
+	asus->ppt_platform_sppt = 5;
+	asus->ppt_fppt = 5;
+	asus->nv_dynamic_boost = 5;
+	asus->nv_temp_target = 75;
+
 	asus->charge_mode_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_CHARGE_MODE);
 	asus->egpu_enable_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_EGPU);
 	asus->egpu_connect_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_EGPU_CONNECTED);
 	asus->dgpu_disable_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_DGPU);
 	asus->kbd_rgb_state_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_STATE);
-	asus->ppt_pl2_sppt_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PPT_PL2_SPPT);
-	asus->ppt_pl1_spl_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PPT_PL1_SPL);
-	asus->ppt_fppt_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PPT_FPPT);
-	asus->ppt_apu_sppt_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PPT_APU_SPPT);
-	asus->ppt_plat_sppt_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PPT_PLAT_SPPT);
-	asus->nv_dyn_boost_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_NV_DYN_BOOST);
-	asus->nv_temp_tgt_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_NV_THERM_TARGET);
 	asus->panel_overdrive_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PANEL_OD);
 	asus->ally_mcu_usb_switch = acpi_has_method(NULL, ASUS_USB0_PWR_EC0_CSEE)
 						&& dmi_match(DMI_BOARD_NAME, "RC71L");
-- 
2.44.0

Re: [PATCH v2 5/9] platform/x86: asus-wmi: store a min default for ppt options

[Ilpo Järvinen]

On Tue, 2 Apr 2024, Luke D. Jones wrote:

> Laptops with any of the ppt or nv tunables default to the minimum setting
> on boot so we can safely assume a stored value is correct.
> 
> This patch adds storing of those values in the local struct, and enables
> reading of those values back. To prevent creating a series of byte holes
> in the struct the "<name>_available" bool is removed and
> `asus_sysfs_is_visible()` uses the `ASUS_WMI_DEVID_<name>` directly.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>  drivers/platform/x86/asus-wmi.c | 127 +++++++++++++++++++++++++-------
>  1 file changed, 99 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index b795f9c1941f..c80afa385532 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -262,13 +262,13 @@ struct asus_wmi {
>  	u32 gpu_mux_dev;
>  
>  	/* Tunables provided by ASUS for gaming laptops */
> -	bool ppt_pl2_sppt_available;
> -	bool ppt_pl1_spl_available;
> -	bool ppt_apu_sppt_available;
> -	bool ppt_plat_sppt_available;
> -	bool ppt_fppt_available;
> -	bool nv_dyn_boost_available;
> -	bool nv_temp_tgt_available;
> +	u32 ppt_pl2_sppt;
> +	u32 ppt_pl1_spl;
> +	u32 ppt_apu_sppt;
> +	u32 ppt_platform_sppt;
> +	u32 ppt_fppt;
> +	u32 nv_dynamic_boost;
> +	u32 nv_temp_target;
>  
>  	u32 kbd_rgb_dev;
>  	bool kbd_rgb_state_available;
> @@ -1020,11 +1020,21 @@ static ssize_t ppt_pl2_sppt_store(struct device *dev,
>  		return -EIO;
>  	}
>  
> +	asus->ppt_pl2_sppt = value;
>  	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "ppt_pl2_sppt");
>  
>  	return count;
>  }
> -static DEVICE_ATTR_WO(ppt_pl2_sppt);
> +
> +static ssize_t ppt_pl2_sppt_show(struct device *dev,
> +				       struct device_attribute *attr,
> +				       char *buf)
> +{
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%d\n", asus->ppt_pl2_sppt);

Use %u for u32 values. This applies to all sysfs_emits in this patch.

-- 
 i.

Re: [PATCH v2 5/9] platform/x86: asus-wmi: store a min default for ppt options

[Luke Jones]

On Tuesday, 2 April 2024 11:49:50 PM NZDT Ilpo Järvinen wrote:
> On Tue, 2 Apr 2024, Luke D. Jones wrote:
> > Laptops with any of the ppt or nv tunables default to the minimum setting
> > on boot so we can safely assume a stored value is correct.
> > 
> > This patch adds storing of those values in the local struct, and enables
> > reading of those values back. To prevent creating a series of byte holes
> > in the struct the "<name>_available" bool is removed and
> > `asus_sysfs_is_visible()` uses the `ASUS_WMI_DEVID_<name>` directly.
> > 
> > Signed-off-by: Luke D. Jones <luke@ljones.dev>
> > ---
> > 
> >  drivers/platform/x86/asus-wmi.c | 127 +++++++++++++++++++++++++-------
> >  1 file changed, 99 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/asus-wmi.c
> > b/drivers/platform/x86/asus-wmi.c index b795f9c1941f..c80afa385532 100644
> > --- a/drivers/platform/x86/asus-wmi.c
> > +++ b/drivers/platform/x86/asus-wmi.c
> > @@ -262,13 +262,13 @@ struct asus_wmi {
> > 
> >  	u32 gpu_mux_dev;
> >  	
> >  	/* Tunables provided by ASUS for gaming laptops */
> > 
> > -	bool ppt_pl2_sppt_available;
> > -	bool ppt_pl1_spl_available;
> > -	bool ppt_apu_sppt_available;
> > -	bool ppt_plat_sppt_available;
> > -	bool ppt_fppt_available;
> > -	bool nv_dyn_boost_available;
> > -	bool nv_temp_tgt_available;
> > +	u32 ppt_pl2_sppt;
> > +	u32 ppt_pl1_spl;
> > +	u32 ppt_apu_sppt;
> > +	u32 ppt_platform_sppt;
> > +	u32 ppt_fppt;
> > +	u32 nv_dynamic_boost;
> > +	u32 nv_temp_target;
> > 
> >  	u32 kbd_rgb_dev;
> >  	bool kbd_rgb_state_available;
> > 
> > @@ -1020,11 +1020,21 @@ static ssize_t ppt_pl2_sppt_store(struct device
> > *dev,> 
> >  		return -EIO;
> >  	
> >  	}
> > 
> > +	asus->ppt_pl2_sppt = value;
> > 
> >  	sysfs_notify(&asus->platform_device->dev.kobj, NULL, 
"ppt_pl2_sppt");
> >  	
> >  	return count;
> >  
> >  }
> > 
> > -static DEVICE_ATTR_WO(ppt_pl2_sppt);
> > +
> > +static ssize_t ppt_pl2_sppt_show(struct device *dev,
> > +				       struct device_attribute 
*attr,
> > +				       char *buf)
> > +{
> > +	struct asus_wmi *asus = dev_get_drvdata(dev);
> > +
> > +	return sysfs_emit(buf, "%d\n", asus->ppt_pl2_sppt);
> 
> Use %u for u32 values. This applies to all sysfs_emits in this patch.

Thought checkpatch or the compiler would catch that. Done.

I've added your reviewed tag due to the simplicity of the change requested. I 
hope this was okay.

Re: [PATCH v2 5/9] platform/x86: asus-wmi: store a min default for ppt options

[Ilpo Järvinen]

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

On Wed, 3 Apr 2024, Luke Jones wrote:

> On Tuesday, 2 April 2024 11:49:50 PM NZDT Ilpo Järvinen wrote:
> > On Tue, 2 Apr 2024, Luke D. Jones wrote:
> > > Laptops with any of the ppt or nv tunables default to the minimum setting
> > > on boot so we can safely assume a stored value is correct.
> > > 
> > > This patch adds storing of those values in the local struct, and enables
> > > reading of those values back. To prevent creating a series of byte holes
> > > in the struct the "<name>_available" bool is removed and
> > > `asus_sysfs_is_visible()` uses the `ASUS_WMI_DEVID_<name>` directly.
> > > 
> > > Signed-off-by: Luke D. Jones <luke@ljones.dev>
> > > ---
> > > 
> > >  drivers/platform/x86/asus-wmi.c | 127 +++++++++++++++++++++++++-------
> > >  1 file changed, 99 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/asus-wmi.c
> > > b/drivers/platform/x86/asus-wmi.c index b795f9c1941f..c80afa385532 100644
> > > --- a/drivers/platform/x86/asus-wmi.c
> > > +++ b/drivers/platform/x86/asus-wmi.c
> > > @@ -262,13 +262,13 @@ struct asus_wmi {
> > > 
> > >  	u32 gpu_mux_dev;
> > >  	
> > >  	/* Tunables provided by ASUS for gaming laptops */
> > > 
> > > -	bool ppt_pl2_sppt_available;
> > > -	bool ppt_pl1_spl_available;
> > > -	bool ppt_apu_sppt_available;
> > > -	bool ppt_plat_sppt_available;
> > > -	bool ppt_fppt_available;
> > > -	bool nv_dyn_boost_available;
> > > -	bool nv_temp_tgt_available;
> > > +	u32 ppt_pl2_sppt;
> > > +	u32 ppt_pl1_spl;
> > > +	u32 ppt_apu_sppt;
> > > +	u32 ppt_platform_sppt;
> > > +	u32 ppt_fppt;
> > > +	u32 nv_dynamic_boost;
> > > +	u32 nv_temp_target;
> > > 
> > >  	u32 kbd_rgb_dev;
> > >  	bool kbd_rgb_state_available;
> > > 
> > > @@ -1020,11 +1020,21 @@ static ssize_t ppt_pl2_sppt_store(struct device
> > > *dev,> 
> > >  		return -EIO;
> > >  	
> > >  	}
> > > 
> > > +	asus->ppt_pl2_sppt = value;
> > > 
> > >  	sysfs_notify(&asus->platform_device->dev.kobj, NULL, 
> "ppt_pl2_sppt");
> > >  	
> > >  	return count;
> > >  
> > >  }
> > > 
> > > -static DEVICE_ATTR_WO(ppt_pl2_sppt);
> > > +
> > > +static ssize_t ppt_pl2_sppt_show(struct device *dev,
> > > +				       struct device_attribute 
> *attr,
> > > +				       char *buf)
> > > +{
> > > +	struct asus_wmi *asus = dev_get_drvdata(dev);
> > > +
> > > +	return sysfs_emit(buf, "%d\n", asus->ppt_pl2_sppt);
> > 
> > Use %u for u32 values. This applies to all sysfs_emits in this patch.
> 
> Thought checkpatch or the compiler would catch that. Done.
> 
> I've added your reviewed tag due to the simplicity of the change requested. I 
> hope this was okay.

Hi,

Please don't add Reviewed-by tags automatically in the future. Its 
meaning is not that the person has looked the patch.

-- 
 i.

[PATCH v2 6/9] platform/x86: asus-wmi: adjust formatting of ppt-<name>() functions

[Luke D. Jones]

Shift the call to dev_get_drvdata() up to top of the function block
in all of the ppt_<name>() functions as part of a minor cleanup.

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 drivers/platform/x86/asus-wmi.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index c80afa385532..1743ff1eb0ac 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -997,11 +997,10 @@ static ssize_t ppt_pl2_sppt_store(struct device *dev,
 				    struct device_attribute *attr,
 				    const char *buf, size_t count)
 {
+	struct asus_wmi *asus = dev_get_drvdata(dev);
 	int result, err;
 	u32 value;
 
-	struct asus_wmi *asus = dev_get_drvdata(dev);
-
 	result = kstrtou32(buf, 10, &value);
 	if (result)
 		return result;
@@ -1041,11 +1040,10 @@ static ssize_t ppt_pl1_spl_store(struct device *dev,
 				    struct device_attribute *attr,
 				    const char *buf, size_t count)
 {
+	struct asus_wmi *asus = dev_get_drvdata(dev);
 	int result, err;
 	u32 value;
 
-	struct asus_wmi *asus = dev_get_drvdata(dev);
-
 	result = kstrtou32(buf, 10, &value);
 	if (result)
 		return result;
@@ -1084,11 +1082,10 @@ static ssize_t ppt_fppt_store(struct device *dev,
 				    struct device_attribute *attr,
 				    const char *buf, size_t count)
 {
+	struct asus_wmi *asus = dev_get_drvdata(dev);
 	int result, err;
 	u32 value;
 
-	struct asus_wmi *asus = dev_get_drvdata(dev);
-
 	result = kstrtou32(buf, 10, &value);
 	if (result)
 		return result;
@@ -1128,11 +1125,10 @@ static ssize_t ppt_apu_sppt_store(struct device *dev,
 				    struct device_attribute *attr,
 				    const char *buf, size_t count)
 {
+	struct asus_wmi *asus = dev_get_drvdata(dev);
 	int result, err;
 	u32 value;
 
-	struct asus_wmi *asus = dev_get_drvdata(dev);
-
 	result = kstrtou32(buf, 10, &value);
 	if (result)
 		return result;
@@ -1172,11 +1168,10 @@ static ssize_t ppt_platform_sppt_store(struct device *dev,
 				    struct device_attribute *attr,
 				    const char *buf, size_t count)
 {
+	struct asus_wmi *asus = dev_get_drvdata(dev);
 	int result, err;
 	u32 value;
 
-	struct asus_wmi *asus = dev_get_drvdata(dev);
-
 	result = kstrtou32(buf, 10, &value);
 	if (result)
 		return result;
@@ -1216,11 +1211,10 @@ static ssize_t nv_dynamic_boost_store(struct device *dev,
 				    struct device_attribute *attr,
 				    const char *buf, size_t count)
 {
+	struct asus_wmi *asus = dev_get_drvdata(dev);
 	int result, err;
 	u32 value;
 
-	struct asus_wmi *asus = dev_get_drvdata(dev);
-
 	result = kstrtou32(buf, 10, &value);
 	if (result)
 		return result;
@@ -1260,11 +1254,10 @@ static ssize_t nv_temp_target_store(struct device *dev,
 				    struct device_attribute *attr,
 				    const char *buf, size_t count)
 {
+	struct asus_wmi *asus = dev_get_drvdata(dev);
 	int result, err;
 	u32 value;
 
-	struct asus_wmi *asus = dev_get_drvdata(dev);
-
 	result = kstrtou32(buf, 10, &value);
 	if (result)
 		return result;
-- 
2.44.0

Re: [PATCH v2 6/9] platform/x86: asus-wmi: adjust formatting of ppt-<name>() functions

[Ilpo Järvinen]

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

On Tue, 2 Apr 2024, Luke D. Jones wrote:

> Shift the call to dev_get_drvdata() up to top of the function block
> in all of the ppt_<name>() functions as part of a minor cleanup.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>

Thanks for doing this,

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

[PATCH v2 7/9] platform/x86: asus-wmi: ROG Ally increase wait time, allow MCU powersave

[Luke D. Jones]

The previous work to allow the MCU to be resumed correctly after sleep
and resume tried to take the shortest possible time. However as work
continues in various other parts of the s2idle subsystems it has shown
that it wasn't entirely reliable.

If the MCU disable/enable call is done correctly the MCU fully removes
its USB endpoints, and this shows as a full USB device reconnection on
resume. When we tried to short this as much as possible sometimes the
MCU doesn't get to complete what it needs to do before going to low-power
and this affected the reconnection.

Through trial it is found that the minimum time required is approx 1200ms
to allow a proper disconnect and disable, and the same amount of time on
resume is required to prevent a rapid disconnect/reconnect happening on
seemingly random occasions. To be safe the time is now 1500ms for msleep.

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 drivers/platform/x86/asus-wmi.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 1743ff1eb0ac..ddf568ef8c5e 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -129,7 +129,7 @@ module_param(fnlock_default, bool, 0444);
 /* Controls the power state of the USB0 hub on ROG Ally which input is on */
 #define ASUS_USB0_PWR_EC0_CSEE "\\_SB.PCI0.SBRG.EC0.CSEE"
 /* 300ms so far seems to produce a reliable result on AC and battery */
-#define ASUS_USB0_PWR_EC0_CSEE_WAIT 300
+#define ASUS_USB0_PWR_EC0_CSEE_WAIT 1500
 
 static const char * const ashs_ids[] = { "ATK4001", "ATK4002", NULL };
 
@@ -4811,6 +4811,7 @@ static int asus_hotk_resume_early(struct device *device)
 	struct asus_wmi *asus = dev_get_drvdata(device);
 
 	if (asus->ally_mcu_usb_switch) {
+		/* sleep required to prevent USB0 being yanked then reappearing rapidly */
 		if (ACPI_FAILURE(acpi_execute_simple_method(NULL, ASUS_USB0_PWR_EC0_CSEE, 0xB8)))
 			dev_err(device, "ROG Ally MCU failed to connect USB dev\n");
 		else
@@ -4822,17 +4823,8 @@ static int asus_hotk_resume_early(struct device *device)
 static int asus_hotk_prepare(struct device *device)
 {
 	struct asus_wmi *asus = dev_get_drvdata(device);
-	int result, err;
 
 	if (asus->ally_mcu_usb_switch) {
-		/* When powersave is enabled it causes many issues with resume of USB hub */
-		result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_MCU_POWERSAVE);
-		if (result == 1) {
-			dev_warn(device, "MCU powersave enabled, disabling to prevent resume issues");
-			err = asus_wmi_set_devstate(ASUS_WMI_DEVID_MCU_POWERSAVE, 0, &result);
-			if (err || result != 1)
-				dev_err(device, "Failed to set MCU powersave mode: %d\n", err);
-		}
 		/* sleep required to ensure USB0 is disabled before sleep continues */
 		if (ACPI_FAILURE(acpi_execute_simple_method(NULL, ASUS_USB0_PWR_EC0_CSEE, 0xB7)))
 			dev_err(device, "ROG Ally MCU failed to disconnect USB dev\n");
-- 
2.44.0

[PATCH v2 8/9] platform/x86: asus-wmi: Add support for MCU powersave

[Luke D. Jones]

Add support for an MCU powersave WMI call. This is intended to set the
MCU in to a low-power mode when sleeping. This mode can cut sleep power
use by around half.

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 .../ABI/testing/sysfs-platform-asus-wmi       | 11 +++-
 drivers/platform/x86/asus-wmi.c               | 50 +++++++++++++++++++
 2 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index 41b92e53e88a..28144371a0f1 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -202,4 +202,13 @@ Contact:	"Luke Jones" <luke@ljones.dev>
 Description:
 		Set if the BIOS POST sound is played on boot.
 			* 0 - False,
-			* 1 - True
\ No newline at end of file
+			* 1 - True
+
+What:		/sys/devices/platform/<platform>/mcu_powersave
+Date:		Apr 2024
+KernelVersion:	6.10
+Contact:	"Luke Jones" <luke@ljones.dev>
+Description:
+		Set if the MCU can go in to low-power mode on system sleep
+			* 0 - False,
+			* 1 - True
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index ddf568ef8c5e..cf872eed0986 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -1292,6 +1292,53 @@ static ssize_t nv_temp_target_show(struct device *dev,
 }
 static DEVICE_ATTR_RW(nv_temp_target);
 
+/* Ally MCU Powersave ********************************************************/
+static ssize_t mcu_powersave_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+	int result;
+
+	result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_MCU_POWERSAVE);
+	if (result < 0)
+		return result;
+
+	return sysfs_emit(buf, "%d\n", result);
+}
+
+static ssize_t mcu_powersave_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	int result, err;
+	u32 enable;
+
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	result = kstrtou32(buf, 10, &enable);
+	if (result)
+		return result;
+
+	if (enable > 1)
+		return -EINVAL;
+
+	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_MCU_POWERSAVE, enable, &result);
+	if (err) {
+		pr_warn("Failed to set MCU powersave: %d\n", err);
+		return err;
+	}
+
+	if (result > 1) {
+		pr_warn("Failed to set MCU powersave (result): 0x%x\n", result);
+		return -EIO;
+	}
+
+	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "mcu_powersave");
+
+	return count;
+}
+static DEVICE_ATTR_RW(mcu_powersave);
+
 /* Battery ********************************************************************/
 
 /* The battery maximum charging percentage */
@@ -4299,6 +4346,7 @@ static struct attribute *platform_attributes[] = {
 	&dev_attr_ppt_platform_sppt.attr,
 	&dev_attr_nv_dynamic_boost.attr,
 	&dev_attr_nv_temp_target.attr,
+	&dev_attr_mcu_powersave.attr,
 	&dev_attr_boot_sound.attr,
 	&dev_attr_panel_od.attr,
 	&dev_attr_mini_led_mode.attr,
@@ -4352,6 +4400,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
 		devid = ASUS_WMI_DEVID_NV_DYN_BOOST;
 	else if (attr == &dev_attr_nv_temp_target.attr)
 		devid = ASUS_WMI_DEVID_NV_THERM_TARGET;
+	else if (attr == &dev_attr_mcu_powersave.attr)
+		devid = ASUS_WMI_DEVID_MCU_POWERSAVE;
 	else if (attr == &dev_attr_boot_sound.attr)
 		devid = ASUS_WMI_DEVID_BOOT_SOUND;
 	else if (attr == &dev_attr_panel_od.attr)
-- 
2.44.0

Re: [PATCH v2 8/9] platform/x86: asus-wmi: Add support for MCU powersave

[Ilpo Järvinen]

On Tue, 2 Apr 2024, Luke D. Jones wrote:

> Add support for an MCU powersave WMI call. This is intended to set the
> MCU in to a low-power mode when sleeping. This mode can cut sleep power
> use by around half.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>

Hi,

I fail to follow the logic of the patch here. This patch makes it 
configurable which is not bad in itself but what is the reason why a user 
would not always want to cut sleep power usage down? So this sounds like a 
feature that the user wants always enabled if available.

So what are the downsides of enabling it if it's supported?

-- 
 i.


> ---
>  .../ABI/testing/sysfs-platform-asus-wmi       | 11 +++-
>  drivers/platform/x86/asus-wmi.c               | 50 +++++++++++++++++++
>  2 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> index 41b92e53e88a..28144371a0f1 100644
> --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> @@ -202,4 +202,13 @@ Contact:	"Luke Jones" <luke@ljones.dev>
>  Description:
>  		Set if the BIOS POST sound is played on boot.
>  			* 0 - False,
> -			* 1 - True
> \ No newline at end of file
> +			* 1 - True
> +
> +What:		/sys/devices/platform/<platform>/mcu_powersave
> +Date:		Apr 2024
> +KernelVersion:	6.10
> +Contact:	"Luke Jones" <luke@ljones.dev>
> +Description:
> +		Set if the MCU can go in to low-power mode on system sleep
> +			* 0 - False,
> +			* 1 - True
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index ddf568ef8c5e..cf872eed0986 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -1292,6 +1292,53 @@ static ssize_t nv_temp_target_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(nv_temp_target);
>  
> +/* Ally MCU Powersave ********************************************************/
> +static ssize_t mcu_powersave_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +	int result;
> +
> +	result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_MCU_POWERSAVE);
> +	if (result < 0)
> +		return result;
> +
> +	return sysfs_emit(buf, "%d\n", result);
> +}
> +
> +static ssize_t mcu_powersave_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t count)
> +{
> +	int result, err;
> +	u32 enable;
> +
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +	result = kstrtou32(buf, 10, &enable);
> +	if (result)
> +		return result;
> +
> +	if (enable > 1)
> +		return -EINVAL;
> +
> +	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_MCU_POWERSAVE, enable, &result);
> +	if (err) {
> +		pr_warn("Failed to set MCU powersave: %d\n", err);
> +		return err;
> +	}
> +
> +	if (result > 1) {
> +		pr_warn("Failed to set MCU powersave (result): 0x%x\n", result);
> +		return -EIO;
> +	}
> +
> +	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "mcu_powersave");
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(mcu_powersave);
> +
>  /* Battery ********************************************************************/
>  
>  /* The battery maximum charging percentage */
> @@ -4299,6 +4346,7 @@ static struct attribute *platform_attributes[] = {
>  	&dev_attr_ppt_platform_sppt.attr,
>  	&dev_attr_nv_dynamic_boost.attr,
>  	&dev_attr_nv_temp_target.attr,
> +	&dev_attr_mcu_powersave.attr,
>  	&dev_attr_boot_sound.attr,
>  	&dev_attr_panel_od.attr,
>  	&dev_attr_mini_led_mode.attr,
> @@ -4352,6 +4400,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
>  		devid = ASUS_WMI_DEVID_NV_DYN_BOOST;
>  	else if (attr == &dev_attr_nv_temp_target.attr)
>  		devid = ASUS_WMI_DEVID_NV_THERM_TARGET;
> +	else if (attr == &dev_attr_mcu_powersave.attr)
> +		devid = ASUS_WMI_DEVID_MCU_POWERSAVE;
>  	else if (attr == &dev_attr_boot_sound.attr)
>  		devid = ASUS_WMI_DEVID_BOOT_SOUND;
>  	else if (attr == &dev_attr_panel_od.attr)
> 

Re: [PATCH v2 8/9] platform/x86: asus-wmi: Add support for MCU powersave

[Luke Jones]

On Wednesday, 3 April 2024 12:01:15 AM NZDT Ilpo Järvinen wrote:
> On Tue, 2 Apr 2024, Luke D. Jones wrote:
> > Add support for an MCU powersave WMI call. This is intended to set the
> > MCU in to a low-power mode when sleeping. This mode can cut sleep power
> > use by around half.
> > 
> > Signed-off-by: Luke D. Jones <luke@ljones.dev>
> 
> Hi,
> 
> I fail to follow the logic of the patch here. This patch makes it
> configurable which is not bad in itself but what is the reason why a user
> would not always want to cut sleep power usage down? So this sounds like a
> feature that the user wants always enabled if available.
> 
> So what are the downsides of enabling it if it's supported?

Honestly, I'm not sure. Previously it was a source of issues but with recent 
bios and more work in the various gaming-handheld distros it's much less of a 
problem. The issue before was that the MCU would appear every second suspend 
due to the suspend sequence being more parallel compared to windows being 
sequential - the acpi calls related to this would "unplug" the USB devices 
that are related to the n-key (keyboard, same internal dev as laptops) and not 
complete it before suspend. And then on resume it was unreliable.

I worked around this by calling the unplug very early, and trying to "plug" 
super early also so that subsystems wouldn't notice the absence of the device 
and create new devices + paths on add. Most of the requirement for that came 
from some userspace programs unable to handle the unplug/plug part, but also 
bad device state occurring.

But now with the forced wait for the device to finish its task, and then the 
forced wait before letting it add itself back everything is fine - although it 
does mean everything sees a device properly unplugged and plugged.

All of the above is to say that the "powersave" function was also part of the 
issue as delayed things further and we couldn't add the device back before 
subsystems noticed.

Distros like bazzite and chimeraOS are now using this patch, and I'd like to 
leave it to them to set a default for now. If it turns out everything is 
indeed safe as houses then we can adjust the kernel default.

A side-note I think is that because there is a forced wait time due to unable 
to use the right acpi path, the old excuse of "but users might want the device 
to wake faster by turning off powersave" doesn't really apply now.

I will discuss with the main stakeholders, but for now can we accept as is? If 
changes are required we'll get them done before the merge window.

> 
> > ---
> > 
> >  .../ABI/testing/sysfs-platform-asus-wmi       | 11 +++-
> >  drivers/platform/x86/asus-wmi.c               | 50 +++++++++++++++++++
> >  2 files changed, 60 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> > b/Documentation/ABI/testing/sysfs-platform-asus-wmi index
> > 41b92e53e88a..28144371a0f1 100644
> > --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> > +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> > @@ -202,4 +202,13 @@ Contact:	"Luke Jones" <luke@ljones.dev>
> > 
> >  Description:
> >  		Set if the BIOS POST sound is played on boot.
> >  		
> >  			* 0 - False,
> > 
> > -			* 1 - True
> > \ No newline at end of file
> > +			* 1 - True
> > +
> > +What:		/sys/devices/platform/<platform>/mcu_powersave
> > +Date:		Apr 2024
> > +KernelVersion:	6.10
> > +Contact:	"Luke Jones" <luke@ljones.dev>
> > +Description:
> > +		Set if the MCU can go in to low-power mode on system 
sleep
> > +			* 0 - False,
> > +			* 1 - True
> > diff --git a/drivers/platform/x86/asus-wmi.c
> > b/drivers/platform/x86/asus-wmi.c index ddf568ef8c5e..cf872eed0986 100644
> > --- a/drivers/platform/x86/asus-wmi.c
> > +++ b/drivers/platform/x86/asus-wmi.c
> > @@ -1292,6 +1292,53 @@ static ssize_t nv_temp_target_show(struct device
> > *dev,> 
> >  }
> >  static DEVICE_ATTR_RW(nv_temp_target);
> > 
> > +/* Ally MCU Powersave
> > ********************************************************/ +static ssize_t
> > mcu_powersave_show(struct device *dev,
> > +				   struct device_attribute *attr, 
char *buf)
> > +{
> > +	struct asus_wmi *asus = dev_get_drvdata(dev);
> > +	int result;
> > +
> > +	result = asus_wmi_get_devstate_simple(asus,
> > ASUS_WMI_DEVID_MCU_POWERSAVE); +	if (result < 0)
> > +		return result;
> > +
> > +	return sysfs_emit(buf, "%d\n", result);
> > +}
> > +
> > +static ssize_t mcu_powersave_store(struct device *dev,
> > +				    struct device_attribute *attr,
> > +				    const char *buf, size_t count)
> > +{
> > +	int result, err;
> > +	u32 enable;
> > +
> > +	struct asus_wmi *asus = dev_get_drvdata(dev);
> > +
> > +	result = kstrtou32(buf, 10, &enable);
> > +	if (result)
> > +		return result;
> > +
> > +	if (enable > 1)
> > +		return -EINVAL;
> > +
> > +	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_MCU_POWERSAVE, enable,
> > &result); +	if (err) {
> > +		pr_warn("Failed to set MCU powersave: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	if (result > 1) {
> > +		pr_warn("Failed to set MCU powersave (result): 0x%x\n", 
result);
> > +		return -EIO;
> > +	}
> > +
> > +	sysfs_notify(&asus->platform_device->dev.kobj, NULL, 
"mcu_powersave");
> > +
> > +	return count;
> > +}
> > +static DEVICE_ATTR_RW(mcu_powersave);
> > +
> > 
> >  /* Battery
> >  ********************************************************************/
> >  
> >  /* The battery maximum charging percentage */
> > 
> > @@ -4299,6 +4346,7 @@ static struct attribute *platform_attributes[] = {
> > 
> >  	&dev_attr_ppt_platform_sppt.attr,
> >  	&dev_attr_nv_dynamic_boost.attr,
> >  	&dev_attr_nv_temp_target.attr,
> > 
> > +	&dev_attr_mcu_powersave.attr,
> > 
> >  	&dev_attr_boot_sound.attr,
> >  	&dev_attr_panel_od.attr,
> >  	&dev_attr_mini_led_mode.attr,
> > 
> > @@ -4352,6 +4400,8 @@ static umode_t asus_sysfs_is_visible(struct kobject
> > *kobj,> 
> >  		devid = ASUS_WMI_DEVID_NV_DYN_BOOST;
> >  	
> >  	else if (attr == &dev_attr_nv_temp_target.attr)
> >  	
> >  		devid = ASUS_WMI_DEVID_NV_THERM_TARGET;
> > 
> > +	else if (attr == &dev_attr_mcu_powersave.attr)
> > +		devid = ASUS_WMI_DEVID_MCU_POWERSAVE;
> > 
> >  	else if (attr == &dev_attr_boot_sound.attr)
> >  	
> >  		devid = ASUS_WMI_DEVID_BOOT_SOUND;
> >  	
> >  	else if (attr == &dev_attr_panel_od.attr)

Re: [PATCH v2 8/9] platform/x86: asus-wmi: Add support for MCU powersave

[Ilpo Järvinen]

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

On Wed, 3 Apr 2024, Luke Jones wrote:

> On Wednesday, 3 April 2024 12:01:15 AM NZDT Ilpo Järvinen wrote:
> > On Tue, 2 Apr 2024, Luke D. Jones wrote:
> > > Add support for an MCU powersave WMI call. This is intended to set the
> > > MCU in to a low-power mode when sleeping. This mode can cut sleep power
> > > use by around half.
> > > 
> > > Signed-off-by: Luke D. Jones <luke@ljones.dev>
> > 
> > Hi,
> > 
> > I fail to follow the logic of the patch here. This patch makes it
> > configurable which is not bad in itself but what is the reason why a user
> > would not always want to cut sleep power usage down? So this sounds like a
> > feature that the user wants always enabled if available.
> > 
> > So what are the downsides of enabling it if it's supported?
> 
> Honestly, I'm not sure. Previously it was a source of issues but with recent 
> bios and more work in the various gaming-handheld distros it's much less of a 
> problem. The issue before was that the MCU would appear every second suspend 
> due to the suspend sequence being more parallel compared to windows being 
> sequential - the acpi calls related to this would "unplug" the USB devices 
> that are related to the n-key (keyboard, same internal dev as laptops) and not 
> complete it before suspend. And then on resume it was unreliable.
> 
> I worked around this by calling the unplug very early, and trying to "plug" 
> super early also so that subsystems wouldn't notice the absence of the device 
> and create new devices + paths on add. Most of the requirement for that came 
> from some userspace programs unable to handle the unplug/plug part, but also 
> bad device state occurring.
> 
> But now with the forced wait for the device to finish its task, and then the 
> forced wait before letting it add itself back everything is fine - although it 
> does mean everything sees a device properly unplugged and plugged.
> 
> All of the above is to say that the "powersave" function was also part of the 
> issue as delayed things further and we couldn't add the device back before 
> subsystems noticed.
> 
> Distros like bazzite and chimeraOS are now using this patch, and I'd like to 
> leave it to them to set a default for now. If it turns out everything is 
> indeed safe as houses then we can adjust the kernel default.
> 
> A side-note I think is that because there is a forced wait time due to unable 
> to use the right acpi path, the old excuse of "but users might want the device 
> to wake faster by turning off powersave" doesn't really apply now.
> 
> I will discuss with the main stakeholders, but for now can we accept as is? If 
> changes are required we'll get them done before the merge window.

Yes, I think it is okay to make it configurable first and then look 
separately into making it default on.

-- 
 i.

[PATCH v2 9/9] platform/x86: asus-wmi: cleanup main struct to avoid some holes

[Luke D. Jones]

Reorganises some attr-available calls to remove a few unrequired
booleans in the main driver struct which combined with some
reorganisation prevents a series of large holes seen with pahole.

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 drivers/platform/x86/asus-wmi.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index cf872eed0986..c8a8ddaeff29 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -243,6 +243,9 @@ struct asus_wmi {
 	u32 tablet_switch_dev_id;
 	bool tablet_switch_inverted;
 
+	/* The ROG Ally device requires the MCU USB device be disconnected before suspend */
+	bool ally_mcu_usb_switch;
+
 	enum fan_type fan_type;
 	enum fan_type gpu_fan_type;
 	enum fan_type mid_fan_type;
@@ -255,9 +258,7 @@ struct asus_wmi {
 	u8 fan_boost_mode_mask;
 	u8 fan_boost_mode;
 
-	bool charge_mode_available;
 	bool egpu_enable_available;
-	bool egpu_connect_available;
 	bool dgpu_disable_available;
 	u32 gpu_mux_dev;
 
@@ -298,9 +299,6 @@ struct asus_wmi {
 
 	bool fnlock_locked;
 
-	/* The ROG Ally device requires the MCU USB device be disconnected before suspend */
-	bool ally_mcu_usb_switch;
-
 	struct asus_wmi_debug debug;
 
 	struct asus_wmi_driver *driver;
@@ -4373,11 +4371,11 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
 	else if (attr == &dev_attr_als_enable.attr)
 		devid = ASUS_WMI_DEVID_ALS_ENABLE;
 	else if (attr == &dev_attr_charge_mode.attr)
-		ok = asus->charge_mode_available;
+		devid = ASUS_WMI_DEVID_CHARGE_MODE;
 	else if (attr == &dev_attr_egpu_enable.attr)
 		ok = asus->egpu_enable_available;
 	else if (attr == &dev_attr_egpu_connected.attr)
-		ok = asus->egpu_connect_available;
+		devid = ASUS_WMI_DEVID_EGPU_CONNECTED;
 	else if (attr == &dev_attr_dgpu_disable.attr)
 		ok = asus->dgpu_disable_available;
 	else if (attr == &dev_attr_gpu_mux_mode.attr)
@@ -4405,7 +4403,7 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
 	else if (attr == &dev_attr_boot_sound.attr)
 		devid = ASUS_WMI_DEVID_BOOT_SOUND;
 	else if (attr == &dev_attr_panel_od.attr)
-		ok = asus->panel_overdrive_available;
+		devid = ASUS_WMI_DEVID_PANEL_OD;
 	else if (attr == &dev_attr_mini_led_mode.attr)
 		ok = asus->mini_led_dev_id != 0;
 	else if (attr == &dev_attr_available_mini_led_mode.attr)
@@ -4656,12 +4654,9 @@ static int asus_wmi_add(struct platform_device *pdev)
 	asus->nv_dynamic_boost = 5;
 	asus->nv_temp_target = 75;
 
-	asus->charge_mode_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_CHARGE_MODE);
 	asus->egpu_enable_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_EGPU);
-	asus->egpu_connect_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_EGPU_CONNECTED);
 	asus->dgpu_disable_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_DGPU);
 	asus->kbd_rgb_state_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_STATE);
-	asus->panel_overdrive_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PANEL_OD);
 	asus->ally_mcu_usb_switch = acpi_has_method(NULL, ASUS_USB0_PWR_EC0_CSEE)
 						&& dmi_match(DMI_BOARD_NAME, "RC71L");
 
-- 
2.44.0

Re: [PATCH v2 9/9] platform/x86: asus-wmi: cleanup main struct to avoid some holes

[Ilpo Järvinen]

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

On Tue, 2 Apr 2024, Luke D. Jones wrote:

> Reorganises some attr-available calls to remove a few unrequired
> booleans in the main driver struct which combined with some
> reorganisation prevents a series of large holes seen with pahole.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.