[PATCH 0/6] asus-wmi: cleanup dgpu_disable, egpu_enable, panel_od

[PATCH 0/6] asus-wmi: cleanup dgpu_disable, egpu_enable, panel_od

[Luke D. Jones]

This patch series does two things for previously added features:
- dgpu_disable
- egpu_enable
- panel_od

The fixes add missing documentation, and the refactors vastly clean up how
the features work, including reading the values from WMI methods on *_show()
and checking the result correctly (these methods return 1 on success).

Luke D. Jones (6):
  Fixes 98829e84dc67 ("asus-wmi: Add dgpu disable method")
  Fixes: 382b91db8044 ("asus-wmi: Add egpu enable method")
  Fixes: ca91ea34778f ("asus-wmi: Add panel overdrive functionality")
  asus-wmi: Refactor disable_gpu attribute
  asus-wmi: Refactor egpu_enable attribute
  asus-wmi: Refactor panel_od attribute

 .../ABI/testing/sysfs-platform-asus-wmi       |  28 +++
 drivers/platform/x86/asus-wmi.c               | 231 ++++++------------
 2 files changed, 103 insertions(+), 156 deletions(-)

-- 
2.37.1

[PATCH 1/6] Fixes 98829e84dc67 ("asus-wmi: Add dgpu disable method")

[Luke D. Jones]

The dgpu_disable attribute was not documented, this adds the
required documentation.

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 Documentation/ABI/testing/sysfs-platform-asus-wmi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index 04885738cf15..0f932fd60f4a 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -57,3 +57,12 @@ Description:
 			* 0 - default,
 			* 1 - overboost,
 			* 2 - silent
+
+What:          /sys/devices/platform/<platform>/dgpu_disable
+Date:          Aug 2022
+KernelVersion: 5.17
+Contact:       "Luke Jones" <luke@ljones.dev>
+Description:
+               Disable discrete GPU:
+                       * 0 - Enable dGPU,
+                       * 1 - Disable dGPU,
\ No newline at end of file
-- 
2.37.1

Re: [PATCH 1/6] Fixes 98829e84dc67 ("asus-wmi: Add dgpu disable method")

[Hans de Goede]

Hi Luke,

On 8/13/22 00:25, Luke D. Jones wrote:
> The dgpu_disable attribute was not documented, this adds the
> required documentation.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>

Thanks for the patch. Note that the Fixes tag should be above your
signed-off-by and then the patch should otherwise have a normal
subject + body. I've changed the commit msg to the following
while merging this:

"""
platform/x86: asus-wmi: Document the dgpu_disable sysfs attribute
    
The dgpu_disable attribute was not documented, this adds the
required documentation.
    
Fixes 98829e84dc67 ("asus-wmi: Add dgpu disable method")
Signed-off-by: Luke D. Jones <luke@ljones.dev>
"""

and I will make similar changes to patch 2/6 and 3/6
> ---
>  Documentation/ABI/testing/sysfs-platform-asus-wmi | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> index 04885738cf15..0f932fd60f4a 100644
> --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> @@ -57,3 +57,12 @@ Description:
>  			* 0 - default,
>  			* 1 - overboost,
>  			* 2 - silent
> +
> +What:          /sys/devices/platform/<platform>/dgpu_disable
> +Date:          Aug 2022
> +KernelVersion: 5.17
> +Contact:       "Luke Jones" <luke@ljones.dev>
> +Description:
> +               Disable discrete GPU:
> +                       * 0 - Enable dGPU,
> +                       * 1 - Disable dGPU,
> \ No newline at end of file

Next time please make sure the file always ends with a newline
even in intermediate patches.

Regards,

Hans

Re: [PATCH 1/6] Fixes 98829e84dc67 ("asus-wmi: Add dgpu disable method")

[Hans de Goede]

Hi,

On 8/15/22 15:25, Hans de Goede wrote:
> Hi Luke,
> 
> On 8/13/22 00:25, Luke D. Jones wrote:
>> The dgpu_disable attribute was not documented, this adds the
>> required documentation.
>>
>> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> 
> Thanks for the patch. Note that the Fixes tag should be above your
> signed-off-by and then the patch should otherwise have a normal
> subject + body. I've changed the commit msg to the following
> while merging this:
> 
> """
> platform/x86: asus-wmi: Document the dgpu_disable sysfs attribute
>     
> The dgpu_disable attribute was not documented, this adds the
> required documentation.
>     
> Fixes 98829e84dc67 ("asus-wmi: Add dgpu disable method")
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> """

