[PATCH 0/3] Support for ASUS egpu, dpgu disable, panel overdrive

[PATCH 0/3] Support for ASUS egpu, dpgu disable, panel overdrive

[Luke D. Jones]

This patch series adds support for some functions that are found on newer
ASUS gaming laptops:

- Panel overdrive: Some laptops can drive the LCD matrix slightly faster
  to eliminate or reduce ghosting artifacts

- dGPU disable: ASUS added a function in ACPI to disable or enable the dGPU
  which removes it from the PCI bus. Presumably this was to help prevent
  Windows apps from using the dGPU when the user didn't want them to but
  because of how it works it also means that when rebooted to Linux the dGPU
  no-longer exits. This patch enables a user to echo 0/1 to a WMI path to
  re-enable it (or disable, but the drivers *must* be unloaded first).

- eGPU enable: The ASUS x-flow lpatop has an iGPU, a dGPU, and an optional
  eGPU. This patch enables the user to echo 0/1 to a WMI path to enable or
  disable the eGPU. In ACPI this also appears to remove the dGPU from the
  PCI bus.

All of the above patches have been tested over the course of a few months.
There is a small possibility of user error perhaps, where the user tries to
enable or disable the dGPU/eGPU while drivers are loaded which would cause
a system hang, but it is expected that almost all users would be using the
`asusctl` daemon and dbus methods to manage the above which then eliminates
these issues.

Luke D. Jones (3):
  asus-wmi: Add panel overdrive functionality
  asus-wmi: Add dgpu disable method
  asus-wmi: Add egpu enable method

 drivers/platform/x86/asus-wmi.c            | 282 +++++++++++++++++++++
 include/linux/platform_data/x86/asus-wmi.h |   7 +
 2 files changed, 289 insertions(+)

--
2.31.1

[PATCH 1/3] asus-wmi: Add panel overdrive functionality

[Luke D. Jones]

Some ASUS ROG laptops have the ability to drive the display panel
a a higher rate to eliminate or reduce ghosting.

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

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index ebaeb7bb80f5..2468076d6cd8 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -216,6 +216,9 @@ struct asus_wmi {
 	// The RSOC controls the maximum charging percentage.
 	bool battery_rsoc_available;
 
+	bool panel_overdrive_available;
+	u8 panel_overdrive;
+
 	struct hotplug_slot hotplug_slot;
 	struct mutex hotplug_lock;
 	struct mutex wmi_lock;
@@ -1221,6 +1224,87 @@ static int asus_wmi_rfkill_init(struct asus_wmi *asus)
 	return result;
 }
 
+/* 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)
+		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)
+{
+	int err;
+	u8 value;
+	u32 retval;
+
+	value = asus->panel_overdrive;
+
+	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_PANEL_OD, value, &retval);
+
+	sysfs_notify(&asus->platform_device->dev.kobj, NULL,
+			"panel_od");
+
+	if (err) {
+		pr_warn("Failed to set panel overdrive: %d\n", err);
+		return err;
+	}
+
+	if (retval > 1 || retval < 0) {
+		pr_warn("Failed to set panel overdrive (retval): 0x%x\n",
+			retval);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static ssize_t panel_od_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+	u8 mode = asus->panel_overdrive;
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", mode);
+}
+
+static ssize_t panel_od_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	int result;
+	u8 overdrive;
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	result = kstrtou8(buf, 10, &overdrive);
+	if (result < 0)
+		return result;
+
+	if (overdrive > 1 || overdrive < 0)
+		return -EINVAL;
+
+	asus->panel_overdrive = overdrive;
+	panel_od_write(asus);
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(panel_od);
+
 /* Quirks *********************************************************************/
 
 static void asus_wmi_set_xusb2pr(struct asus_wmi *asus)
@@ -2332,6 +2416,7 @@ static struct attribute *platform_attributes[] = {
 	&dev_attr_als_enable.attr,
 	&dev_attr_fan_boost_mode.attr,
 	&dev_attr_throttle_thermal_policy.attr,
+	&dev_attr_panel_od.attr,
 	NULL
 };
 
@@ -2357,6 +2442,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
 		ok = asus->fan_boost_mode_available;
 	else if (attr == &dev_attr_throttle_thermal_policy.attr)
 		ok = asus->throttle_thermal_policy_available;
+	else if (attr == &dev_attr_panel_od.attr)
+		ok = asus->panel_overdrive_available;
 
 	if (devid != -1)
 		ok = !(asus_wmi_get_devstate_simple(asus, devid) < 0);
@@ -2622,6 +2709,10 @@ static int asus_wmi_add(struct platform_device *pdev)
 	else
 		throttle_thermal_policy_set_default(asus);
 
+	err = panel_od_check_present(asus);
+	if (err)
+		goto fail_panel_od;
+
 	err = asus_wmi_sysfs_init(asus->platform_device);
 	if (err)
 		goto fail_sysfs;
@@ -2709,6 +2800,7 @@ static int asus_wmi_add(struct platform_device *pdev)
 fail_throttle_thermal_policy:
 fail_fan_boost_mode:
 fail_platform:
+fail_panel_od:
 	kfree(asus);
 	return err;
 }
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 2f274cf52805..428aea701c7b 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -61,6 +61,7 @@
 #define ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY 0x00120075
 
 /* Misc */
+#define ASUS_WMI_DEVID_PANEL_OD		0x00050019
 #define ASUS_WMI_DEVID_CAMERA		0x00060013
 #define ASUS_WMI_DEVID_LID_FLIP		0x00060062
 
-- 
2.31.1

Re: [PATCH 1/3] asus-wmi: Add panel overdrive functionality

[kernel test robot]

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

Hi "Luke,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.13 next-20210701]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Luke-D-Jones/Support-for-ASUS-egpu-dpgu-disable-panel-overdrive/20210705-062341
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 28e92f990337b8b4c5fdec47667f8b96089c503e
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/b46e8ec7f42ce4b644c43f40218fe853d078f098
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Luke-D-Jones/Support-for-ASUS-egpu-dpgu-disable-panel-overdrive/20210705-062341
        git checkout b46e8ec7f42ce4b644c43f40218fe853d078f098
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/platform/x86/asus-wmi.c: In function 'panel_od_check_present':
>> drivers/platform/x86/asus-wmi.c:1242:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
    1242 |  if (result & ASUS_WMI_DSTS_PRESENCE_BIT)
         |  ^~
   drivers/platform/x86/asus-wmi.c:1244:3: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
    1244 |   asus->panel_overdrive = result & ASUS_WMI_DSTS_STATUS_BIT;
         |   ^~~~

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for PHY_SPARX5_SERDES
   Depends on (ARCH_SPARX5 || COMPILE_TEST && OF && HAS_IOMEM
   Selected by
   - SPARX5_SWITCH && NETDEVICES && ETHERNET && NET_VENDOR_MICROCHIP && NET_SWITCHDEV && HAS_IOMEM


vim +/if +1242 drivers/platform/x86/asus-wmi.c

  1226	
  1227	/* Panel Overdrive ************************************************************/
  1228	static int panel_od_check_present(struct asus_wmi *asus)
  1229	{
  1230		u32 result;
  1231		int err;
  1232	
  1233		asus->panel_overdrive_available = false;
  1234	
  1235		err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_PANEL_OD, &result);
  1236		if (err) {
  1237			if (err == -ENODEV)
  1238				return 0;
  1239			return err;
  1240		}
  1241	
> 1242		if (result & ASUS_WMI_DSTS_PRESENCE_BIT)
  1243			asus->panel_overdrive_available = true;
  1244			asus->panel_overdrive = result & ASUS_WMI_DSTS_STATUS_BIT;
  1245	
  1246		return 0;
  1247	}
  1248	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 65978 bytes --]

Re: [PATCH 1/3] asus-wmi: Add panel overdrive functionality

[Hans de Goede]

Hi,

Thank you for your patches, I've added several remarks inline (below);

On 7/5/21 12:21 AM, Luke D. Jones wrote:
> Some ASUS ROG laptops have the ability to drive the display panel
> a a higher rate to eliminate or reduce ghosting.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>  drivers/platform/x86/asus-wmi.c            | 92 ++++++++++++++++++++++
>  include/linux/platform_data/x86/asus-wmi.h |  1 +
>  2 files changed, 93 insertions(+)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index ebaeb7bb80f5..2468076d6cd8 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -216,6 +216,9 @@ struct asus_wmi {
>  	// The RSOC controls the maximum charging percentage.
>  	bool battery_rsoc_available;
>  
> +	bool panel_overdrive_available;
> +	u8 panel_overdrive;
> +
>  	struct hotplug_slot hotplug_slot;
>  	struct mutex hotplug_lock;
>  	struct mutex wmi_lock;
> @@ -1221,6 +1224,87 @@ static int asus_wmi_rfkill_init(struct asus_wmi *asus)
>  	return result;
>  }
>  
> +/* 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)
> +		asus->panel_overdrive_available = true;
> +		asus->panel_overdrive = result & ASUS_WMI_DSTS_STATUS_BIT;

As the kernel-test-robot pointed out this if is missing { } around the
2 statements which should only be executed if the condition is true.

> +
> +	return 0;
> +}
> +
> +static int panel_od_write(struct asus_wmi *asus)
> +{
> +	int err;
> +	u8 value;
> +	u32 retval;
> +
> +	value = asus->panel_overdrive;
> +
> +	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_PANEL_OD, value, &retval);
> +
> +	sysfs_notify(&asus->platform_device->dev.kobj, NULL,
> +			"panel_od");

Please make this a single line.

> +
> +	if (err) {
> +		pr_warn("Failed to set panel overdrive: %d\n", err);
> +		return err;
> +	}
> +
> +	if (retval > 1 || retval < 0) {
> +		pr_warn("Failed to set panel overdrive (retval): 0x%x\n",
> +			retval);

Please make this a single line too.

> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static ssize_t panel_od_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +	u8 mode = asus->panel_overdrive;
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", mode);
> +}
> +
> +static ssize_t panel_od_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t count)
> +{
> +	int result;
> +	u8 overdrive;
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +	result = kstrtou8(buf, 10, &overdrive);
> +	if (result < 0)
> +		return result;
> +
> +	if (overdrive > 1 || overdrive < 0)

An u8 can never be < 0, so please drop that check
(it will likely trigger compiler warnings in some cases).

> +		return -EINVAL;
> +
> +	asus->panel_overdrive = overdrive;
> +	panel_od_write(asus);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(panel_od);
> +
>  /* Quirks *********************************************************************/
>  
>  static void asus_wmi_set_xusb2pr(struct asus_wmi *asus)
> @@ -2332,6 +2416,7 @@ static struct attribute *platform_attributes[] = {
>  	&dev_attr_als_enable.attr,
>  	&dev_attr_fan_boost_mode.attr,
>  	&dev_attr_throttle_thermal_policy.attr,
> +	&dev_attr_panel_od.attr,
>  	NULL
>  };
>  
> @@ -2357,6 +2442,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
>  		ok = asus->fan_boost_mode_available;
>  	else if (attr == &dev_attr_throttle_thermal_policy.attr)
>  		ok = asus->throttle_thermal_policy_available;
> +	else if (attr == &dev_attr_panel_od.attr)
> +		ok = asus->panel_overdrive_available;
>  
>  	if (devid != -1)
>  		ok = !(asus_wmi_get_devstate_simple(asus, devid) < 0);
> @@ -2622,6 +2709,10 @@ static int asus_wmi_add(struct platform_device *pdev)
>  	else
>  		throttle_thermal_policy_set_default(asus);
>  
> +	err = panel_od_check_present(asus);
> +	if (err)
> +		goto fail_panel_od;
> +
>  	err = asus_wmi_sysfs_init(asus->platform_device);
>  	if (err)
>  		goto fail_sysfs;
> @@ -2709,6 +2800,7 @@ static int asus_wmi_add(struct platform_device *pdev)
>  fail_throttle_thermal_policy:
>  fail_fan_boost_mode:
>  fail_platform:
> +fail_panel_od:
>  	kfree(asus);
>  	return err;
>  }
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index 2f274cf52805..428aea701c7b 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -61,6 +61,7 @@
>  #define ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY 0x00120075
>  
>  /* Misc */
> +#define ASUS_WMI_DEVID_PANEL_OD		0x00050019
>  #define ASUS_WMI_DEVID_CAMERA		0x00060013
>  #define ASUS_WMI_DEVID_LID_FLIP		0x00060062
>  
> 

Otherwise this looks good to me,

Regards,

Hans

Re: [PATCH 1/3] asus-wmi: Add panel overdrive functionality

[Hans de Goede]

Hi,

One more remark inline:

On 7/5/21 12:21 AM, Luke D. Jones wrote:
> Some ASUS ROG laptops have the ability to drive the display panel
> a a higher rate to eliminate or reduce ghosting.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>  drivers/platform/x86/asus-wmi.c            | 92 ++++++++++++++++++++++
>  include/linux/platform_data/x86/asus-wmi.h |  1 +
>  2 files changed, 93 insertions(+)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index ebaeb7bb80f5..2468076d6cd8 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c

<snip>

> +static ssize_t panel_od_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +	u8 mode = asus->panel_overdrive;
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", mode);

As Barnabás pointed out in his review of patch 2/3, please
use sysfs_emit here.

Regards,

Hans

[PATCH 2/3] asus-wmi: Add dgpu disable method

[Luke D. Jones]

In Windows the ASUS Armory Crate progrm can enable or disable the
dGPU via a WMI call. This functions much the same as various Linux
methods in software where the dGPU is removed from the device tree.