While merging this I noticed that this is also using spaces
instead of tabs for indentation, where as the rest of
the file is using tabs.

I've also fixed this up while merging, but next time
please make sure to use spaces.

I will make similar spaces -> tabs changes to patch 2/6 and 3/6

Regards,

Hans




>> ---
>>  Documentation/ABI/testing/sysfs-platform-asus-wmi | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
>> index 04885738cf15..0f932fd60f4a 100644
>> --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
>> +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
>> @@ -57,3 +57,12 @@ Description:
>>  			* 0 - default,
>>  			* 1 - overboost,
>>  			* 2 - silent
>> +
>> +What:          /sys/devices/platform/<platform>/dgpu_disable
>> +Date:          Aug 2022
>> +KernelVersion: 5.17
>> +Contact:       "Luke Jones" <luke@ljones.dev>
>> +Description:
>> +               Disable discrete GPU:
>> +                       * 0 - Enable dGPU,
>> +                       * 1 - Disable dGPU,
>> \ No newline at end of file
> 
> Next time please make sure the file always ends with a newline
> even in intermediate patches.
> 
> Regards,
> 
> Hans
> 
> 

Re: [PATCH 1/6] Fixes 98829e84dc67 ("asus-wmi: Add dgpu disable method")

[Luke Jones]

On Mon, 2022-08-15 at 15:40 +0200, Hans de Goede wrote:
> Hi,
> 
> On 8/15/22 15:25, Hans de Goede wrote:
> > Hi Luke,
> > 
> > On 8/13/22 00:25, Luke D. Jones wrote:
> > > The dgpu_disable attribute was not documented, this adds the
> > > required documentation.
> > > 
> > > Signed-off-by: Luke D. Jones <luke@ljones.dev>
> > 
> > Thanks for the patch. Note that the Fixes tag should be above your
> > signed-off-by and then the patch should otherwise have a normal
> > subject + body. I've changed the commit msg to the following
> > while merging this:
> > 
> > """
> > platform/x86: asus-wmi: Document the dgpu_disable sysfs attribute
> >     
> > The dgpu_disable attribute was not documented, this adds the
> > required documentation.
> >     
> > Fixes 98829e84dc67 ("asus-wmi: Add dgpu disable method")
> > Signed-off-by: Luke D. Jones <luke@ljones.dev>
> > """
> 
> While merging this I noticed that this is also using spaces
> instead of tabs for indentation, where as the rest of
> the file is using tabs.
> 
> I've also fixed this up while merging, but next time
> please make sure to use spaces.
> 
> I will make similar spaces -> tabs changes to patch 2/6 and 3/6
> 

I ran these through ./scripts/checkpatch.pl... I'm really not sure what
happened. I'll triple-check that in future.

> Regards,
> 
> Hans
> 
> 
> 
> 
> > > ---
> > >  Documentation/ABI/testing/sysfs-platform-asus-wmi | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> > > b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> > > index 04885738cf15..0f932fd60f4a 100644
> > > --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> > > +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> > > @@ -57,3 +57,12 @@ Description:
> > >                         * 0 - default,
> > >                         * 1 - overboost,
> > >                         * 2 - silent
> > > +
> > > +What:          /sys/devices/platform/<platform>/dgpu_disable
> > > +Date:          Aug 2022
> > > +KernelVersion: 5.17
> > > +Contact:       "Luke Jones" <luke@ljones.dev>
> > > +Description:
> > > +               Disable discrete GPU:
> > > +                       * 0 - Enable dGPU,
> > > +                       * 1 - Disable dGPU,
> > > \ No newline at end of file
> > 
> > Next time please make sure the file always ends with a newline
> > even in intermediate patches.
> > 
> > Regards,
> > 
> > Hans
> > 
> > 
> 

Re: [PATCH 1/6] Fixes 98829e84dc67 ("asus-wmi: Add dgpu disable method")

[Luke Jones]

> Thanks for the patch. Note that the Fixes tag should be above your
> signed-off-by and then the patch should otherwise have a normal
> subject + body. I've changed the commit msg to the following
> while merging this:
> 
> """
> platform/x86: asus-wmi: Document the dgpu_disable sysfs attribute
>     
> The dgpu_disable attribute was not documented, this adds the
> required documentation.
>     
> Fixes 98829e84dc67 ("asus-wmi: Add dgpu disable method")
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> """
> 
> and I will make similar changes to patch 2/6 and 3/6
> 

Ah thank you, sorry about that, I thought I got it correct Perhaps I
didn't read
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#describe-changes
well enough.

> > ---
> >  Documentation/ABI/testing/sysfs-platform-asus-wmi | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> > b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> > index 04885738cf15..0f932fd60f4a 100644
> > --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> > +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> > @@ -57,3 +57,12 @@ Description:
> >                         * 0 - default,
> >                         * 1 - overboost,
> >                         * 2 - silent
> > +
> > +What:          /sys/devices/platform/<platform>/dgpu_disable
> > +Date:          Aug 2022
> > +KernelVersion: 5.17
> > +Contact:       "Luke Jones" <luke@ljones.dev>
> > +Description:
> > +               Disable discrete GPU:
> > +                       * 0 - Enable dGPU,
> > +                       * 1 - Disable dGPU,
> > \ No newline at end of file
> 
> Next time please make sure the file always ends with a newline
> even in intermediate patches.
> 
> Regards,
> 
> Hans
> 
> 

[PATCH 2/6] Fixes: 382b91db8044 ("asus-wmi: Add egpu enable method")

[Luke D. Jones]

The egpu_enable attribute was not documented, this adds the
required documentation.

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 Documentation/ABI/testing/sysfs-platform-asus-wmi | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index 0f932fd60f4a..b984bdcd0c40 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -65,4 +65,14 @@ Contact:       "Luke Jones" <luke@ljones.dev>
 Description:
                Disable discrete GPU:
                        * 0 - Enable dGPU,
-                       * 1 - Disable dGPU,
\ No newline at end of file
+                       * 1 - Disable dGPU,
+
+What:          /sys/devices/platform/<platform>/egpu_enable
+Date:          Aug 2022
+KernelVersion: 5.17
+Contact:       "Luke Jones" <luke@ljones.dev>
+Description:
+               Enable the external GPU paired with ROG X-Flow laptops.
+               Toggling this setting will also trigger ACPI to disable the dGPU:
+                       * 0 - Disable,
+                       * 1 - Enable,
\ No newline at end of file
-- 
2.37.1

[PATCH 3/6] Fixes: ca91ea34778f ("asus-wmi: Add panel overdrive functionality")

[Luke D. Jones]

The panel_od attribute was not documented, this adds the
required documentation.

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 Documentation/ABI/testing/sysfs-platform-asus-wmi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index b984bdcd0c40..574b5170a37d 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -74,5 +74,14 @@ Contact:       "Luke Jones" <luke@ljones.dev>
 Description:
                Enable the external GPU paired with ROG X-Flow laptops.
                Toggling this setting will also trigger ACPI to disable the dGPU:
+                       * 0 - Disable,
+                       * 1 - Enable,
+
+What:          /sys/devices/platform/<platform>/panel_od
+Date:          Aug 2022
+KernelVersion: 5.17
+Contact:       "Luke Jones" <luke@ljones.dev>
+Description:
+               Enable an LCD response-time boost to reduce or remove ghosting:
                        * 0 - Disable,
                        * 1 - Enable,
\ No newline at end of file
-- 
2.37.1

[PATCH 4/6] asus-wmi: Refactor disable_gpu attribute

[Luke D. Jones]

The settings for these attributes can be read from the device, this
is now done instead of reading a stored value from module. The stored
value is also removed.

This means the simpler asus_wmi_dev_is_present() can be used in
*_check_present() - it is not an error for these methods to be
missing.