However the WMI call saves the state of dGPU enabled or not and this
then changes the dGPU visibility in Linux with no way for Linux
users to re-enable it. We expose the WMI method so users can see
and change the dGPU ACPI state.

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

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 2468076d6cd8..8dc3f7ed021f 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -210,6 +210,9 @@ struct asus_wmi {
 	u8 fan_boost_mode_mask;
 	u8 fan_boost_mode;
 
+	bool dgpu_disable_available;
+	u8 dgpu_disable_mode;
+
 	bool throttle_thermal_policy_available;
 	u8 throttle_thermal_policy_mode;
 
@@ -427,6 +430,93 @@ 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)
+		asus->dgpu_disable_available = true;
+		asus->dgpu_disable_mode = result & ASUS_WMI_DSTS_STATUS_BIT;
+
+	return 0;
+}
+
+static int dgpu_disable_write(struct asus_wmi *asus)
+{
+	int err;
+	u8 value;
+	u32 retval;
+
+	value = asus->dgpu_disable_mode;
+
+	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_DGPU, value, &retval);
+
+	sysfs_notify(&asus->platform_device->dev.kobj, NULL,
+			"dgpu_disable");
+
+	if (err) {
+		pr_warn("Failed to set dgpu disable: %d\n", err);
+		return err;
+	}
+
+	if (retval > 1 || retval < 0) {
+		pr_warn("Failed to set dgpu disable (retval): 0x%x\n",
+			retval);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+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_mode;
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", mode);
+}
+
+static ssize_t dgpu_disable_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	int result;
+	u8 disable;
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	result = kstrtou8(buf, 10, &disable);
+	if (result < 0)
+		return result;
+
+	if (disable > 1 || disable < 0)
+		return -EINVAL;
+
+	asus->dgpu_disable_mode = disable;
+	/*
+	 * The ACPI call used does not save the mode unless the call is run twice.
+	 * Once to disable, then once to check status and save - this is two code
+	 * paths in the method in the ACPI dumps.
+	*/
+	dgpu_disable_write(asus);
+	dgpu_disable_write(asus);
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(dgpu_disable);
+
 /* Battery ********************************************************************/
 
 /* The battery maximum charging percentage */
@@ -2412,6 +2502,7 @@ static struct attribute *platform_attributes[] = {
 	&dev_attr_camera.attr,
 	&dev_attr_cardr.attr,
 	&dev_attr_touchpad.attr,
+	&dev_attr_dgpu_disable.attr,
 	&dev_attr_lid_resume.attr,
 	&dev_attr_als_enable.attr,
 	&dev_attr_fan_boost_mode.attr,
@@ -2438,6 +2529,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
 		devid = ASUS_WMI_DEVID_LID_RESUME;
 	else if (attr == &dev_attr_als_enable.attr)
 		devid = ASUS_WMI_DEVID_ALS_ENABLE;
+	else if (attr == &dev_attr_dgpu_disable.attr)
+		ok = asus->dgpu_disable_available;
 	else if (attr == &dev_attr_fan_boost_mode.attr)
 		ok = asus->fan_boost_mode_available;
 	else if (attr == &dev_attr_throttle_thermal_policy.attr)
@@ -2699,6 +2792,10 @@ static int asus_wmi_add(struct platform_device *pdev)
 	if (err)
 		goto fail_platform;
 
+	err = dgpu_disable_check_present(asus);
+	if (err)
+		goto fail_dgpu_disable;
+
 	err = fan_boost_mode_check_present(asus);
 	if (err)
 		goto fail_fan_boost_mode;
@@ -2799,6 +2896,7 @@ static int asus_wmi_add(struct platform_device *pdev)
 fail_sysfs:
 fail_throttle_thermal_policy:
 fail_fan_boost_mode:
+fail_dgpu_disable:
 fail_platform:
 fail_panel_od:
 	kfree(asus);
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 428aea701c7b..a528f9d0e4b7 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -90,6 +90,9 @@
 /* Keyboard dock */
 #define ASUS_WMI_DEVID_KBD_DOCK		0x00120063
 
+/* dgpu on/off */
+#define ASUS_WMI_DEVID_DGPU		0x00090020
+
 /* DSTS masks */
 #define ASUS_WMI_DSTS_STATUS_BIT	0x00000001
 #define ASUS_WMI_DSTS_UNKNOWN_BIT	0x00000002
-- 
2.31.1

Re: [PATCH 2/3] asus-wmi: Add dgpu disable method

[Barnabás Pőcze]

Hi

I have added a couple comments inline.


2021. július 5., hétfő 0:21 keltezéssel, Luke D. Jones írta:

> In Windows the ASUS Armory Crate progrm can enable or disable the
                                        ^
"program"


> dGPU via a WMI call. This functions much the same as various Linux
> methods in software where the dGPU is removed from the device tree.
>
> However the WMI call saves the state of dGPU enabled or not and this

I think "[...] the WMI call saves whether the dGPU is enabled or not, and [...]"
might be better.
Or "[...] the WMI call saves the state of the dGPU (enabled or not) and [...]".


> then changes the dGPU visibility in Linux with no way for Linux
> users to re-enable it. We expose the WMI method so users can see
> and change the dGPU ACPI state.
>
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>  drivers/platform/x86/asus-wmi.c            | 98 ++++++++++++++++++++++
>  include/linux/platform_data/x86/asus-wmi.h |  3 +
>  2 files changed, 101 insertions(+)
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 2468076d6cd8..8dc3f7ed021f 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -210,6 +210,9 @@ struct asus_wmi {
>  	u8 fan_boost_mode_mask;
>  	u8 fan_boost_mode;
>
> +	bool dgpu_disable_available;
> +	u8 dgpu_disable_mode;
> +
>  	bool throttle_thermal_policy_available;
>  	u8 throttle_thermal_policy_mode;
>
> @@ -427,6 +430,93 @@ 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)
> +		asus->dgpu_disable_available = true;
> +		asus->dgpu_disable_mode = result & ASUS_WMI_DSTS_STATUS_BIT;
> +

Aren't braces missing here?


> +	return 0;
> +}
> +
> +static int dgpu_disable_write(struct asus_wmi *asus)
> +{
> +	int err;
> +	u8 value;
> +	u32 retval;
> +
> +	value = asus->dgpu_disable_mode;
> +
> +	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_DGPU, value, &retval);
> +
> +	sysfs_notify(&asus->platform_device->dev.kobj, NULL,
> +			"dgpu_disable");

A similar line with the exact same length in patch 3/3 is not broken in two.
And shouldn't the notification be sent if the operation succeeded?