The _write() functions have their bodies shifted in to *_store()
which simplifies things further.

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

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 0e7fbed8a50d..a72ecc4e33b7 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -232,7 +232,6 @@ struct asus_wmi {
 	bool egpu_enable;
 
 	bool dgpu_disable_available;
-	bool dgpu_disable;
 
 	bool throttle_thermal_policy_available;
 	u8 throttle_thermal_policy_mode;
@@ -562,47 +561,10 @@ static void lid_flip_tablet_mode_get_state(struct asus_wmi *asus)
 /* dGPU ********************************************************************/
 static int dgpu_disable_check_present(struct asus_wmi *asus)
 {
-	u32 result;
-	int err;
-
 	asus->dgpu_disable_available = false;
 
-	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_DGPU, &result);
-	if (err) {
-		if (err == -ENODEV)
-			return 0;
-		return err;
-	}
-
-	if (result & ASUS_WMI_DSTS_PRESENCE_BIT) {
+	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_DGPU))
 		asus->dgpu_disable_available = true;
-		asus->dgpu_disable = result & ASUS_WMI_DSTS_STATUS_BIT;
-	}
-
-	return 0;
-}
-
-static int dgpu_disable_write(struct asus_wmi *asus)
-{
-	u32 retval;
-	u8 value;
-	int err;
-
-	/* Don't rely on type conversion */
-	value = asus->dgpu_disable ? 1 : 0;
-
-	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_DGPU, value, &retval);
-	if (err) {
-		pr_warn("Failed to set dgpu disable: %d\n", err);
-		return err;
-	}
-
-	if (retval > 1) {
-		pr_warn("Failed to set dgpu disable (retval): 0x%x\n", retval);
-		return -EIO;
-	}
-
-	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "dgpu_disable");
 
 	return 0;
 }
@@ -611,9 +573,13 @@ static ssize_t dgpu_disable_show(struct device *dev,
 				   struct device_attribute *attr, char *buf)
 {
 	struct asus_wmi *asus = dev_get_drvdata(dev);
-	u8 mode = asus->dgpu_disable;
+	int result;
 
-	return sysfs_emit(buf, "%d\n", mode);
+	result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_DGPU);
+	if (result < 0)
+		return result;
+
+	return sysfs_emit(buf, "%d\n", result);
 }
 
 /*
@@ -626,24 +592,33 @@ static ssize_t dgpu_disable_store(struct device *dev,
 				    struct device_attribute *attr,
 				    const char *buf, size_t count)
 {
-	bool disable;
-	int result;
+	int result, err;
+	u32 disable;
 
 	struct asus_wmi *asus = dev_get_drvdata(dev);
 
-	result = kstrtobool(buf, &disable);
+	result = kstrtou32(buf, 10, &disable);
 	if (result)
 		return result;
 
-	asus->dgpu_disable = disable;
+	if (disable > 1)
+		return -EINVAL;
 
-	result = dgpu_disable_write(asus);
-	if (result)
-		return result;
+	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_DGPU, disable, &result);
+	if (err) {
+		pr_warn("Failed to set dgpu disable: %d\n", err);
+		return err;
+	}
+
+	if (result > 1) {
+		pr_warn("Failed to set dgpu disable (result): 0x%x\n", result);
+		return -EIO;
+	}
+
+	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "dgpu_disable");
 
 	return count;
 }
-
 static DEVICE_ATTR_RW(dgpu_disable);
 
 /* eGPU ********************************************************************/
-- 
2.37.1

[PATCH 5/6] asus-wmi: Refactor egpu_enable attribute

[Luke D. Jones]

The settings for these attributes can be read from the device, this
is now done instead of reading a stored value from module. The stored
value is also removed.

This means the simpler asus_wmi_dev_is_present() can be used in
*_check_present() - it is not an error for these methods to be
missing.

The _write() functions have their bodies shifted in to *_store()
which simplifies things further.

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

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index a72ecc4e33b7..87b042fac1ce 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -228,9 +228,7 @@ struct asus_wmi {
 	u8 fan_boost_mode_mask;
 	u8 fan_boost_mode;
 
-	bool egpu_enable_available; // 0 = enable
-	bool egpu_enable;
-
+	bool egpu_enable_available;
 	bool dgpu_disable_available;
 
 	bool throttle_thermal_policy_available;
@@ -624,48 +622,10 @@ static DEVICE_ATTR_RW(dgpu_disable);
 /* eGPU ********************************************************************/
 static int egpu_enable_check_present(struct asus_wmi *asus)
 {
-	u32 result;
-	int err;
-
 	asus->egpu_enable_available = false;
 
-	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_EGPU, &result);
-	if (err) {
-		if (err == -ENODEV)
-			return 0;
-		return err;
-	}
-
-	if (result & ASUS_WMI_DSTS_PRESENCE_BIT) {
+	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_EGPU))
 		asus->egpu_enable_available = true;
-		asus->egpu_enable = result & ASUS_WMI_DSTS_STATUS_BIT;
-	}
-
-	return 0;
-}
-
-static int egpu_enable_write(struct asus_wmi *asus)
-{
-	u32 retval;
-	u8 value;
-	int err;
-
-	/* Don't rely on type conversion */
-	value = asus->egpu_enable ? 1 : 0;
-
-	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_EGPU, value, &retval);
-
-	if (err) {
-		pr_warn("Failed to set egpu disable: %d\n", err);
-		return err;
-	}
-
-	if (retval > 1) {
-		pr_warn("Failed to set egpu disable (retval): 0x%x\n", retval);
-		return -EIO;
-	}
-
-	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "egpu_enable");
 
 	return 0;
 }
@@ -674,9 +634,13 @@ static ssize_t egpu_enable_show(struct device *dev,
 				   struct device_attribute *attr, char *buf)
 {
 	struct asus_wmi *asus = dev_get_drvdata(dev);
-	bool mode = asus->egpu_enable;
+	int result;
+
+	result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_EGPU);
+	if (result < 0)
+		return result;
 
-	return sysfs_emit(buf, "%d\n", mode);
+	return sysfs_emit(buf, "%d\n", result);
 }
 
 /* The ACPI call to enable the eGPU also disables the internal dGPU */
@@ -684,29 +648,33 @@ static ssize_t egpu_enable_store(struct device *dev,
 				    struct device_attribute *attr,
 				    const char *buf, size_t count)
 {
-	bool enable;
-	int result;
+	int result, err;
+	u32 enable;
 
 	struct asus_wmi *asus = dev_get_drvdata(dev);
 
-	result = kstrtobool(buf, &enable);
-	if (result)
-		return result;
+	err = kstrtou32(buf, 10, &enable);
+	if (err)
+		return err;
 
-	asus->egpu_enable = enable;
+	if (enable > 1)
+		return -EINVAL;
 
-	result = egpu_enable_write(asus);
-	if (result)
-		return result;
+	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_EGPU, enable, &result);
+	if (err) {
+		pr_warn("Failed to set egpu disable: %d\n", err);
+		return err;
+	}
 
-	/* Ensure that the kernel status of dgpu is updated */
-	result = dgpu_disable_check_present(asus);
-	if (result)
-		return result;
+	if (result > 1) {
+		pr_warn("Failed to set egpu disable (retval): 0x%x\n", result);
+		return -EIO;
+	}
+
+	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "egpu_enable");
 
 	return count;
 }
-
 static DEVICE_ATTR_RW(egpu_enable);
 
 /* Battery ********************************************************************/
-- 
2.37.1

[PATCH 6/6] asus-wmi: Refactor panel_od attribute

[Luke D. Jones]

The settings for these attributes can be read from the device, this
is now done instead of reading a stored value from module. The stored
value is also removed.

This means the simpler asus_wmi_dev_is_present() can be used in
*_check_present() - it is not an error for these methods to be
missing.

The _write() functions have their bodies shifted in to *_store()
which simplifies things further.

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

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 87b042fac1ce..b2595a2d1b0a 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -245,7 +245,6 @@ struct asus_wmi {
 	bool battery_rsoc_available;
 
 	bool panel_overdrive_available;
-	bool panel_overdrive;
 
 	struct hotplug_slot hotplug_slot;
 	struct mutex hotplug_lock;
@@ -1475,48 +1474,10 @@ static int asus_wmi_rfkill_init(struct asus_wmi *asus)
 /* Panel Overdrive ************************************************************/
 static int panel_od_check_present(struct asus_wmi *asus)
 {
-	u32 result;
-	int err;
-
 	asus->panel_overdrive_available = false;
 
-	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_PANEL_OD, &result);
-	if (err) {
-		if (err == -ENODEV)
-			return 0;
-		return err;
-	}
-
-	if (result & ASUS_WMI_DSTS_PRESENCE_BIT) {
+	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PANEL_OD))
 		asus->panel_overdrive_available = true;