> +
> +	if (err) {
> +		pr_warn("Failed to set dgpu disable: %d\n", err);
> +		return err;
> +	}
> +
> +	if (retval > 1 || retval < 0) {
> +		pr_warn("Failed to set dgpu disable (retval): 0x%x\n",
> +			retval);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +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_mode;
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", mode);

You could use `sysfs_emit()`.


> +}
> +
> +static ssize_t dgpu_disable_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t count)
> +{
> +	int result;
> +	u8 disable;
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +	result = kstrtou8(buf, 10, &disable);

You could use `kstrtobool()`. I think that would be better since it accepts
'y', 'n', etc. in addition to 0 and 1.


> +	if (result < 0)
> +		return result;
> +
> +	if (disable > 1 || disable < 0)
> +		return -EINVAL;
> +
> +	asus->dgpu_disable_mode = disable;
> +	/*
> +	 * The ACPI call used does not save the mode unless the call is run twice.
> +	 * Once to disable, then once to check status and save - this is two code
> +	 * paths in the method in the ACPI dumps.
> +	*/
> +	dgpu_disable_write(asus);
> +	dgpu_disable_write(asus);

Is there any reason the potential error codes are not returned?


> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(dgpu_disable);
> +
>  /* Battery ********************************************************************/
>
>  /* The battery maximum charging percentage */
> @@ -2412,6 +2502,7 @@ static struct attribute *platform_attributes[] = {
>  	&dev_attr_camera.attr,
>  	&dev_attr_cardr.attr,
>  	&dev_attr_touchpad.attr,
> +	&dev_attr_dgpu_disable.attr,
>  	&dev_attr_lid_resume.attr,
>  	&dev_attr_als_enable.attr,
>  	&dev_attr_fan_boost_mode.attr,
> @@ -2438,6 +2529,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
>  		devid = ASUS_WMI_DEVID_LID_RESUME;
>  	else if (attr == &dev_attr_als_enable.attr)
>  		devid = ASUS_WMI_DEVID_ALS_ENABLE;
> +	else if (attr == &dev_attr_dgpu_disable.attr)
> +		ok = asus->dgpu_disable_available;
>  	else if (attr == &dev_attr_fan_boost_mode.attr)
>  		ok = asus->fan_boost_mode_available;
>  	else if (attr == &dev_attr_throttle_thermal_policy.attr)
> @@ -2699,6 +2792,10 @@ static int asus_wmi_add(struct platform_device *pdev)
>  	if (err)
>  		goto fail_platform;
>
> +	err = dgpu_disable_check_present(asus);
> +	if (err)
> +		goto fail_dgpu_disable;
> +

Should this really be considered a "fatal" error?


>  	err = fan_boost_mode_check_present(asus);
>  	if (err)
>  		goto fail_fan_boost_mode;
> @@ -2799,6 +2896,7 @@ static int asus_wmi_add(struct platform_device *pdev)
>  fail_sysfs:
>  fail_throttle_thermal_policy:
>  fail_fan_boost_mode:
> +fail_dgpu_disable:
>  fail_platform:
>  fail_panel_od:
>  	kfree(asus);
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index 428aea701c7b..a528f9d0e4b7 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -90,6 +90,9 @@
>  /* Keyboard dock */
>  #define ASUS_WMI_DEVID_KBD_DOCK		0x00120063
>
> +/* dgpu on/off */
> +#define ASUS_WMI_DEVID_DGPU		0x00090020
> +
>  /* DSTS masks */
>  #define ASUS_WMI_DSTS_STATUS_BIT	0x00000001
>  #define ASUS_WMI_DSTS_UNKNOWN_BIT	0x00000002
> --
> 2.31.1


Regards,
Barnabás Pőcze

Re: [PATCH 2/3] asus-wmi: Add dgpu disable method

[Hans de Goede]

Hi,

Barnabás made some good points which I missed.

See me reply inline.

On 7/5/21 2:47 AM, Barnabás Pőcze wrote:
> Hi
> 
> I have added a couple comments inline.
> 
> 
> 2021. július 5., hétfő 0:21 keltezéssel, Luke D. Jones írta:
> 

<snip>

>> +static ssize_t dgpu_disable_store(struct device *dev,
>> +				    struct device_attribute *attr,
>> +				    const char *buf, size_t count)
>> +{
>> +	int result;
>> +	u8 disable;
>> +	struct asus_wmi *asus = dev_get_drvdata(dev);
>> +
>> +	result = kstrtou8(buf, 10, &disable);
> 
> You could use `kstrtobool()`. I think that would be better since it accepts
> 'y', 'n', etc. in addition to 0 and 1.

Good point and the same applies to patch 1/3.

>> +	if (result < 0)
>> +		return result;
>> +
>> +	if (disable > 1 || disable < 0)
>> +		return -EINVAL;
>> +
>> +	asus->dgpu_disable_mode = disable;
>> +	/*
>> +	 * The ACPI call used does not save the mode unless the call is run twice.
>> +	 * Once to disable, then once to check status and save - this is two code
>> +	 * paths in the method in the ACPI dumps.
>> +	*/
>> +	dgpu_disable_write(asus);
>> +	dgpu_disable_write(asus);
> 
> Is there any reason the potential error codes are not returned?

Good question.

<snip>

>> @@ -2699,6 +2792,10 @@ static int asus_wmi_add(struct platform_device *pdev)
>>  	if (err)
>>  		goto fail_platform;
>>
>> +	err = dgpu_disable_check_present(asus);
>> +	if (err)
>> +		goto fail_dgpu_disable;
>> +
> 
> Should this really be considered a "fatal" error?

Well dgpu_disable_check_present() does already contain:

		if (err == -ENODEV)
			return 0;

IOW it only returns an error on unexpected errors and asus_wmi_add()
already contains a couple of other foo_present() checks which are
dealt with in the same way, so this is consistent with that and
being consistent is good, so I think this is fine.

Regards,

Hans

Re: [PATCH 2/3] asus-wmi: Add dgpu disable method

[Barnabás Pőcze]

Hi


2021. július 6., kedd 12:17 keltezéssel, Hans de Goede írta:

> [...]
> >> @@ -2699,6 +2792,10 @@ static int asus_wmi_add(struct platform_device *pdev)
> >>  	if (err)
> >>  		goto fail_platform;
> >>
> >> +	err = dgpu_disable_check_present(asus);
> >> +	if (err)
> >> +		goto fail_dgpu_disable;
> >> +
> >
> > Should this really be considered a "fatal" error?
>
> Well dgpu_disable_check_present() does already contain:
>
> 		if (err == -ENODEV)
> 			return 0;
>
> IOW it only returns an error on unexpected errors and asus_wmi_add()
> already contains a couple of other foo_present() checks which are
> dealt with in the same way, so this is consistent with that and
> being consistent is good, so I think this is fine.
>

Indeed, that's right, I missed that. I am still unsure whether any error
should cause it to fail to load. But I guess if there is precedent for that
in the module, then consistency is probably better.


Regards,
Barnabás Pőcze

Re: [PATCH 2/3] asus-wmi: Add dgpu disable method

[Luke Jones]

Thank you for the insightful feedback.

On Mon, Jul 5 2021 at 00:47:31 +0000, Barnabás Pőcze 
<pobrn@protonmail.com> wrote:
> Hi
> 
> I have added a couple comments inline.
> 
> 
> 2021. július 5., hétfő 0:21 keltezéssel, Luke D. Jones írta:
> 
>>  In Windows the ASUS Armory Crate progrm can enable or disable the
>                                         ^
> "program"
> 
My "a" key is a little hard to press sometimes :(

> 
>>  dGPU via a WMI call. This functions much the same as various Linux
>>  methods in software where the dGPU is removed from the device tree.
>> 
>>  However the WMI call saves the state of dGPU enabled or not and this
> 
> I think "[...] the WMI call saves whether the dGPU is enabled or not, 
> and [...]"
> might be better.
> Or "[...] the WMI call saves the state of the dGPU (enabled or not) 
> and [...]".
> 
I've used the second option, thanks for pointing this out.

> 
>>  then changes the dGPU visibility in Linux with no way for Linux
>>  users to re-enable it. We expose the WMI method so users can see
>>  and change the dGPU ACPI state.
>> 
>>  Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>  ---
>>   drivers/platform/x86/asus-wmi.c            | 98 
>> ++++++++++++++++++++++
>>   include/linux/platform_data/x86/asus-wmi.h |  3 +
>>   2 files changed, 101 insertions(+)
>> 
>>  diff --git a/drivers/platform/x86/asus-wmi.c 
>> b/drivers/platform/x86/asus-wmi.c
>>  index 2468076d6cd8..8dc3f7ed021f 100644
>>  --- a/drivers/platform/x86/asus-wmi.c
>>  +++ b/drivers/platform/x86/asus-wmi.c
>>  @@ -210,6 +210,9 @@ struct asus_wmi {
>>   	u8 fan_boost_mode_mask;
>>   	u8 fan_boost_mode;
>> 
>>  +	bool dgpu_disable_available;
>>  +	u8 dgpu_disable_mode;
>>  +
>>   	bool throttle_thermal_policy_available;
>>   	u8 throttle_thermal_policy_mode;
>> 
>>  @@ -427,6 +430,93 @@ 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)
>>  +		asus->dgpu_disable_available = true;
>>  +		asus->dgpu_disable_mode = result & ASUS_WMI_DSTS_STATUS_BIT;
>>  +
> 
> Aren't braces missing here?
> 
Yep. Fixed.

> 
>>  +	return 0;
>>  +}
>>  +
>>  +static int dgpu_disable_write(struct asus_wmi *asus)
>>  +{
>>  +	int err;
>>  +	u8 value;
>>  +	u32 retval;
>>  +
>>  +	value = asus->dgpu_disable_mode;
>>  +
>>  +	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_DGPU, value, &retval);
>>  +
>>  +	sysfs_notify(&asus->platform_device->dev.kobj, NULL,
>>  +			"dgpu_disable");
> 
> A similar line with the exact same length in patch 3/3 is not broken 
> in two.
> And shouldn't the notification be sent if the operation succeeded?
> 
Fixed. I moved the sysfs_notify down below the error returns.
> 
>>  +
>>  +	if (err) {
>>  +		pr_warn("Failed to set dgpu disable: %d\n", err);
>>  +		return err;
>>  +	}
>>  +
>>  +	if (retval > 1 || retval < 0) {
>>  +		pr_warn("Failed to set dgpu disable (retval): 0x%x\n",
>>  +			retval);
>>  +		return -EIO;
>>  +	}
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +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_mode;
>>  +
>>  +	return scnprintf(buf, PAGE_SIZE, "%d\n", mode);
> 
> You could use `sysfs_emit()`.
> 
Thanks. Many things like this I'm actually unaware of. Most of these 
patches are done by reading existing code.

> 
>>  +}
>>  +
>>  +static ssize_t dgpu_disable_store(struct device *dev,
>>  +				    struct device_attribute *attr,
>>  +				    const char *buf, size_t count)
>>  +{
>>  +	int result;
>>  +	u8 disable;
>>  +	struct asus_wmi *asus = dev_get_drvdata(dev);
>>  +
>>  +	result = kstrtou8(buf, 10, &disable);
> 
> You could use `kstrtobool()`. I think that would be better since it 
> accepts
> 'y', 'n', etc. in addition to 0 and 1.
> 
Thanks! Wasn't aware of that. And since the setting for all 3 patches 
can only ever be 0/1 I've changed to use a bool instead of u8

> 
>>  +	if (result < 0)
>>  +		return result;
>>  +
>>  +	if (disable > 1 || disable < 0)
>>  +		return -EINVAL;
>>  +
>>  +	asus->dgpu_disable_mode = disable;
>>  +	/*
>>  +	 * The ACPI call used does not save the mode unless the call is 
>> run twice.
>>  +	 * Once to disable, then once to check status and save - this is 
>> two code
>>  +	 * paths in the method in the ACPI dumps.
>>  +	*/
>>  +	dgpu_disable_write(asus);
>>  +	dgpu_disable_write(asus);
> 
> Is there any reason the potential error codes are not returned?
> 
No I've fixed now. Guess I missed it.

> 
>>  +
>>  +	return count;
>>  +}
>>  +
>>  +static DEVICE_ATTR_RW(dgpu_disable);
>>  +
>>   /* Battery 
>> ********************************************************************/
>> 
>>   /* The battery maximum charging percentage */
>>  @@ -2412,6 +2502,7 @@ static struct attribute 
>> *platform_attributes[] = {
>>   	&dev_attr_camera.attr,
>>   	&dev_attr_cardr.attr,
>>   	&dev_attr_touchpad.attr,
>>  +	&dev_attr_dgpu_disable.attr,
>>   	&dev_attr_lid_resume.attr,
>>   	&dev_attr_als_enable.attr,
>>   	&dev_attr_fan_boost_mode.attr,
>>  @@ -2438,6 +2529,8 @@ static umode_t asus_sysfs_is_visible(struct 
>> kobject *kobj,
>>   		devid = ASUS_WMI_DEVID_LID_RESUME;
>>   	else if (attr == &dev_attr_als_enable.attr)
>>   		devid = ASUS_WMI_DEVID_ALS_ENABLE;
>>  +	else if (attr == &dev_attr_dgpu_disable.attr)
>>  +		ok = asus->dgpu_disable_available;
>>   	else if (attr == &dev_attr_fan_boost_mode.attr)
>>   		ok = asus->fan_boost_mode_available;
>>   	else if (attr == &dev_attr_throttle_thermal_policy.attr)
>>  @@ -2699,6 +2792,10 @@ static int asus_wmi_add(struct 
>> platform_device *pdev)
>>   	if (err)
>>   		goto fail_platform;
>> 
>>  +	err = dgpu_disable_check_present(asus);
>>  +	if (err)
>>  +		goto fail_dgpu_disable;
>>  +
> 
> Should this really be considered a "fatal" error?
> 
I was modelling this on fail_fan_boost_mode and 
fail_throttle_thermal_policy since a laptop can't have both of those 
which indicates that a failed one is fine, it seemed appropriate to 
follow the same behaviour here

> 
>>   	err = fan_boost_mode_check_present(asus);
>>   	if (err)
>>   		goto fail_fan_boost_mode;
>>  @@ -2799,6 +2896,7 @@ static int asus_wmi_add(struct 
>> platform_device *pdev)
>>   fail_sysfs:
>>   fail_throttle_thermal_policy:
>>   fail_fan_boost_mode:
>>  +fail_dgpu_disable:
>>   fail_platform:
>>   fail_panel_od:
>>   	kfree(asus);
>>  diff --git a/include/linux/platform_data/x86/asus-wmi.h 
>> b/include/linux/platform_data/x86/asus-wmi.h
>>  index 428aea701c7b..a528f9d0e4b7 100644
>>  --- a/include/linux/platform_data/x86/asus-wmi.h
>>  +++ b/include/linux/platform_data/x86/asus-wmi.h
>>  @@ -90,6 +90,9 @@
>>   /* Keyboard dock */
>>   #define ASUS_WMI_DEVID_KBD_DOCK		0x00120063
>> 
>>  +/* dgpu on/off */
>>  +#define ASUS_WMI_DEVID_DGPU		0x00090020
>>  +
>>   /* DSTS masks */
>>   #define ASUS_WMI_DSTS_STATUS_BIT	0x00000001
>>   #define ASUS_WMI_DSTS_UNKNOWN_BIT	0x00000002
>>  --
>>  2.31.1
> 
> 
> Regards,
> Barnabás Pőcze

Many thanks for your feedback again.
Luke.

Re: [PATCH 2/3] asus-wmi: Add dgpu disable method

[Hans de Goede]

Hi,

Some review remarks inline (mostly just echo-ing what Barnabás already said):

On 7/5/21 12:21 AM, Luke D. Jones wrote:
> In Windows the ASUS Armory Crate progrm can enable or disable the
> dGPU via a WMI call. This functions much the same as various Linux
> methods in software where the dGPU is removed from the device tree.
> 
> However the WMI call saves the state of dGPU enabled or not and this
> then changes the dGPU visibility in Linux with no way for Linux
> users to re-enable it. We expose the WMI method so users can see
> and change the dGPU ACPI state.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>  drivers/platform/x86/asus-wmi.c            | 98 ++++++++++++++++++++++
>  include/linux/platform_data/x86/asus-wmi.h |  3 +
>  2 files changed, 101 insertions(+)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 2468076d6cd8..8dc3f7ed021f 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -210,6 +210,9 @@ struct asus_wmi {
>  	u8 fan_boost_mode_mask;
>  	u8 fan_boost_mode;
>  
> +	bool dgpu_disable_available;
> +	u8 dgpu_disable_mode;
> +
>  	bool throttle_thermal_policy_available;
>  	u8 throttle_thermal_policy_mode;
>  
> @@ -427,6 +430,93 @@ 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)
> +		asus->dgpu_disable_available = true;
> +		asus->dgpu_disable_mode = result & ASUS_WMI_DSTS_STATUS_BIT;

Missing {}

> +
> +	return 0;
> +}
> +
> +static int dgpu_disable_write(struct asus_wmi *asus)
> +{
> +	int err;
> +	u8 value;
> +	u32 retval;
> +
> +	value = asus->dgpu_disable_mode;
> +
> +	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_DGPU, value, &retval);
> +
> +	sysfs_notify(&asus->platform_device->dev.kobj, NULL,
> +			"dgpu_disable");

Make this one line please.

> +
> +	if (err) {
> +		pr_warn("Failed to set dgpu disable: %d\n", err);
> +		return err;
> +	}
> +
> +	if (retval > 1 || retval < 0) {
> +		pr_warn("Failed to set dgpu disable (retval): 0x%x\n",
> +			retval);

Make this one line please.

> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +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_mode;
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", mode);

sysfs_emit() please.

> +}
> +
> +static ssize_t dgpu_disable_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t count)
> +{
> +	int result;
> +	u8 disable;
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +	result = kstrtou8(buf, 10, &disable);
> +	if (result < 0)
> +		return result;
> +
> +	if (disable > 1 || disable < 0)
> +		return -EINVAL;

Drop the "disable < 0" check please.

> +
> +	asus->dgpu_disable_mode = disable;
> +	/*
> +	 * The ACPI call used does not save the mode unless the call is run twice.
> +	 * Once to disable, then once to check status and save - this is two code
> +	 * paths in the method in the ACPI dumps.
> +	*/
> +	dgpu_disable_write(asus);
> +	dgpu_disable_write(asus);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(dgpu_disable);
> +
>  /* Battery ********************************************************************/
>  
>  /* The battery maximum charging percentage */
> @@ -2412,6 +2502,7 @@ static struct attribute *platform_attributes[] = {
>  	&dev_attr_camera.attr,
>  	&dev_attr_cardr.attr,
>  	&dev_attr_touchpad.attr,
> +	&dev_attr_dgpu_disable.attr,
>  	&dev_attr_lid_resume.attr,
>  	&dev_attr_als_enable.attr,
>  	&dev_attr_fan_boost_mode.attr,
> @@ -2438,6 +2529,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
>  		devid = ASUS_WMI_DEVID_LID_RESUME;
>  	else if (attr == &dev_attr_als_enable.attr)
>  		devid = ASUS_WMI_DEVID_ALS_ENABLE;
> +	else if (attr == &dev_attr_dgpu_disable.attr)
> +		ok = asus->dgpu_disable_available;
>  	else if (attr == &dev_attr_fan_boost_mode.attr)
>  		ok = asus->fan_boost_mode_available;
>  	else if (attr == &dev_attr_throttle_thermal_policy.attr)
> @@ -2699,6 +2792,10 @@ static int asus_wmi_add(struct platform_device *pdev)
>  	if (err)
>  		goto fail_platform;
>  
> +	err = dgpu_disable_check_present(asus);
> +	if (err)
> +		goto fail_dgpu_disable;
> +
>  	err = fan_boost_mode_check_present(asus);
>  	if (err)
>  		goto fail_fan_boost_mode;
> @@ -2799,6 +2896,7 @@ static int asus_wmi_add(struct platform_device *pdev)
>  fail_sysfs:
>  fail_throttle_thermal_policy:
>  fail_fan_boost_mode:
> +fail_dgpu_disable:
>  fail_platform:
>  fail_panel_od:
>  	kfree(asus);
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index 428aea701c7b..a528f9d0e4b7 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -90,6 +90,9 @@
>  /* Keyboard dock */
>  #define ASUS_WMI_DEVID_KBD_DOCK		0x00120063
>  
> +/* dgpu on/off */
> +#define ASUS_WMI_DEVID_DGPU		0x00090020
> +
>  /* DSTS masks */
>  #define ASUS_WMI_DSTS_STATUS_BIT	0x00000001
>  #define ASUS_WMI_DSTS_UNKNOWN_BIT	0x00000002
> 

Otherwise this looks good to me.

Regards,

Hans

Re: [PATCH 2/3] asus-wmi: Add dgpu disable method

[Luke Jones]

Thanks Hans, all feedback applied.

On Tue, Jul 6 2021 at 12:10:45 +0200, Hans de Goede 
<hdegoede@redhat.com> wrote:
> Hi,
> 
> Some review remarks inline (mostly just echo-ing what Barnabás 
> already said):
> 
> On 7/5/21 12:21 AM, Luke D. Jones wrote:
>>  In Windows the ASUS Armory Crate progrm can enable or disable the
>>  dGPU via a WMI call. This functions much the same as various Linux
>>  methods in software where the dGPU is removed from the device tree.
>> 
>>  However the WMI call saves the state of dGPU enabled or not and this
>>  then changes the dGPU visibility in Linux with no way for Linux
>>  users to re-enable it. We expose the WMI method so users can see
>>  and change the dGPU ACPI state.
>> 
>>  Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>  ---
>>   drivers/platform/x86/asus-wmi.c            | 98 
>> ++++++++++++++++++++++
>>   include/linux/platform_data/x86/asus-wmi.h |  3 +
>>   2 files changed, 101 insertions(+)
>> 
>>  diff --git a/drivers/platform/x86/asus-wmi.c 
>> b/drivers/platform/x86/asus-wmi.c
>>  index 2468076d6cd8..8dc3f7ed021f 100644
>>  --- a/drivers/platform/x86/asus-wmi.c
>>  +++ b/drivers/platform/x86/asus-wmi.c
>>  @@ -210,6 +210,9 @@ struct asus_wmi {
>>   	u8 fan_boost_mode_mask;
>>   	u8 fan_boost_mode;
>> 
>>  +	bool dgpu_disable_available;
>>  +	u8 dgpu_disable_mode;
>>  +
>>   	bool throttle_thermal_policy_available;
>>   	u8 throttle_thermal_policy_mode;
>> 
>>  @@ -427,6 +430,93 @@ 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)
>>  +		asus->dgpu_disable_available = true;
>>  +		asus->dgpu_disable_mode = result & ASUS_WMI_DSTS_STATUS_BIT;
> 
> Missing {}
> 
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static int dgpu_disable_write(struct asus_wmi *asus)
>>  +{
>>  +	int err;
>>  +	u8 value;
>>  +	u32 retval;
>>  +
>>  +	value = asus->dgpu_disable_mode;
>>  +
>>  +	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_DGPU, value, &retval);
>>  +
>>  +	sysfs_notify(&asus->platform_device->dev.kobj, NULL,
>>  +			"dgpu_disable");
> 
> Make this one line please.
> 
>>  +
>>  +	if (err) {
>>  +		pr_warn("Failed to set dgpu disable: %d\n", err);
>>  +		return err;
>>  +	}
>>  +
>>  +	if (retval > 1 || retval < 0) {
>>  +		pr_warn("Failed to set dgpu disable (retval): 0x%x\n",
>>  +			retval);
> 
> Make this one line please.
> 
>>  +		return -EIO;
>>  +	}
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +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_mode;
>>  +
>>  +	return scnprintf(buf, PAGE_SIZE, "%d\n", mode);
> 
> sysfs_emit() please.
> 
>>  +}
>>  +
>>  +static ssize_t dgpu_disable_store(struct device *dev,
>>  +				    struct device_attribute *attr,
>>  +				    const char *buf, size_t count)
>>  +{
>>  +	int result;
>>  +	u8 disable;
>>  +	struct asus_wmi *asus = dev_get_drvdata(dev);
>>  +
>>  +	result = kstrtou8(buf, 10, &disable);
>>  +	if (result < 0)
>>  +		return result;
>>  +
>>  +	if (disable > 1 || disable < 0)
>>  +		return -EINVAL;
> 
> Drop the "disable < 0" check please.
> 
>>  +
>>  +	asus->dgpu_disable_mode = disable;
>>  +	/*
>>  +	 * The ACPI call used does not save the mode unless the call is 
>> run twice.
>>  +	 * Once to disable, then once to check status and save - this is 
>> two code
>>  +	 * paths in the method in the ACPI dumps.
>>  +	*/
>>  +	dgpu_disable_write(asus);
>>  +	dgpu_disable_write(asus);
>>  +
>>  +	return count;
>>  +}
>>  +
>>  +static DEVICE_ATTR_RW(dgpu_disable);
>>  +
>>   /* Battery 
>> ********************************************************************/
>> 
>>   /* The battery maximum charging percentage */
>>  @@ -2412,6 +2502,7 @@ static struct attribute 
>> *platform_attributes[] = {
>>   	&dev_attr_camera.attr,
>>   	&dev_attr_cardr.attr,
>>   	&dev_attr_touchpad.attr,
>>  +	&dev_attr_dgpu_disable.attr,
>>   	&dev_attr_lid_resume.attr,
>>   	&dev_attr_als_enable.attr,
>>   	&dev_attr_fan_boost_mode.attr,
>>  @@ -2438,6 +2529,8 @@ static umode_t asus_sysfs_is_visible(struct 
>> kobject *kobj,
>>   		devid = ASUS_WMI_DEVID_LID_RESUME;
>>   	else if (attr == &dev_attr_als_enable.attr)
>>   		devid = ASUS_WMI_DEVID_ALS_ENABLE;
>>  +	else if (attr == &dev_attr_dgpu_disable.attr)
>>  +		ok = asus->dgpu_disable_available;
>>   	else if (attr == &dev_attr_fan_boost_mode.attr)
>>   		ok = asus->fan_boost_mode_available;
>>   	else if (attr == &dev_attr_throttle_thermal_policy.attr)
>>  @@ -2699,6 +2792,10 @@ static int asus_wmi_add(struct 
>> platform_device *pdev)
>>   	if (err)
>>   		goto fail_platform;
>> 
>>  +	err = dgpu_disable_check_present(asus);
>>  +	if (err)
>>  +		goto fail_dgpu_disable;
>>  +
>>   	err = fan_boost_mode_check_present(asus);
>>   	if (err)
>>   		goto fail_fan_boost_mode;
>>  @@ -2799,6 +2896,7 @@ static int asus_wmi_add(struct 
>> platform_device *pdev)
>>   fail_sysfs:
>>   fail_throttle_thermal_policy:
>>   fail_fan_boost_mode:
>>  +fail_dgpu_disable:
>>   fail_platform:
>>   fail_panel_od:
>>   	kfree(asus);
>>  diff --git a/include/linux/platform_data/x86/asus-wmi.h 
>> b/include/linux/platform_data/x86/asus-wmi.h
>>  index 428aea701c7b..a528f9d0e4b7 100644
>>  --- a/include/linux/platform_data/x86/asus-wmi.h
>>  +++ b/include/linux/platform_data/x86/asus-wmi.h
>>  @@ -90,6 +90,9 @@
>>   /* Keyboard dock */
>>   #define ASUS_WMI_DEVID_KBD_DOCK		0x00120063
>> 
>>  +/* dgpu on/off */
>>  +#define ASUS_WMI_DEVID_DGPU		0x00090020
>>  +
>>   /* DSTS masks */
>>   #define ASUS_WMI_DSTS_STATUS_BIT	0x00000001
>>   #define ASUS_WMI_DSTS_UNKNOWN_BIT	0x00000002
>> 
> 
> Otherwise this looks good to me.
> 
> Regards,
> 
> Hans
> 

[PATCH 3/3] asus-wmi: Add egpu enable method

[Luke D. Jones]

The X13 Flow laptops can utilise an external GPU. This requires
toggling an ACPI method which will first disable the internal
dGPU, and then enable the eGPU.

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

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 8dc3f7ed021f..c9fe77456b7b 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -210,6 +210,9 @@ struct asus_wmi {
 	u8 fan_boost_mode_mask;
 	u8 fan_boost_mode;
 
+	bool egpu_enable_available; // 0 = enable
+	u8 egpu_enable_mode;
+
 	bool dgpu_disable_available;
 	u8 dgpu_disable_mode;
 
@@ -430,6 +433,87 @@ static void lid_flip_tablet_mode_get_state(struct asus_wmi *asus)
 	}
 }
 
+/* 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) {
+		asus->egpu_enable_available = true;
+		asus->egpu_enable_mode = result & ASUS_WMI_DSTS_STATUS_BIT;
+	}
+
+	return 0;
+}
+
+static int egpu_enable_write(struct asus_wmi *asus)
+{
+	int err;
+	u8 value;
+	u32 retval;
+
+	value = asus->egpu_enable_mode;
+
+	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_EGPU, value, &retval);
+
+	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "egpu_enable");
+
+	if (err) {
+		pr_warn("Failed to set egpu disable: %d\n", err);
+		return err;
+	}
+
+	if (retval > 1 || retval < 0) {
+		pr_warn("Failed to set egpu disable (retval): 0x%x\n", retval);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static ssize_t egpu_enable_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+	u8 mode = asus->egpu_enable_mode;
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", mode);
+}
+
+static ssize_t egpu_enable_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	int result;
+	u8 disable;
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	result = kstrtou8(buf, 10, &disable);
+	if (result < 0)
+		return result;
+
+	if (disable > 1 || disable < 0)
+		return -EINVAL;
+
+	asus->egpu_enable_mode = disable;
+
+	egpu_enable_write(asus);
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(egpu_enable);
+
 /* dGPU ********************************************************************/
 static int dgpu_disable_check_present(struct asus_wmi *asus)
 {
@@ -2502,6 +2586,7 @@ static struct attribute *platform_attributes[] = {
 	&dev_attr_camera.attr,
 	&dev_attr_cardr.attr,
 	&dev_attr_touchpad.attr,
+	&dev_attr_egpu_enable.attr,
 	&dev_attr_dgpu_disable.attr,
 	&dev_attr_lid_resume.attr,
 	&dev_attr_als_enable.attr,
@@ -2529,6 +2614,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
 		devid = ASUS_WMI_DEVID_LID_RESUME;
 	else if (attr == &dev_attr_als_enable.attr)
 		devid = ASUS_WMI_DEVID_ALS_ENABLE;
+	else if (attr == &dev_attr_egpu_enable.attr)
+		ok = asus->egpu_enable_available;
 	else if (attr == &dev_attr_dgpu_disable.attr)
 		ok = asus->dgpu_disable_available;
 	else if (attr == &dev_attr_fan_boost_mode.attr)
@@ -2792,6 +2879,10 @@ static int asus_wmi_add(struct platform_device *pdev)
 	if (err)
 		goto fail_platform;
 
+	err = egpu_enable_check_present(asus);
+	if (err)
+		goto fail_egpu_enable;
+
 	err = dgpu_disable_check_present(asus);
 	if (err)
 		goto fail_dgpu_disable;
@@ -2896,6 +2987,7 @@ static int asus_wmi_add(struct platform_device *pdev)
 fail_sysfs:
 fail_throttle_thermal_policy:
 fail_fan_boost_mode:
+fail_egpu_enable:
 fail_dgpu_disable:
 fail_platform:
 fail_panel_od:
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index a528f9d0e4b7..17dc5cb6f3f2 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -90,6 +90,9 @@
 /* Keyboard dock */
 #define ASUS_WMI_DEVID_KBD_DOCK		0x00120063
 
+/* dgpu on/off */
+#define ASUS_WMI_DEVID_EGPU		0x00090019
+
 /* dgpu on/off */
 #define ASUS_WMI_DEVID_DGPU		0x00090020
 
-- 
2.31.1

Re: [PATCH 3/3] asus-wmi: Add egpu enable method

[Hans de Goede]

Hi,

On 7/5/21 12:21 AM, Luke D. Jones wrote:
> The X13 Flow laptops can utilise an external GPU. This requires
> toggling an ACPI method which will first disable the internal
> dGPU, and then enable the eGPU.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>  drivers/platform/x86/asus-wmi.c            | 92 ++++++++++++++++++++++
>  include/linux/platform_data/x86/asus-wmi.h |  3 +
>  2 files changed, 95 insertions(+)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 8dc3f7ed021f..c9fe77456b7b 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -210,6 +210,9 @@ struct asus_wmi {
>  	u8 fan_boost_mode_mask;
>  	u8 fan_boost_mode;
>  
> +	bool egpu_enable_available; // 0 = enable
> +	u8 egpu_enable_mode;
> +
>  	bool dgpu_disable_available;
>  	u8 dgpu_disable_mode;
>  
> @@ -430,6 +433,87 @@ static void lid_flip_tablet_mode_get_state(struct asus_wmi *asus)
>  	}
>  }
>  
> +/* 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) {
> +		asus->egpu_enable_available = true;
> +		asus->egpu_enable_mode = result & ASUS_WMI_DSTS_STATUS_BIT;
> +	}

And now the braces are there (good) :)
> +
> +	return 0;
> +}
> +
> +static int egpu_enable_write(struct asus_wmi *asus)
> +{
> +	int err;
> +	u8 value;
> +	u32 retval;
> +
> +	value = asus->egpu_enable_mode;
> +
> +	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_EGPU, value, &retval);
> +
> +	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "egpu_enable");
> +
> +	if (err) {
> +		pr_warn("Failed to set egpu disable: %d\n", err);
> +		return err;
> +	}
> +
> +	if (retval > 1 || retval < 0) {
> +		pr_warn("Failed to set egpu disable (retval): 0x%x\n", retval);

And all these now are on one line, good again :)

> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static ssize_t egpu_enable_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +	u8 mode = asus->egpu_enable_mode;
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", mode);

sysfs_emit() please.

> +}
> +
> +static ssize_t egpu_enable_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t count)
> +{
> +	int result;
> +	u8 disable;
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +	result = kstrtou8(buf, 10, &disable);
> +	if (result < 0)
> +		return result;
> +
> +	if (disable > 1 || disable < 0)
> +		return -EINVAL;

Replace this with kstrtobool() ? or otherwise drop the
disable < 0 check.

> +
> +	asus->egpu_enable_mode = disable;
> +
> +	egpu_enable_write(asus);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(egpu_enable);
> +
>  /* dGPU ********************************************************************/
>  static int dgpu_disable_check_present(struct asus_wmi *asus)
>  {
> @@ -2502,6 +2586,7 @@ static struct attribute *platform_attributes[] = {
>  	&dev_attr_camera.attr,
>  	&dev_attr_cardr.attr,
>  	&dev_attr_touchpad.attr,
> +	&dev_attr_egpu_enable.attr,
>  	&dev_attr_dgpu_disable.attr,
>  	&dev_attr_lid_resume.attr,
>  	&dev_attr_als_enable.attr,
> @@ -2529,6 +2614,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
>  		devid = ASUS_WMI_DEVID_LID_RESUME;
>  	else if (attr == &dev_attr_als_enable.attr)
>  		devid = ASUS_WMI_DEVID_ALS_ENABLE;
> +	else if (attr == &dev_attr_egpu_enable.attr)
> +		ok = asus->egpu_enable_available;
>  	else if (attr == &dev_attr_dgpu_disable.attr)
>  		ok = asus->dgpu_disable_available;
>  	else if (attr == &dev_attr_fan_boost_mode.attr)
> @@ -2792,6 +2879,10 @@ static int asus_wmi_add(struct platform_device *pdev)
>  	if (err)
>  		goto fail_platform;
>  
> +	err = egpu_enable_check_present(asus);
> +	if (err)
> +		goto fail_egpu_enable;
> +
>  	err = dgpu_disable_check_present(asus);
>  	if (err)
>  		goto fail_dgpu_disable;
> @@ -2896,6 +2987,7 @@ static int asus_wmi_add(struct platform_device *pdev)
>  fail_sysfs:
>  fail_throttle_thermal_policy:
>  fail_fan_boost_mode:
> +fail_egpu_enable:
>  fail_dgpu_disable:
>  fail_platform:
>  fail_panel_od:
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index a528f9d0e4b7..17dc5cb6f3f2 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -90,6 +90,9 @@
>  /* Keyboard dock */
>  #define ASUS_WMI_DEVID_KBD_DOCK		0x00120063
>  
> +/* dgpu on/off */
> +#define ASUS_WMI_DEVID_EGPU		0x00090019
> +
>  /* dgpu on/off */
>  #define ASUS_WMI_DEVID_DGPU		0x00090020
>  
> 

Otherwise this looks good to me.

Regards,

Hans