-		asus->panel_overdrive = result & ASUS_WMI_DSTS_STATUS_BIT;
-	}
-
-	return 0;
-}
-
-static int panel_od_write(struct asus_wmi *asus)
-{
-	u32 retval;
-	u8 value;
-	int err;
-
-	/* Don't rely on type conversion */
-	value = asus->panel_overdrive ? 1 : 0;
-
-	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_PANEL_OD, value, &retval);
-
-	if (err) {
-		pr_warn("Failed to set panel overdrive: %d\n", err);
-		return err;
-	}
-
-	if (retval > 1) {
-		pr_warn("Failed to set panel overdrive (retval): 0x%x\n", retval);
-		return -EIO;
-	}
-
-	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "panel_od");
 
 	return 0;
 }
@@ -1525,32 +1486,47 @@ static ssize_t panel_od_show(struct device *dev,
 				   struct device_attribute *attr, char *buf)
 {
 	struct asus_wmi *asus = dev_get_drvdata(dev);
+	int result;
 
-	return sysfs_emit(buf, "%d\n", asus->panel_overdrive);
+	result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_PANEL_OD);
+	if (result < 0)
+		return result;
+
+	return sysfs_emit(buf, "%d\n", result);
 }
 
 static ssize_t panel_od_store(struct device *dev,
 				    struct device_attribute *attr,
 				    const char *buf, size_t count)
 {
-	bool overdrive;
-	int result;
+	int result, err;
+	u32 overdrive;
 
 	struct asus_wmi *asus = dev_get_drvdata(dev);
 
-	result = kstrtobool(buf, &overdrive);
+	result = kstrtou32(buf, 10, &overdrive);
 	if (result)
 		return result;
 
-	asus->panel_overdrive = overdrive;
-	result = panel_od_write(asus);
+	if (overdrive > 1)
+		return -EINVAL;
 
-	if (result)
-		return result;
+	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_PANEL_OD, overdrive, &result);
+
+	if (err) {
+		pr_warn("Failed to set panel overdrive: %d\n", err);
+		return err;
+	}
+
+	if (result > 1) {
+		pr_warn("Failed to set panel overdrive (result): 0x%x\n", result);
+		return -EIO;
+	}
+
+	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "panel_od");
 
 	return count;
 }
-
 static DEVICE_ATTR_RW(panel_od);
 
 /* Quirks *********************************************************************/
-- 
2.37.1

Re: [PATCH 0/6] asus-wmi: cleanup dgpu_disable, egpu_enable, panel_od

[Hans de Goede]

Hi,

On 8/13/22 00:25, Luke D. Jones wrote:
> This patch series does two things for previously added features:
> - dgpu_disable
> - egpu_enable
> - panel_od
> 
> The fixes add missing documentation, and the refactors vastly clean up how
> the features work, including reading the values from WMI methods on *_show()
> and checking the result correctly (these methods return 1 on success).

Thank you for your patch-series, I've applied the series to my
review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans


> 
> Luke D. Jones (6):
>   Fixes 98829e84dc67 ("asus-wmi: Add dgpu disable method")
>   Fixes: 382b91db8044 ("asus-wmi: Add egpu enable method")
>   Fixes: ca91ea34778f ("asus-wmi: Add panel overdrive functionality")
>   asus-wmi: Refactor disable_gpu attribute
>   asus-wmi: Refactor egpu_enable attribute
>   asus-wmi: Refactor panel_od attribute
> 
>  .../ABI/testing/sysfs-platform-asus-wmi       |  28 +++
>  drivers/platform/x86/asus-wmi.c               | 231 ++++++------------
>  2 files changed, 103 insertions(+), 156 deletions(-)
>