[PATCH 0/9] platform-x86-asus-wmi: multiple fixes, improvements, features

[PATCH 0/9] platform-x86-asus-wmi: multiple fixes, improvements, features

[Luke D. Jones]

This is a series of patches containing either fixes, improvements, or new features.

Most of it is vary straightforward and I'd hope quick and easy to review.
However patch 0003 does a fair bit of work in reducing a series of common
patterns down to a few macros and a more generic store function. This ends
up unifying a lot of things to and making the end result much cleaner to read
plus add future features of the same pattern.

I am unsure if I've got the macros correct. Everything compiles, all is tested
and working correctly - but maybe I could use some guidance on formatting and
style there? Should they be in a header?

The debug statement patch was invaluable for debugging some issues on another
users machine. I will likely add some more debug in future.

The platform_profile issue.. It seems there is a race bewteen asus-wmi and an
AMD driver to register the platform_profile on boot on some laptops. The
asus-wmi driver can live without platform_profile quite fine as the feature
is really just a wrapper around throttle_thermal_policy. But it does raise
the question of "what's going on here?".

Many thanks,
Luke.

Luke D. Jones (9):
  platform/x86: asus-wmi: add debug print in more key places
  platform/x86: asus-wmi: don't fail if platform_profile already
    registered
  platform/x86: asus-wmi: add macros and expose min/max sysfs for ppt
    tunables
  platform/x86: asus-wmi: reduce code duplication with macros
  platform/x86: asus-wmi: use WMI_SIMPLE_SHOW in more places
  platform/x86: asus-wmi: add panel-fhd functionality
  platform/x86: asus-wmi: add enable/disable CPU cores
  platform/x86: asus-wmi: add apu_mem setting
  platform/x86: asus-wmi: add setting dGPU TGP

 .../ABI/testing/sysfs-platform-asus-wmi       |   78 +-
 drivers/platform/x86/asus-wmi.c               | 1248 ++++++++---------
 include/linux/platform_data/x86/asus-wmi.h    |   12 +
 3 files changed, 688 insertions(+), 650 deletions(-)

-- 
2.45.1

[PATCH 1/9] platform/x86: asus-wmi: add debug print in more key places

[Luke D. Jones]

Add more verbose debug print in the WMI method calls. This helps a lot
with debugging various issues working with regular users as the WMI
methods can be traced now.

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

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index a58df18a70ad..adc841abbb2a 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -334,20 +334,29 @@ static int asus_wmi_evaluate_method3(u32 method_id,
 	status = wmi_evaluate_method(ASUS_WMI_MGMT_GUID, 0, method_id,
 				     &input, &output);
 
-	if (ACPI_FAILURE(status))
+	pr_debug("%s called (0x%08x) with args: 0x%08x, 0x%08x, 0x%08x\n",
+		__func__, method_id, arg0, arg1, arg2);
+	if (ACPI_FAILURE(status)) {
+		pr_debug("%s, (0x%08x), arg 0x%08x failed: %d\n",
+			__func__, method_id, arg0, -EIO);
 		return -EIO;
+	}
 
 	obj = (union acpi_object *)output.pointer;
 	if (obj && obj->type == ACPI_TYPE_INTEGER)
 		tmp = (u32) obj->integer.value;
 
+	pr_debug("Result: 0x%08x\n", tmp);
 	if (retval)
 		*retval = tmp;
 
 	kfree(obj);
 
-	if (tmp == ASUS_WMI_UNSUPPORTED_METHOD)
+	if (tmp == ASUS_WMI_UNSUPPORTED_METHOD) {
+		pr_debug("%s, (0x%08x), arg 0x%08x failed: %d\n",
+			__func__, method_id, arg0, -ENODEV);
 		return -ENODEV;
+	}
 
 	return 0;
 }
@@ -377,20 +386,29 @@ static int asus_wmi_evaluate_method5(u32 method_id,
 	status = wmi_evaluate_method(ASUS_WMI_MGMT_GUID, 0, method_id,
 				     &input, &output);
 
-	if (ACPI_FAILURE(status))
+	pr_debug("%s called (0x%08x) with args: 0x%08x, 0x%08x, 0x%08x, 0x%08x, 0x%08x\n",
+		__func__, method_id, arg0, arg1, arg2, arg3, arg4);
+	if (ACPI_FAILURE(status)) {
+		pr_debug("%s, (0x%08x), arg 0x%08x failed: %d\n",
+			__func__, method_id, arg0, -EIO);
 		return -EIO;
+	}
 
 	obj = (union acpi_object *)output.pointer;
 	if (obj && obj->type == ACPI_TYPE_INTEGER)
 		tmp = (u32) obj->integer.value;
 
+	pr_debug("Result: %x\n", tmp);
 	if (retval)
 		*retval = tmp;
 
 	kfree(obj);
 
-	if (tmp == ASUS_WMI_UNSUPPORTED_METHOD)
+	if (tmp == ASUS_WMI_UNSUPPORTED_METHOD) {
+		pr_debug("%s, (0x%08x), arg 0x%08x failed: %d\n",
+			__func__, method_id, arg0, -ENODEV);
 		return -ENODEV;
+	}
 
 	return 0;
 }
@@ -416,8 +434,13 @@ static int asus_wmi_evaluate_method_buf(u32 method_id,
 	status = wmi_evaluate_method(ASUS_WMI_MGMT_GUID, 0, method_id,
 				     &input, &output);
 
-	if (ACPI_FAILURE(status))
+	pr_debug("%s called (0x%08x) with args: 0x%08x, 0x%08x\n",
+		__func__, method_id, arg0, arg1);
+	if (ACPI_FAILURE(status)) {
+		pr_debug("%s, (0x%08x), arg 0x%08x failed: %d\n",
+			__func__, method_id, arg0, -EIO);
 		return -EIO;
+	}
 
 	obj = (union acpi_object *)output.pointer;
 
@@ -453,8 +476,11 @@ static int asus_wmi_evaluate_method_buf(u32 method_id,
 
 	kfree(obj);
 
-	if (err)
+	if (err) {
+		pr_debug("%s, (0x%08x), arg 0x%08x failed: %d\n",
+			__func__, method_id, arg0, err);
 		return err;
+	}
 
 	return 0;
 }
@@ -542,6 +568,7 @@ static bool asus_wmi_dev_is_present(struct asus_wmi *asus, u32 dev_id)
 {
 	u32 retval;
 	int status = asus_wmi_get_devstate(asus, dev_id, &retval);
+	pr_debug("%s called (0x%08x), retval: 0x%08x\n", __func__, dev_id, retval);
 
 	return status == 0 && (retval & ASUS_WMI_DSTS_PRESENCE_BIT);
 }
@@ -3559,18 +3586,27 @@ static int asus_wmi_custom_fan_curve_init(struct asus_wmi *asus)
 
 	err = fan_curve_check_present(asus, &asus->cpu_fan_curve_available,
 				      ASUS_WMI_DEVID_CPU_FAN_CURVE);
-	if (err)
+	if (err) {
+		pr_err("%s, checked 0x%08x, failed: %d\n",
+			__func__, ASUS_WMI_DEVID_CPU_FAN_CURVE, err);
 		return err;
+	}
 
 	err = fan_curve_check_present(asus, &asus->gpu_fan_curve_available,
 				      ASUS_WMI_DEVID_GPU_FAN_CURVE);
-	if (err)
+	if (err) {
+		pr_err("%s, checked 0x%08x, failed: %d\n",
+			__func__, ASUS_WMI_DEVID_GPU_FAN_CURVE, err);
 		return err;
+	}
 
 	err = fan_curve_check_present(asus, &asus->mid_fan_curve_available,
 				      ASUS_WMI_DEVID_MID_FAN_CURVE);
-	if (err)
+	if (err) {
+		pr_err("%s, checked 0x%08x, failed: %d\n",
+			__func__, ASUS_WMI_DEVID_MID_FAN_CURVE, err);
 		return err;
+	}
 
 	if (!asus->cpu_fan_curve_available
 		&& !asus->gpu_fan_curve_available
@@ -4398,8 +4434,10 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
 	else if (attr == &dev_attr_available_mini_led_mode.attr)
 		ok = asus->mini_led_dev_id != 0;
 
-	if (devid != -1)
+	if (devid != -1) {
 		ok = !(asus_wmi_get_devstate_simple(asus, devid) < 0);
+		pr_debug("%s called 0x%08x, ok: %x\n", __func__, devid, ok);
+	}
 
 	return ok ? attr->mode : 0;
 }
-- 
2.45.1

[PATCH 2/9] platform/x86: asus-wmi: don't fail if platform_profile already registered

[Luke D. Jones]

On some newer laptops it appears that an AMD driver can register a
platform_profile handler. If this happens then the asus_wmi driver would
error with -EEXIST when trying to register its own handler.

We can safely continue loading the driver instead of bombing out.

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

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index adc841abbb2a..999cd658ec8b 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -3836,8 +3836,13 @@ static int platform_profile_setup(struct asus_wmi *asus)
 		asus->platform_profile_handler.choices);
 
 	err = platform_profile_register(&asus->platform_profile_handler);
-	if (err)
+	if (err == -EEXIST) {
+		pr_warn("%s, a platform_profile handler is already registered\n", __func__);
+		return 0;
+	} else if (err) {
+		pr_err("%s, failed at platform_profile_register: %d\n", __func__, err);
 		return err;
+	}
 
 	asus->platform_profile_support = true;
 	return 0;
@@ -4713,7 +4718,7 @@ static int asus_wmi_add(struct platform_device *pdev)
 		throttle_thermal_policy_set_default(asus);
 
 	err = platform_profile_setup(asus);
-	if (err)
+	if (err && err != -EEXIST)
 		goto fail_platform_profile_setup;
 
 	err = asus_wmi_sysfs_init(asus->platform_device);
-- 
2.45.1

[PATCH 3/9] platform/x86: asus-wmi: add macros and expose min/max sysfs for ppt tunables

[Luke D. Jones]

In most cases the safe min and max values of the various PPT tunables are
known for various ASUS ROG (and other) laptop models. We can match the
DMI string for these and expose min/max sysfs points, plus set some sane
default values.

As part of the addition of the min/max and defaults, to reduce the amount
of code copy/paste and introduce some sanity a group of macros were added
specific to the PPT and NV tunables. The code becomes much cleaner and
easier to read.

This makes the PPT functions much more usable and safe.

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 .../ABI/testing/sysfs-platform-asus-wmi       |  23 +-
 drivers/platform/x86/asus-wmi.c               | 561 ++++++++----------
 2 files changed, 245 insertions(+), 339 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index 28144371a0f1..984a04f32fd0 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -142,8 +142,8 @@ Contact:	"Luke Jones" <luke@ljones.dev>
 Description:
 		Set the Package Power Target total of CPU: PL1 on Intel, SPL on AMD.
 		Shown on Intel+Nvidia or AMD+Nvidia based systems:
-
-			* min=5, max=250
+            * min/max varies, read *_min/*_max sysfs entries
+			* -1 resets to default
 
 What:		/sys/devices/platform/<platform>/ppt_pl2_sppt
 Date:		Jun 2023
@@ -152,8 +152,8 @@ Contact:	"Luke Jones" <luke@ljones.dev>
 Description:
 		Set the Slow Package Power Tracking Limit of CPU: PL2 on Intel, SPPT,
 		on AMD. Shown on Intel+Nvidia or AMD+Nvidia based systems:
-
-			* min=5, max=250
+            * min/max varies, read *_min/*_max sysfs entries
+			* -1 resets to default
 
 What:		/sys/devices/platform/<platform>/ppt_fppt
 Date:		Jun 2023
@@ -161,7 +161,8 @@ KernelVersion:	6.5
 Contact:	"Luke Jones" <luke@ljones.dev>
 Description:
 		Set the Fast Package Power Tracking Limit of CPU. AMD+Nvidia only:
-			* min=5, max=250
+			* min/max varies, read *_min/*_max sysfs entries
+			* -1 resets to default
 
 What:		/sys/devices/platform/<platform>/ppt_apu_sppt
 Date:		Jun 2023
@@ -169,7 +170,8 @@ KernelVersion:	6.5
 Contact:	"Luke Jones" <luke@ljones.dev>
 Description:
 		Set the APU SPPT limit. Shown on full AMD systems only:
-			* min=5, max=130
+			* min/max varies, read *_min/*_max sysfs entries
+			* -1 resets to default
 
 What:		/sys/devices/platform/<platform>/ppt_platform_sppt
 Date:		Jun 2023
@@ -177,7 +179,8 @@ KernelVersion:	6.5
 Contact:	"Luke Jones" <luke@ljones.dev>
 Description:
 		Set the platform SPPT limit. Shown on full AMD systems only:
-			* min=5, max=130
+			* min/max varies, read *_min/*_max sysfs entries
+			* -1 resets to default
 
 What:		/sys/devices/platform/<platform>/nv_dynamic_boost
 Date:		Jun 2023
@@ -185,7 +188,8 @@ KernelVersion:	6.5
 Contact:	"Luke Jones" <luke@ljones.dev>
 Description:
 		Set the dynamic boost limit of the Nvidia dGPU:
-			* min=5, max=25
+			* min/max varies, read *_min/*_max sysfs entries
+			* -1 resets to default
 
 What:		/sys/devices/platform/<platform>/nv_temp_target
 Date:		Jun 2023
@@ -193,7 +197,8 @@ KernelVersion:	6.5
 Contact:	"Luke Jones" <luke@ljones.dev>
 Description:
 		Set the target temperature limit of the Nvidia dGPU:
-			* min=75, max=87
+			* min/max varies, read *_min/*_max sysfs entries
+			* -1 resets to default
 
 What:		/sys/devices/platform/<platform>/boot_sound
 Date:		Apr 2024
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 999cd658ec8b..d016acb23789 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -112,11 +112,13 @@ module_param(fnlock_default, bool, 0444);
 /* Mask to determine if setting temperature or percentage */
 #define FAN_CURVE_PWM_MASK		0x04
 
-/* Limits for tunables available on ASUS ROG laptops */
-#define PPT_TOTAL_MIN		5
-#define PPT_TOTAL_MAX		250
-#define PPT_CPU_MIN			5
-#define PPT_CPU_MAX			130
+/* Default limits for tunables available on ASUS ROG laptops */
+#define PPT_CPU_LIMIT_MIN	5
+#define PPT_CPU_LIMIT_MAX	150
+#define PPT_CPU_LIMIT_DEFAULT	80
+#define PPT_PLATFORM_MIN	5
+#define PPT_PLATFORM_MAX	100
+#define PPT_PLATFORM_DEFAULT	80
 #define NVIDIA_BOOST_MIN	5
 #define NVIDIA_BOOST_MAX	25
 #define NVIDIA_TEMP_MIN		75
@@ -219,6 +221,29 @@ struct fan_curve_data {
 	u8 percents[FAN_CURVE_POINTS];
 };
 
+/* Tunables provided by ASUS for gaming laptops */
+struct rog_tunables {
+	u32 cpu_default;
+	u32 cpu_max;
+
+	u32 platform_default;
+	u32 platform_max;
+
+	u32 ppt_pl1_spl; // total
+	u32 ppt_pl2_sppt; // total
+	u32 ppt_apu_sppt; // cpu
+	u32 ppt_platform_sppt; // cpu
+	u32 ppt_fppt; // total
+
+	u32 nv_boost_default;
+	u32 nv_boost_max;
+	u32 nv_dynamic_boost;
+
+	u32 nv_temp_default;
+	u32 nv_temp_max;
+	u32 nv_temp_target;
+};
+
 struct asus_wmi {
 	int dsts_id;
 	int spec;
@@ -273,14 +298,7 @@ struct asus_wmi {
 	bool dgpu_disable_available;
 	u32 gpu_mux_dev;
 
-	/* Tunables provided by ASUS for gaming laptops */
-	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;
+	struct rog_tunables rog_tunables;
 
 	u32 kbd_rgb_dev;
 	bool kbd_rgb_state_available;
@@ -652,6 +670,98 @@ static void asus_wmi_input_exit(struct asus_wmi *asus)
 	asus->inputdev = NULL;
 }
 
+/* Helper macros for generalised WMI calls */
+
+/* Generic store function for use with many ROG tunables */
+static ssize_t rog_tunable_store(struct asus_wmi *asus,
+				struct attribute *attr,
+				const char *buf, size_t count,
+				u32 min, u32 max, u32 defaultv,
+				u32 *store_value, u32 wmi_dev)
+{
+	int result, err, value;
+
+	result = kstrtoint(buf, 10, &value);
+	if (result)
+		return result;
+
+	if (value == -1 )
+		value = defaultv;
+	if (value < min || value > max)
+		return -EINVAL;
+
+	err = asus_wmi_set_devstate(wmi_dev, value, &result);
+	if (err) {
+		pr_err("Failed to set %s: %d\n", attr->name, err);
+		return err;
+	}
+
+	if (result > 1) {
+		pr_err("Failed to set %s (result): 0x%x\n", attr->name, result);
+		return -EIO;
+	}
+
+	if (store_value != NULL)
+		*store_value = value;
+	sysfs_notify(&asus->platform_device->dev.kobj, NULL, attr->name);
+
+	return count;
+}
+
+#define ROG_TUNABLE_STORE(_fname, _min, _max, _default, _wmi) \
+static ssize_t _fname##_store(struct device *dev, \
+	struct device_attribute *attr, const char *buf, size_t count) \
+{ \
+	struct asus_wmi *asus = dev_get_drvdata(dev); \
+	return rog_tunable_store(asus, &attr->attr, buf, count, \
+			_min, asus->rog_tunables._max, asus->rog_tunables._default, \
+			&asus->rog_tunables._fname, _wmi); \
+}
+
+#define ROG_TUNABLE_SHOW(_fname) \
+static ssize_t _fname##_show(struct device *dev, struct device_attribute *attr, char *buf) \
+{ \
+	struct asus_wmi *asus = dev_get_drvdata(dev); \
+	return sysfs_emit(buf, "%u\n", asus->rog_tunables._fname); \
+}
+
+#define ROG_TUNABLE_MIN_SHOW(_fname, _minv) \
+static ssize_t _fname##_min_show(struct device *dev, struct device_attribute *attr, char *buf) \
+{ \
+	return sysfs_emit(buf, "%u\n", _minv); \
+}
+
+#define ROG_TUNABLE_MAX_SHOW(_fname, _maxv) \
+static ssize_t _fname##_max_show(struct device *dev, struct device_attribute *attr, char *buf) \
+{ \
+	struct asus_wmi *asus = dev_get_drvdata(dev); \
+	return sysfs_emit(buf, "%u\n", asus->rog_tunables._maxv); \
+}
+
+#define ROG_ATTR_RW(_fname, _minv, _maxv, _defaultv, _wmi) \
+ROG_TUNABLE_MIN_SHOW(_fname, _minv); \
+ROG_TUNABLE_MAX_SHOW(_fname, _maxv); \
+ROG_TUNABLE_STORE(_fname, _minv, _maxv, _defaultv, _wmi);\
+ROG_TUNABLE_SHOW(_fname); \
+static DEVICE_ATTR_RO(_fname##_min); \
+static DEVICE_ATTR_RO(_fname##_max); \
+static DEVICE_ATTR_RW(_fname)
+
+ROG_ATTR_RW(ppt_platform_sppt,
+	PPT_PLATFORM_MIN, platform_max, platform_default, ASUS_WMI_DEVID_PPT_PLAT_SPPT);
+ROG_ATTR_RW(ppt_pl2_sppt,
+	PPT_CPU_LIMIT_MIN, cpu_max, cpu_default, ASUS_WMI_DEVID_PPT_PL2_SPPT);
+ROG_ATTR_RW(ppt_apu_sppt,
+	PPT_PLATFORM_MIN, platform_max, platform_default, ASUS_WMI_DEVID_PPT_APU_SPPT);
+ROG_ATTR_RW(ppt_pl1_spl,
+	PPT_CPU_LIMIT_MIN, cpu_max, cpu_default, ASUS_WMI_DEVID_PPT_PL1_SPL);
+ROG_ATTR_RW(ppt_fppt,
+	PPT_CPU_LIMIT_MIN, cpu_max, cpu_default, ASUS_WMI_DEVID_PPT_FPPT);
+ROG_ATTR_RW(nv_dynamic_boost,
+	NVIDIA_BOOST_MIN, nv_boost_max, nv_boost_default, ASUS_WMI_DEVID_NV_DYN_BOOST);
+ROG_ATTR_RW(nv_temp_target,
+	NVIDIA_TEMP_MIN, nv_temp_max, nv_temp_default, ASUS_WMI_DEVID_NV_THERM_TARGET);
+
 /* Tablet mode ****************************************************************/
 
 static void asus_wmi_tablet_mode_get_state(struct asus_wmi *asus)
@@ -1018,306 +1128,6 @@ static const struct attribute_group *kbd_rgb_mode_groups[] = {
 	NULL,
 };
 
-/* Tunable: PPT: Intel=PL1, AMD=SPPT *****************************************/
-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;
-
-	result = kstrtou32(buf, 10, &value);
-	if (result)
-		return result;
-
-	if (value < PPT_TOTAL_MIN || value > PPT_TOTAL_MAX)
-		return -EINVAL;
-
-	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_PPT_PL2_SPPT, value, &result);
-	if (err) {
-		pr_warn("Failed to set ppt_pl2_sppt: %d\n", err);
-		return err;
-	}
-
-	if (result > 1) {
-		pr_warn("Failed to set ppt_pl2_sppt (result): 0x%x\n", result);
-		return -EIO;
-	}
-
-	asus->ppt_pl2_sppt = value;
-	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "ppt_pl2_sppt");
-
-	return count;
-}
-
-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, "%u\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,
-				    struct device_attribute *attr,
-				    const char *buf, size_t count)
-{
-	struct asus_wmi *asus = dev_get_drvdata(dev);
-	int result, err;
-	u32 value;
-
-	result = kstrtou32(buf, 10, &value);
-	if (result)
-		return result;
-
-	if (value < PPT_TOTAL_MIN || value > PPT_TOTAL_MAX)
-		return -EINVAL;
-
-	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_PPT_PL1_SPL, value, &result);
-	if (err) {
-		pr_warn("Failed to set ppt_pl1_spl: %d\n", err);
-		return err;
-	}
-
-	if (result > 1) {
-		pr_warn("Failed to set ppt_pl1_spl (result): 0x%x\n", result);
-		return -EIO;
-	}
-
-	asus->ppt_pl1_spl = value;
-	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "ppt_pl1_spl");
-
-	return count;
-}
-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, "%u\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,
-				    struct device_attribute *attr,
-				    const char *buf, size_t count)
-{
-	struct asus_wmi *asus = dev_get_drvdata(dev);
-	int result, err;
-	u32 value;
-
-	result = kstrtou32(buf, 10, &value);
-	if (result)
-		return result;
-
-	if (value < PPT_TOTAL_MIN || value > PPT_TOTAL_MAX)
-		return -EINVAL;
-
-	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_PPT_FPPT, value, &result);
-	if (err) {
-		pr_warn("Failed to set ppt_fppt: %d\n", err);
-		return err;
-	}
-
-	if (result > 1) {
-		pr_warn("Failed to set ppt_fppt (result): 0x%x\n", result);
-		return -EIO;
-	}
-
-	asus->ppt_fppt = value;
-	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "ppt_fpu_sppt");
-
-	return count;
-}
-
-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, "%u\n", asus->ppt_fppt);
-}
-static DEVICE_ATTR_RW(ppt_fppt);
-
-/* Tunable: PPT APU SPPT *****************************************************/
-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;
-
-	result = kstrtou32(buf, 10, &value);
-	if (result)
-		return result;
-
-	if (value < PPT_CPU_MIN || value > PPT_CPU_MAX)
-		return -EINVAL;
-
-	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_PPT_APU_SPPT, value, &result);
-	if (err) {
-		pr_warn("Failed to set ppt_apu_sppt: %d\n", err);
-		return err;
-	}
-
-	if (result > 1) {
-		pr_warn("Failed to set ppt_apu_sppt (result): 0x%x\n", result);
-		return -EIO;
-	}
-
-	asus->ppt_apu_sppt = value;
-	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "ppt_apu_sppt");
-
-	return count;
-}
-
-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, "%u\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,
-				    struct device_attribute *attr,
-				    const char *buf, size_t count)
-{
-	struct asus_wmi *asus = dev_get_drvdata(dev);
-	int result, err;
-	u32 value;
-
-	result = kstrtou32(buf, 10, &value);
-	if (result)
-		return result;
-
-	if (value < PPT_CPU_MIN || value > PPT_CPU_MAX)
-		return -EINVAL;
-
-	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_PPT_PLAT_SPPT, value, &result);
-	if (err) {
-		pr_warn("Failed to set ppt_platform_sppt: %d\n", err);
-		return err;
-	}
-
-	if (result > 1) {
-		pr_warn("Failed to set ppt_platform_sppt (result): 0x%x\n", result);
-		return -EIO;
-	}
-
-	asus->ppt_platform_sppt = value;
-	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "ppt_platform_sppt");
-
-	return count;
-}
-
-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, "%u\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,
-				    struct device_attribute *attr,
-				    const char *buf, size_t count)
-{
-	struct asus_wmi *asus = dev_get_drvdata(dev);
-	int result, err;
-	u32 value;
-
-	result = kstrtou32(buf, 10, &value);
-	if (result)
-		return result;
-
-	if (value < NVIDIA_BOOST_MIN || value > NVIDIA_BOOST_MAX)
-		return -EINVAL;
-
-	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_NV_DYN_BOOST, value, &result);
-	if (err) {
-		pr_warn("Failed to set nv_dynamic_boost: %d\n", err);
-		return err;
-	}
-
-	if (result > 1) {
-		pr_warn("Failed to set nv_dynamic_boost (result): 0x%x\n", result);
-		return -EIO;
-	}
-
-	asus->nv_dynamic_boost = value;
-	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "nv_dynamic_boost");
-
-	return count;
-}
-
-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, "%u\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,
-				    struct device_attribute *attr,
-				    const char *buf, size_t count)
-{
-	struct asus_wmi *asus = dev_get_drvdata(dev);
-	int result, err;
-	u32 value;
-
-	result = kstrtou32(buf, 10, &value);
-	if (result)
-		return result;
-
-	if (value < NVIDIA_TEMP_MIN || value > NVIDIA_TEMP_MAX)
-		return -EINVAL;
-
-	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_NV_THERM_TARGET, value, &result);
-	if (err) {
-		pr_warn("Failed to set nv_temp_target: %d\n", err);
-		return err;
-	}
-
-	if (result > 1) {
-		pr_warn("Failed to set nv_temp_target (result): 0x%x\n", result);
-		return -EIO;
-	}
-
-	asus->nv_temp_target = value;
-	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "nv_temp_target");
-
-	return count;
-}
-
-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, "%u\n", asus->nv_temp_target);
-}
-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)
@@ -4367,13 +4177,27 @@ static struct attribute *platform_attributes[] = {
 	&dev_attr_als_enable.attr,
 	&dev_attr_fan_boost_mode.attr,
 	&dev_attr_throttle_thermal_policy.attr,
-	&dev_attr_ppt_pl2_sppt.attr,
 	&dev_attr_ppt_pl1_spl.attr,
+	&dev_attr_ppt_pl1_spl_min.attr,
+	&dev_attr_ppt_pl1_spl_max.attr,
+	&dev_attr_ppt_pl2_sppt.attr,
+	&dev_attr_ppt_pl2_sppt_min.attr,
+	&dev_attr_ppt_pl2_sppt_max.attr,
 	&dev_attr_ppt_fppt.attr,
+	&dev_attr_ppt_fppt_min.attr,
+	&dev_attr_ppt_fppt_max.attr,
 	&dev_attr_ppt_apu_sppt.attr,
+	&dev_attr_ppt_apu_sppt_min.attr,
+	&dev_attr_ppt_apu_sppt_max.attr,
 	&dev_attr_ppt_platform_sppt.attr,
+	&dev_attr_ppt_platform_sppt_min.attr,
+	&dev_attr_ppt_platform_sppt_max.attr,
 	&dev_attr_nv_dynamic_boost.attr,
+	&dev_attr_nv_dynamic_boost_min.attr,
+	&dev_attr_nv_dynamic_boost_max.attr,
 	&dev_attr_nv_temp_target.attr,
+	&dev_attr_nv_temp_target_min.attr,
+	&dev_attr_nv_temp_target_max.attr,
 	&dev_attr_mcu_powersave.attr,
 	&dev_attr_boot_sound.attr,
 	&dev_attr_panel_od.attr,
@@ -4414,19 +4238,33 @@ 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_ppt_pl2_sppt.attr)
+	else if (attr == &dev_attr_ppt_pl2_sppt.attr
+		|| attr == &dev_attr_ppt_pl2_sppt_min.attr
+		|| attr == &dev_attr_ppt_pl2_sppt_max.attr)
 		devid = ASUS_WMI_DEVID_PPT_PL2_SPPT;
-	else if (attr == &dev_attr_ppt_pl1_spl.attr)
+	else if (attr == &dev_attr_ppt_pl1_spl.attr
+		|| attr == &dev_attr_ppt_pl1_spl_min.attr
+		|| attr == &dev_attr_ppt_pl1_spl_max.attr)
 		devid = ASUS_WMI_DEVID_PPT_PL1_SPL;
-	else if (attr == &dev_attr_ppt_fppt.attr)
+	else if (attr == &dev_attr_ppt_fppt.attr
+		|| attr == &dev_attr_ppt_fppt_min.attr
+		|| attr == &dev_attr_ppt_fppt_max.attr)
 		devid = ASUS_WMI_DEVID_PPT_FPPT;
-	else if (attr == &dev_attr_ppt_apu_sppt.attr)
+	else if (attr == &dev_attr_ppt_apu_sppt.attr
+		|| attr == &dev_attr_ppt_apu_sppt_min.attr
+		|| attr == &dev_attr_ppt_apu_sppt_max.attr)
 		devid = ASUS_WMI_DEVID_PPT_APU_SPPT;
-	else if (attr == &dev_attr_ppt_platform_sppt.attr)
+	else if (attr == &dev_attr_ppt_platform_sppt.attr
+		|| attr == &dev_attr_ppt_platform_sppt_min.attr
+		|| attr == &dev_attr_ppt_platform_sppt_max.attr)
 		devid = ASUS_WMI_DEVID_PPT_PLAT_SPPT;
-	else if (attr == &dev_attr_nv_dynamic_boost.attr)
+	else if (attr == &dev_attr_nv_dynamic_boost.attr
+		|| attr == &dev_attr_nv_dynamic_boost_min.attr
+		|| attr == &dev_attr_nv_dynamic_boost_max.attr)
 		devid = ASUS_WMI_DEVID_NV_DYN_BOOST;
-	else if (attr == &dev_attr_nv_temp_target.attr)
+	else if (attr == &dev_attr_nv_temp_target.attr
+		|| attr == &dev_attr_nv_temp_target_min.attr
+		|| attr == &dev_attr_nv_temp_target_max.attr)
 		devid = ASUS_WMI_DEVID_NV_THERM_TARGET;
 	else if (attr == &dev_attr_mcu_powersave.attr)
 		devid = ASUS_WMI_DEVID_MCU_POWERSAVE;
@@ -4652,6 +4490,77 @@ static void asus_wmi_debugfs_init(struct asus_wmi *asus)
 
 /* Init / exit ****************************************************************/
 
+/* Set up the min/max and defaults for ROG tunables */
+static void init_rog_tunables(struct asus_wmi *asus)
+{
+	const char *product;
+	u32 max_boost = NVIDIA_BOOST_MAX;
+	u32 cpu_default = PPT_CPU_LIMIT_DEFAULT;
+	u32 cpu_max = PPT_CPU_LIMIT_MAX;
+	u32 platform_default = PPT_PLATFORM_DEFAULT;
+	u32 platform_max = PPT_PLATFORM_MAX;
+
+	/*
+	 * ASUS product_name contains everything required, e.g,
+	 * "ROG Flow X16 GV601VV_GV601VV_00185149B"
+	 */
+	product = dmi_get_system_info(DMI_PRODUCT_NAME);
+
+	if (strstr(product, "GA402R")) {
+		cpu_default = 125;
+	} else if (strstr(product, "13QY")) {
+		cpu_max = 250;
+	} else if (strstr(product, "X13")) {
+		cpu_max = 75;
+		cpu_default = 50;
+	} else if (strstr(product, "RC71")) {
+		cpu_max = 50;
+		cpu_default = 30;
+	} else if (strstr(product, "G814")
+		|| strstr(product, "G614")
+		|| strstr(product, "G834")
+		|| strstr(product, "G634")) {
+		cpu_max = 175;
+	} else if (strstr(product, "GA402X")
+		|| strstr(product, "GA403")
+		|| strstr(product, "FA507N")
+		|| strstr(product, "FA507X")
+		|| strstr(product, "FA707N")
+		|| strstr(product, "FA707X")) {
+		cpu_max = 90;
+	}
+
+	if (strstr(product, "GZ301ZE"))
+		max_boost = 5;
+	else if (strstr(product, "FX507ZC4"))
+		max_boost = 15;
+	else if (strstr(product, "GU605"))
+		max_boost = 20;
+
+	/* ensure defaults for tunables */
+	asus->rog_tunables.cpu_default = cpu_default;
+	asus->rog_tunables.cpu_max = cpu_max;
+
+	asus->rog_tunables.platform_default = platform_default;
+	asus->rog_tunables.platform_max = platform_max;
+
+	asus->rog_tunables.ppt_pl1_spl = cpu_default;
+	asus->rog_tunables.ppt_pl2_sppt = cpu_default;
+	asus->rog_tunables.ppt_apu_sppt = cpu_default;
+
+	asus->rog_tunables.ppt_platform_sppt = platform_default;
+	asus->rog_tunables.ppt_fppt = platform_default;
+
+	asus->rog_tunables.nv_boost_default = NVIDIA_BOOST_MAX;
+	asus->rog_tunables.nv_boost_max = max_boost;
+	asus->rog_tunables.nv_dynamic_boost = NVIDIA_BOOST_MIN;
+
+	asus->rog_tunables.nv_temp_default = NVIDIA_TEMP_MAX;
+	asus->rog_tunables.nv_temp_max = NVIDIA_TEMP_MAX;
+	asus->rog_tunables.nv_temp_target = NVIDIA_TEMP_MIN;
+
+}
+
 static int asus_wmi_add(struct platform_device *pdev)
 {
 	struct platform_driver *pdrv = to_platform_driver(pdev->dev.driver);
@@ -4677,15 +4586,7 @@ 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;
-
+	init_rog_tunables(asus);
 	asus->egpu_enable_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_EGPU);
 	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);
-- 
2.45.1

Re: [PATCH 3/9] platform/x86: asus-wmi: add macros and expose min/max sysfs for ppt tunables

[Ilpo Järvinen]

On Tue, 28 May 2024, Luke D. Jones wrote:

> In most cases the safe min and max values of the various PPT tunables are
> known for various ASUS ROG (and other) laptop models. We can match the
> DMI string for these and expose min/max sysfs points, plus set some sane
> default values.
> 
> As part of the addition of the min/max and defaults, to reduce the amount
> of code copy/paste and introduce some sanity a group of macros were added
> specific to the PPT and NV tunables. The code becomes much cleaner and
> easier to read.
> 
> This makes the PPT functions much more usable and safe.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>  .../ABI/testing/sysfs-platform-asus-wmi       |  23 +-
>  drivers/platform/x86/asus-wmi.c               | 561 ++++++++----------
>  2 files changed, 245 insertions(+), 339 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> index 28144371a0f1..984a04f32fd0 100644
> --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> @@ -142,8 +142,8 @@ Contact:	"Luke Jones" <luke@ljones.dev>
>  Description:
>  		Set the Package Power Target total of CPU: PL1 on Intel, SPL on AMD.
>  		Shown on Intel+Nvidia or AMD+Nvidia based systems:
> -
> -			* min=5, max=250
> +            * min/max varies, read *_min/*_max sysfs entries
> +			* -1 resets to default
>  
>  What:		/sys/devices/platform/<platform>/ppt_pl2_sppt
>  Date:		Jun 2023
> @@ -152,8 +152,8 @@ Contact:	"Luke Jones" <luke@ljones.dev>
>  Description:
>  		Set the Slow Package Power Tracking Limit of CPU: PL2 on Intel, SPPT,
>  		on AMD. Shown on Intel+Nvidia or AMD+Nvidia based systems:
> -
> -			* min=5, max=250
> +            * min/max varies, read *_min/*_max sysfs entries
> +			* -1 resets to default
>  
>  What:		/sys/devices/platform/<platform>/ppt_fppt
>  Date:		Jun 2023
> @@ -161,7 +161,8 @@ KernelVersion:	6.5
>  Contact:	"Luke Jones" <luke@ljones.dev>
>  Description:
>  		Set the Fast Package Power Tracking Limit of CPU. AMD+Nvidia only:
> -			* min=5, max=250
> +			* min/max varies, read *_min/*_max sysfs entries
> +			* -1 resets to default
>  
>  What:		/sys/devices/platform/<platform>/ppt_apu_sppt
>  Date:		Jun 2023
> @@ -169,7 +170,8 @@ KernelVersion:	6.5
>  Contact:	"Luke Jones" <luke@ljones.dev>
>  Description:
>  		Set the APU SPPT limit. Shown on full AMD systems only:
> -			* min=5, max=130
> +			* min/max varies, read *_min/*_max sysfs entries
> +			* -1 resets to default
>  
>  What:		/sys/devices/platform/<platform>/ppt_platform_sppt
>  Date:		Jun 2023
> @@ -177,7 +179,8 @@ KernelVersion:	6.5
>  Contact:	"Luke Jones" <luke@ljones.dev>
>  Description:
>  		Set the platform SPPT limit. Shown on full AMD systems only:
> -			* min=5, max=130
> +			* min/max varies, read *_min/*_max sysfs entries
> +			* -1 resets to default
>  
>  What:		/sys/devices/platform/<platform>/nv_dynamic_boost
>  Date:		Jun 2023
> @@ -185,7 +188,8 @@ KernelVersion:	6.5
>  Contact:	"Luke Jones" <luke@ljones.dev>
>  Description:
>  		Set the dynamic boost limit of the Nvidia dGPU:
> -			* min=5, max=25
> +			* min/max varies, read *_min/*_max sysfs entries
> +			* -1 resets to default
>  
>  What:		/sys/devices/platform/<platform>/nv_temp_target
>  Date:		Jun 2023
> @@ -193,7 +197,8 @@ KernelVersion:	6.5
>  Contact:	"Luke Jones" <luke@ljones.dev>
>  Description:
>  		Set the target temperature limit of the Nvidia dGPU:
> -			* min=75, max=87
> +			* min/max varies, read *_min/*_max sysfs entries
> +			* -1 resets to default
>  
>  What:		/sys/devices/platform/<platform>/boot_sound
>  Date:		Apr 2024
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 999cd658ec8b..d016acb23789 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -112,11 +112,13 @@ module_param(fnlock_default, bool, 0444);
>  /* Mask to determine if setting temperature or percentage */
>  #define FAN_CURVE_PWM_MASK		0x04
>  
> -/* Limits for tunables available on ASUS ROG laptops */
> -#define PPT_TOTAL_MIN		5
> -#define PPT_TOTAL_MAX		250
> -#define PPT_CPU_MIN			5
> -#define PPT_CPU_MAX			130
> +/* Default limits for tunables available on ASUS ROG laptops */
> +#define PPT_CPU_LIMIT_MIN	5
> +#define PPT_CPU_LIMIT_MAX	150
> +#define PPT_CPU_LIMIT_DEFAULT	80
> +#define PPT_PLATFORM_MIN	5
> +#define PPT_PLATFORM_MAX	100
> +#define PPT_PLATFORM_DEFAULT	80
>  #define NVIDIA_BOOST_MIN	5
>  #define NVIDIA_BOOST_MAX	25
>  #define NVIDIA_TEMP_MIN		75
> @@ -219,6 +221,29 @@ struct fan_curve_data {
>  	u8 percents[FAN_CURVE_POINTS];
>  };
>  
> +/* Tunables provided by ASUS for gaming laptops */
> +struct rog_tunables {
> +	u32 cpu_default;
> +	u32 cpu_max;
> +
> +	u32 platform_default;
> +	u32 platform_max;
> +
> +	u32 ppt_pl1_spl; // total
> +	u32 ppt_pl2_sppt; // total
> +	u32 ppt_apu_sppt; // cpu
> +	u32 ppt_platform_sppt; // cpu
> +	u32 ppt_fppt; // total
> +
> +	u32 nv_boost_default;
> +	u32 nv_boost_max;
> +	u32 nv_dynamic_boost;
> +
> +	u32 nv_temp_default;
> +	u32 nv_temp_max;
> +	u32 nv_temp_target;
> +};
> +
>  struct asus_wmi {
>  	int dsts_id;
>  	int spec;
> @@ -273,14 +298,7 @@ struct asus_wmi {
>  	bool dgpu_disable_available;
>  	u32 gpu_mux_dev;
>  
> -	/* Tunables provided by ASUS for gaming laptops */
> -	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;
> +	struct rog_tunables rog_tunables;
>  
>  	u32 kbd_rgb_dev;
>  	bool kbd_rgb_state_available;
> @@ -652,6 +670,98 @@ static void asus_wmi_input_exit(struct asus_wmi *asus)
>  	asus->inputdev = NULL;
>  }
>  
> +/* Helper macros for generalised WMI calls */
> +
> +/* Generic store function for use with many ROG tunables */
> +static ssize_t rog_tunable_store(struct asus_wmi *asus,
> +				struct attribute *attr,
> +				const char *buf, size_t count,
> +				u32 min, u32 max, u32 defaultv,
> +				u32 *store_value, u32 wmi_dev)
> +{
> +	int result, err, value;
> +
> +	result = kstrtoint(buf, 10, &value);
> +	if (result)
> +		return result;
> +
> +	if (value == -1 )

Remove the extra space.

> +		value = defaultv;
> +	if (value < min || value > max)
> +		return -EINVAL;
> +
> +	err = asus_wmi_set_devstate(wmi_dev, value, &result);
> +	if (err) {
> +		pr_err("Failed to set %s: %d\n", attr->name, err);
> +		return err;
> +	}
> +
> +	if (result > 1) {

Remove the empty line in between since this is still the error handling of 
the same call.

> +		pr_err("Failed to set %s (result): 0x%x\n", attr->name, result);
> +		return -EIO;
> +	}
> +
> +	if (store_value != NULL)
> +		*store_value = value;
> +	sysfs_notify(&asus->platform_device->dev.kobj, NULL, attr->name);
> +
> +	return count;
> +}
> +
> +#define ROG_TUNABLE_STORE(_fname, _min, _max, _default, _wmi) \
> +static ssize_t _fname##_store(struct device *dev, \
> +	struct device_attribute *attr, const char *buf, size_t count) \
> +{ \
> +	struct asus_wmi *asus = dev_get_drvdata(dev); \
> +	return rog_tunable_store(asus, &attr->attr, buf, count, \
> +			_min, asus->rog_tunables._max, asus->rog_tunables._default, \
> +			&asus->rog_tunables._fname, _wmi); \
> +}

For all function macros like this, align all the continuation backslashed 
with tabs somewhere far right so they don't interfere with the code.

> +#define ROG_TUNABLE_SHOW(_fname) \
> +static ssize_t _fname##_show(struct device *dev, struct device_attribute *attr, char *buf) \
> +{ \
> +	struct asus_wmi *asus = dev_get_drvdata(dev); \
> +	return sysfs_emit(buf, "%u\n", asus->rog_tunables._fname); \
> +}
> +
> +#define ROG_TUNABLE_MIN_SHOW(_fname, _minv) \
> +static ssize_t _fname##_min_show(struct device *dev, struct device_attribute *attr, char *buf) \
> +{ \
> +	return sysfs_emit(buf, "%u\n", _minv); \
> +}
> +
> +#define ROG_TUNABLE_MAX_SHOW(_fname, _maxv) \
> +static ssize_t _fname##_max_show(struct device *dev, struct device_attribute *attr, char *buf) \
> +{ \
> +	struct asus_wmi *asus = dev_get_drvdata(dev); \
> +	return sysfs_emit(buf, "%u\n", asus->rog_tunables._maxv); \
> +}
> +
> +#define ROG_ATTR_RW(_fname, _minv, _maxv, _defaultv, _wmi) \
> +ROG_TUNABLE_MIN_SHOW(_fname, _minv); \
> +ROG_TUNABLE_MAX_SHOW(_fname, _maxv); \
> +ROG_TUNABLE_STORE(_fname, _minv, _maxv, _defaultv, _wmi);\
> +ROG_TUNABLE_SHOW(_fname); \
> +static DEVICE_ATTR_RO(_fname##_min); \
> +static DEVICE_ATTR_RO(_fname##_max); \
> +static DEVICE_ATTR_RW(_fname)

Also here, align \ to right with \ so it doesn't interfere with the real 
content of the macro.

> +
> +ROG_ATTR_RW(ppt_platform_sppt,
> +	PPT_PLATFORM_MIN, platform_max, platform_default, ASUS_WMI_DEVID_PPT_PLAT_SPPT);
> +ROG_ATTR_RW(ppt_pl2_sppt,
> +	PPT_CPU_LIMIT_MIN, cpu_max, cpu_default, ASUS_WMI_DEVID_PPT_PL2_SPPT);
> +ROG_ATTR_RW(ppt_apu_sppt,
> +	PPT_PLATFORM_MIN, platform_max, platform_default, ASUS_WMI_DEVID_PPT_APU_SPPT);
> +ROG_ATTR_RW(ppt_pl1_spl,
> +	PPT_CPU_LIMIT_MIN, cpu_max, cpu_default, ASUS_WMI_DEVID_PPT_PL1_SPL);
> +ROG_ATTR_RW(ppt_fppt,
> +	PPT_CPU_LIMIT_MIN, cpu_max, cpu_default, ASUS_WMI_DEVID_PPT_FPPT);
> +ROG_ATTR_RW(nv_dynamic_boost,
> +	NVIDIA_BOOST_MIN, nv_boost_max, nv_boost_default, ASUS_WMI_DEVID_NV_DYN_BOOST);
> +ROG_ATTR_RW(nv_temp_target,
> +	NVIDIA_TEMP_MIN, nv_temp_max, nv_temp_default, ASUS_WMI_DEVID_NV_THERM_TARGET);
> +
>  /* Tablet mode ****************************************************************/
>  
>  static void asus_wmi_tablet_mode_get_state(struct asus_wmi *asus)
> @@ -1018,306 +1128,6 @@ static const struct attribute_group *kbd_rgb_mode_groups[] = {
>  	NULL,
>  };
>  
> -/* Tunable: PPT: Intel=PL1, AMD=SPPT *****************************************/
> -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;
> -
> -	result = kstrtou32(buf, 10, &value);
> -	if (result)
> -		return result;
> -
> -	if (value < PPT_TOTAL_MIN || value > PPT_TOTAL_MAX)
> -		return -EINVAL;
> -
> -	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_PPT_PL2_SPPT, value, &result);
> -	if (err) {
> -		pr_warn("Failed to set ppt_pl2_sppt: %d\n", err);
> -		return err;
> -	}
> -
> -	if (result > 1) {
> -		pr_warn("Failed to set ppt_pl2_sppt (result): 0x%x\n", result);
> -		return -EIO;
> -	}
> -
> -	asus->ppt_pl2_sppt = value;
> -	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "ppt_pl2_sppt");
> -
> -	return count;
> -}
> -
> -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, "%u\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,
> -				    struct device_attribute *attr,
> -				    const char *buf, size_t count)
> -{
> -	struct asus_wmi *asus = dev_get_drvdata(dev);
> -	int result, err;
> -	u32 value;
> -
> -	result = kstrtou32(buf, 10, &value);
> -	if (result)
> -		return result;
> -
> -	if (value < PPT_TOTAL_MIN || value > PPT_TOTAL_MAX)
> -		return -EINVAL;
> -
> -	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_PPT_PL1_SPL, value, &result);
> -	if (err) {
> -		pr_warn("Failed to set ppt_pl1_spl: %d\n", err);
> -		return err;
> -	}
> -
> -	if (result > 1) {
> -		pr_warn("Failed to set ppt_pl1_spl (result): 0x%x\n", result);
> -		return -EIO;
> -	}
> -
> -	asus->ppt_pl1_spl = value;
> -	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "ppt_pl1_spl");
> -
> -	return count;
> -}
> -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, "%u\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,
> -				    struct device_attribute *attr,
> -				    const char *buf, size_t count)
> -{
> -	struct asus_wmi *asus = dev_get_drvdata(dev);
> -	int result, err;
> -	u32 value;
> -
> -	result = kstrtou32(buf, 10, &value);
> -	if (result)
> -		return result;
> -
> -	if (value < PPT_TOTAL_MIN || value > PPT_TOTAL_MAX)
> -		return -EINVAL;
> -
> -	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_PPT_FPPT, value, &result);
> -	if (err) {
> -		pr_warn("Failed to set ppt_fppt: %d\n", err);
> -		return err;
> -	}
> -
> -	if (result > 1) {
> -		pr_warn("Failed to set ppt_fppt (result): 0x%x\n", result);
> -		return -EIO;
> -	}
> -
> -	asus->ppt_fppt = value;
> -	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "ppt_fpu_sppt");
> -
> -	return count;
> -}
> -
> -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, "%u\n", asus->ppt_fppt);
> -}
> -static DEVICE_ATTR_RW(ppt_fppt);
> -
> -/* Tunable: PPT APU SPPT *****************************************************/
> -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;
> -
> -	result = kstrtou32(buf, 10, &value);
> -	if (result)
> -		return result;
> -
> -	if (value < PPT_CPU_MIN || value > PPT_CPU_MAX)
> -		return -EINVAL;
> -
> -	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_PPT_APU_SPPT, value, &result);
> -	if (err) {
> -		pr_warn("Failed to set ppt_apu_sppt: %d\n", err);
> -		return err;
> -	}
> -
> -	if (result > 1) {
> -		pr_warn("Failed to set ppt_apu_sppt (result): 0x%x\n", result);
> -		return -EIO;
> -	}
> -
> -	asus->ppt_apu_sppt = value;
> -	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "ppt_apu_sppt");
> -
> -	return count;
> -}
> -
> -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, "%u\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,
> -				    struct device_attribute *attr,
> -				    const char *buf, size_t count)
> -{
> -	struct asus_wmi *asus = dev_get_drvdata(dev);
> -	int result, err;
> -	u32 value;
> -
> -	result = kstrtou32(buf, 10, &value);
> -	if (result)
> -		return result;
> -
> -	if (value < PPT_CPU_MIN || value > PPT_CPU_MAX)
> -		return -EINVAL;
> -
> -	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_PPT_PLAT_SPPT, value, &result);
> -	if (err) {
> -		pr_warn("Failed to set ppt_platform_sppt: %d\n", err);
> -		return err;
> -	}
> -
> -	if (result > 1) {
> -		pr_warn("Failed to set ppt_platform_sppt (result): 0x%x\n", result);
> -		return -EIO;
> -	}
> -
> -	asus->ppt_platform_sppt = value;
> -	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "ppt_platform_sppt");
> -
> -	return count;
> -}
> -
> -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, "%u\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,
> -				    struct device_attribute *attr,
> -				    const char *buf, size_t count)
> -{
> -	struct asus_wmi *asus = dev_get_drvdata(dev);
> -	int result, err;
> -	u32 value;
> -
> -	result = kstrtou32(buf, 10, &value);
> -	if (result)
> -		return result;
> -
> -	if (value < NVIDIA_BOOST_MIN || value > NVIDIA_BOOST_MAX)
> -		return -EINVAL;
> -
> -	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_NV_DYN_BOOST, value, &result);
> -	if (err) {
> -		pr_warn("Failed to set nv_dynamic_boost: %d\n", err);
> -		return err;
> -	}
> -
> -	if (result > 1) {
> -		pr_warn("Failed to set nv_dynamic_boost (result): 0x%x\n", result);
> -		return -EIO;
> -	}
> -
> -	asus->nv_dynamic_boost = value;
> -	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "nv_dynamic_boost");
> -
> -	return count;
> -}
> -
> -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, "%u\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,
> -				    struct device_attribute *attr,
> -				    const char *buf, size_t count)
> -{
> -	struct asus_wmi *asus = dev_get_drvdata(dev);
> -	int result, err;
> -	u32 value;
> -
> -	result = kstrtou32(buf, 10, &value);
> -	if (result)
> -		return result;
> -
> -	if (value < NVIDIA_TEMP_MIN || value > NVIDIA_TEMP_MAX)
> -		return -EINVAL;
> -
> -	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_NV_THERM_TARGET, value, &result);
> -	if (err) {
> -		pr_warn("Failed to set nv_temp_target: %d\n", err);
> -		return err;
> -	}
> -
> -	if (result > 1) {
> -		pr_warn("Failed to set nv_temp_target (result): 0x%x\n", result);
> -		return -EIO;
> -	}
> -
> -	asus->nv_temp_target = value;
> -	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "nv_temp_target");
> -
> -	return count;
> -}
> -
> -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, "%u\n", asus->nv_temp_target);
> -}
> -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)
> @@ -4367,13 +4177,27 @@ static struct attribute *platform_attributes[] = {
>  	&dev_attr_als_enable.attr,
>  	&dev_attr_fan_boost_mode.attr,
>  	&dev_attr_throttle_thermal_policy.attr,
> -	&dev_attr_ppt_pl2_sppt.attr,
>  	&dev_attr_ppt_pl1_spl.attr,
> +	&dev_attr_ppt_pl1_spl_min.attr,
> +	&dev_attr_ppt_pl1_spl_max.attr,
> +	&dev_attr_ppt_pl2_sppt.attr,
> +	&dev_attr_ppt_pl2_sppt_min.attr,
> +	&dev_attr_ppt_pl2_sppt_max.attr,
>  	&dev_attr_ppt_fppt.attr,
> +	&dev_attr_ppt_fppt_min.attr,
> +	&dev_attr_ppt_fppt_max.attr,
>  	&dev_attr_ppt_apu_sppt.attr,
> +	&dev_attr_ppt_apu_sppt_min.attr,
> +	&dev_attr_ppt_apu_sppt_max.attr,
>  	&dev_attr_ppt_platform_sppt.attr,
> +	&dev_attr_ppt_platform_sppt_min.attr,
> +	&dev_attr_ppt_platform_sppt_max.attr,
>  	&dev_attr_nv_dynamic_boost.attr,
> +	&dev_attr_nv_dynamic_boost_min.attr,
> +	&dev_attr_nv_dynamic_boost_max.attr,
>  	&dev_attr_nv_temp_target.attr,
> +	&dev_attr_nv_temp_target_min.attr,
> +	&dev_attr_nv_temp_target_max.attr,
>  	&dev_attr_mcu_powersave.attr,
>  	&dev_attr_boot_sound.attr,
>  	&dev_attr_panel_od.attr,
> @@ -4414,19 +4238,33 @@ 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_ppt_pl2_sppt.attr)
> +	else if (attr == &dev_attr_ppt_pl2_sppt.attr
> +		|| attr == &dev_attr_ppt_pl2_sppt_min.attr
> +		|| attr == &dev_attr_ppt_pl2_sppt_max.attr)
>  		devid = ASUS_WMI_DEVID_PPT_PL2_SPPT;

Please don't align if condition and the code block to same level but add 
the one space to make it obvious it's inside the opening (. The usual 
coding style is to put || at the end of the line and align attr to the 
same column which I think would be beneficial especially on lines which 
begin the same like here.

> -	else if (attr == &dev_attr_ppt_pl1_spl.attr)
> +	else if (attr == &dev_attr_ppt_pl1_spl.attr
> +		|| attr == &dev_attr_ppt_pl1_spl_min.attr
> +		|| attr == &dev_attr_ppt_pl1_spl_max.attr)
>  		devid = ASUS_WMI_DEVID_PPT_PL1_SPL;
> -	else if (attr == &dev_attr_ppt_fppt.attr)
> +	else if (attr == &dev_attr_ppt_fppt.attr
> +		|| attr == &dev_attr_ppt_fppt_min.attr
> +		|| attr == &dev_attr_ppt_fppt_max.attr)
>  		devid = ASUS_WMI_DEVID_PPT_FPPT;
> -	else if (attr == &dev_attr_ppt_apu_sppt.attr)
> +	else if (attr == &dev_attr_ppt_apu_sppt.attr
> +		|| attr == &dev_attr_ppt_apu_sppt_min.attr
> +		|| attr == &dev_attr_ppt_apu_sppt_max.attr)
>  		devid = ASUS_WMI_DEVID_PPT_APU_SPPT;
> -	else if (attr == &dev_attr_ppt_platform_sppt.attr)
> +	else if (attr == &dev_attr_ppt_platform_sppt.attr
> +		|| attr == &dev_attr_ppt_platform_sppt_min.attr
> +		|| attr == &dev_attr_ppt_platform_sppt_max.attr)
>  		devid = ASUS_WMI_DEVID_PPT_PLAT_SPPT;
> -	else if (attr == &dev_attr_nv_dynamic_boost.attr)
> +	else if (attr == &dev_attr_nv_dynamic_boost.attr
> +		|| attr == &dev_attr_nv_dynamic_boost_min.attr
> +		|| attr == &dev_attr_nv_dynamic_boost_max.attr)
>  		devid = ASUS_WMI_DEVID_NV_DYN_BOOST;
> -	else if (attr == &dev_attr_nv_temp_target.attr)
> +	else if (attr == &dev_attr_nv_temp_target.attr
> +		|| attr == &dev_attr_nv_temp_target_min.attr
> +		|| attr == &dev_attr_nv_temp_target_max.attr)
>  		devid = ASUS_WMI_DEVID_NV_THERM_TARGET;
>  	else if (attr == &dev_attr_mcu_powersave.attr)
>  		devid = ASUS_WMI_DEVID_MCU_POWERSAVE;
> @@ -4652,6 +4490,77 @@ static void asus_wmi_debugfs_init(struct asus_wmi *asus)
>  
>  /* Init / exit ****************************************************************/
>  
> +/* Set up the min/max and defaults for ROG tunables */
> +static void init_rog_tunables(struct asus_wmi *asus)
> +{
> +	const char *product;
> +	u32 max_boost = NVIDIA_BOOST_MAX;
> +	u32 cpu_default = PPT_CPU_LIMIT_DEFAULT;
> +	u32 cpu_max = PPT_CPU_LIMIT_MAX;
> +	u32 platform_default = PPT_PLATFORM_DEFAULT;
> +	u32 platform_max = PPT_PLATFORM_MAX;
> +
> +	/*
> +	 * ASUS product_name contains everything required, e.g,
> +	 * "ROG Flow X16 GV601VV_GV601VV_00185149B"
> +	 */
> +	product = dmi_get_system_info(DMI_PRODUCT_NAME);
> +
> +	if (strstr(product, "GA402R")) {
> +		cpu_default = 125;
> +	} else if (strstr(product, "13QY")) {
> +		cpu_max = 250;
> +	} else if (strstr(product, "X13")) {
> +		cpu_max = 75;
> +		cpu_default = 50;
> +	} else if (strstr(product, "RC71")) {
> +		cpu_max = 50;
> +		cpu_default = 30;
> +	} else if (strstr(product, "G814")
> +		|| strstr(product, "G614")
> +		|| strstr(product, "G834")
> +		|| strstr(product, "G634")) {
> +		cpu_max = 175;

Here the unusual placement of || made you even intentionally break the 
alignment to match strstr(). This is a clear proof to me it's not a good 
idea to put || at the beginning of the line because it incentivizes you 
to do this kind of misalignment.

The contination of if condition should start from the column after the 
opening parenthesis (which will make strstr() jumpy, which is why the
usual style with || at the end of the previous line seems clearly better).

> +	} else if (strstr(product, "GA402X")
> +		|| strstr(product, "GA403")
> +		|| strstr(product, "FA507N")
> +		|| strstr(product, "FA507X")
> +		|| strstr(product, "FA707N")
> +		|| strstr(product, "FA707X")) {
> +		cpu_max = 90;

Ditto.

> +	}
> +
> +	if (strstr(product, "GZ301ZE"))
> +		max_boost = 5;
> +	else if (strstr(product, "FX507ZC4"))
> +		max_boost = 15;
> +	else if (strstr(product, "GU605"))
> +		max_boost = 20;
> +
> +	/* ensure defaults for tunables */
> +	asus->rog_tunables.cpu_default = cpu_default;
> +	asus->rog_tunables.cpu_max = cpu_max;
> +
> +	asus->rog_tunables.platform_default = platform_default;
> +	asus->rog_tunables.platform_max = platform_max;
> +
> +	asus->rog_tunables.ppt_pl1_spl = cpu_default;
> +	asus->rog_tunables.ppt_pl2_sppt = cpu_default;
> +	asus->rog_tunables.ppt_apu_sppt = cpu_default;
> +
> +	asus->rog_tunables.ppt_platform_sppt = platform_default;
> +	asus->rog_tunables.ppt_fppt = platform_default;
> +
> +	asus->rog_tunables.nv_boost_default = NVIDIA_BOOST_MAX;
> +	asus->rog_tunables.nv_boost_max = max_boost;
> +	asus->rog_tunables.nv_dynamic_boost = NVIDIA_BOOST_MIN;
> +
> +	asus->rog_tunables.nv_temp_default = NVIDIA_TEMP_MAX;
> +	asus->rog_tunables.nv_temp_max = NVIDIA_TEMP_MAX;
> +	asus->rog_tunables.nv_temp_target = NVIDIA_TEMP_MIN;
> +
> +}
> +
>  static int asus_wmi_add(struct platform_device *pdev)
>  {
>  	struct platform_driver *pdrv = to_platform_driver(pdev->dev.driver);
> @@ -4677,15 +4586,7 @@ 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;
> -
> +	init_rog_tunables(asus);
>  	asus->egpu_enable_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_EGPU);
>  	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);
> 

-- 
 i.

Re: [PATCH 3/9] platform/x86: asus-wmi: add macros and expose min/max sysfs for ppt tunables

[kernel test robot]

Hi Luke,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.10-rc2 next-20240603]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Luke-D-Jones/platform-x86-asus-wmi-add-debug-print-in-more-key-places/20240528-094139
base:   linus/master
patch link:    https://lore.kernel.org/r/20240528013626.14066-4-luke%40ljones.dev
patch subject: [PATCH 3/9] platform/x86: asus-wmi: add macros and expose min/max sysfs for ppt tunables
config: x86_64-randconfig-006-20240604 (https://download.01.org/0day-ci/archive/20240604/202406041330.25g44UcT-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240604/202406041330.25g44UcT-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406041330.25g44UcT-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/platform/x86/asus-wmi.c: In function 'init_rog_tunables':
>> drivers/platform/x86/asus-wmi.c:4508:13: warning: argument 1 null where non-null expected [-Wnonnull]
    4508 |         if (strstr(product, "GA402R")) {
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from include/linux/bitmap.h:13,
                    from include/linux/cpumask.h:13,
                    from include/linux/smp.h:13,
                    from include/linux/lockdep.h:14,
                    from include/linux/spinlock.h:63,
                    from include/linux/mmzone.h:8,
                    from include/linux/gfp.h:7,
                    from include/linux/slab.h:16,
                    from include/linux/resource_ext.h:11,
                    from include/linux/acpi.h:13,
                    from drivers/platform/x86/asus-wmi.c:16:
   include/linux/string.h:190:15: note: in a call to function 'strstr' declared 'nonnull'
     190 | extern char * strstr(const char *, const char *);
         |               ^~~~~~
   drivers/platform/x86/asus-wmi.c:4510:20: warning: argument 1 null where non-null expected [-Wnonnull]
    4510 |         } else if (strstr(product, "13QY")) {
         |                    ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:190:15: note: in a call to function 'strstr' declared 'nonnull'
     190 | extern char * strstr(const char *, const char *);
         |               ^~~~~~
   drivers/platform/x86/asus-wmi.c:4512:20: warning: argument 1 null where non-null expected [-Wnonnull]
    4512 |         } else if (strstr(product, "X13")) {
         |                    ^~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:190:15: note: in a call to function 'strstr' declared 'nonnull'
     190 | extern char * strstr(const char *, const char *);
         |               ^~~~~~
   drivers/platform/x86/asus-wmi.c:4515:20: warning: argument 1 null where non-null expected [-Wnonnull]
    4515 |         } else if (strstr(product, "RC71")) {
         |                    ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:190:15: note: in a call to function 'strstr' declared 'nonnull'
     190 | extern char * strstr(const char *, const char *);
         |               ^~~~~~
   drivers/platform/x86/asus-wmi.c:4518:20: warning: argument 1 null where non-null expected [-Wnonnull]
    4518 |         } else if (strstr(product, "G814")
         |                    ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:190:15: note: in a call to function 'strstr' declared 'nonnull'
     190 | extern char * strstr(const char *, const char *);
         |               ^~~~~~
   drivers/platform/x86/asus-wmi.c:4519:20: warning: argument 1 null where non-null expected [-Wnonnull]
    4519 |                 || strstr(product, "G614")
         |                    ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:190:15: note: in a call to function 'strstr' declared 'nonnull'
     190 | extern char * strstr(const char *, const char *);
         |               ^~~~~~
   drivers/platform/x86/asus-wmi.c:4520:20: warning: argument 1 null where non-null expected [-Wnonnull]
    4520 |                 || strstr(product, "G834")
         |                    ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:190:15: note: in a call to function 'strstr' declared 'nonnull'
     190 | extern char * strstr(const char *, const char *);
         |               ^~~~~~
   drivers/platform/x86/asus-wmi.c:4521:20: warning: argument 1 null where non-null expected [-Wnonnull]
    4521 |                 || strstr(product, "G634")) {
         |                    ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:190:15: note: in a call to function 'strstr' declared 'nonnull'
     190 | extern char * strstr(const char *, const char *);
         |               ^~~~~~
   drivers/platform/x86/asus-wmi.c:4523:20: warning: argument 1 null where non-null expected [-Wnonnull]
    4523 |         } else if (strstr(product, "GA402X")
         |                    ^~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:190:15: note: in a call to function 'strstr' declared 'nonnull'
     190 | extern char * strstr(const char *, const char *);
         |               ^~~~~~
   drivers/platform/x86/asus-wmi.c:4524:20: warning: argument 1 null where non-null expected [-Wnonnull]
    4524 |                 || strstr(product, "GA403")
         |                    ^~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:190:15: note: in a call to function 'strstr' declared 'nonnull'
     190 | extern char * strstr(const char *, const char *);
         |               ^~~~~~
   drivers/platform/x86/asus-wmi.c:4525:20: warning: argument 1 null where non-null expected [-Wnonnull]
    4525 |                 || strstr(product, "FA507N")
         |                    ^~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:190:15: note: in a call to function 'strstr' declared 'nonnull'
     190 | extern char * strstr(const char *, const char *);
         |               ^~~~~~
   drivers/platform/x86/asus-wmi.c:4526:20: warning: argument 1 null where non-null expected [-Wnonnull]
    4526 |                 || strstr(product, "FA507X")
         |                    ^~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:190:15: note: in a call to function 'strstr' declared 'nonnull'
     190 | extern char * strstr(const char *, const char *);
         |               ^~~~~~
   drivers/platform/x86/asus-wmi.c:4527:20: warning: argument 1 null where non-null expected [-Wnonnull]
    4527 |                 || strstr(product, "FA707N")
         |                    ^~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:190:15: note: in a call to function 'strstr' declared 'nonnull'
     190 | extern char * strstr(const char *, const char *);
         |               ^~~~~~
   drivers/platform/x86/asus-wmi.c:4528:20: warning: argument 1 null where non-null expected [-Wnonnull]
    4528 |                 || strstr(product, "FA707X")) {
         |                    ^~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:190:15: note: in a call to function 'strstr' declared 'nonnull'
     190 | extern char * strstr(const char *, const char *);
         |               ^~~~~~
   drivers/platform/x86/asus-wmi.c:4532:13: warning: argument 1 null where non-null expected [-Wnonnull]
    4532 |         if (strstr(product, "GZ301ZE"))
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/string.h:190:15: note: in a call to function 'strstr' declared 'nonnull'
     190 | extern char * strstr(const char *, const char *);
         |               ^~~~~~


vim +4508 drivers/platform/x86/asus-wmi.c

  4491	
  4492	/* Set up the min/max and defaults for ROG tunables */
  4493	static void init_rog_tunables(struct asus_wmi *asus)
  4494	{
  4495		const char *product;
  4496		u32 max_boost = NVIDIA_BOOST_MAX;
  4497		u32 cpu_default = PPT_CPU_LIMIT_DEFAULT;
  4498		u32 cpu_max = PPT_CPU_LIMIT_MAX;
  4499		u32 platform_default = PPT_PLATFORM_DEFAULT;
  4500		u32 platform_max = PPT_PLATFORM_MAX;
  4501	
  4502		/*
  4503		 * ASUS product_name contains everything required, e.g,
  4504		 * "ROG Flow X16 GV601VV_GV601VV_00185149B"
  4505		 */
  4506		product = dmi_get_system_info(DMI_PRODUCT_NAME);
  4507	
> 4508		if (strstr(product, "GA402R")) {
  4509			cpu_default = 125;
  4510		} else if (strstr(product, "13QY")) {
  4511			cpu_max = 250;
  4512		} else if (strstr(product, "X13")) {
  4513			cpu_max = 75;
  4514			cpu_default = 50;
  4515		} else if (strstr(product, "RC71")) {
  4516			cpu_max = 50;
  4517			cpu_default = 30;
  4518		} else if (strstr(product, "G814")
  4519			|| strstr(product, "G614")
  4520			|| strstr(product, "G834")
  4521			|| strstr(product, "G634")) {
  4522			cpu_max = 175;
  4523		} else if (strstr(product, "GA402X")
  4524			|| strstr(product, "GA403")
  4525			|| strstr(product, "FA507N")
  4526			|| strstr(product, "FA507X")
  4527			|| strstr(product, "FA707N")
  4528			|| strstr(product, "FA707X")) {
  4529			cpu_max = 90;
  4530		}
  4531	
  4532		if (strstr(product, "GZ301ZE"))
  4533			max_boost = 5;
  4534		else if (strstr(product, "FX507ZC4"))
  4535			max_boost = 15;
  4536		else if (strstr(product, "GU605"))
  4537			max_boost = 20;
  4538	
  4539		/* ensure defaults for tunables */
  4540		asus->rog_tunables.cpu_default = cpu_default;
  4541		asus->rog_tunables.cpu_max = cpu_max;
  4542	
  4543		asus->rog_tunables.platform_default = platform_default;
  4544		asus->rog_tunables.platform_max = platform_max;
  4545	
  4546		asus->rog_tunables.ppt_pl1_spl = cpu_default;
  4547		asus->rog_tunables.ppt_pl2_sppt = cpu_default;
  4548		asus->rog_tunables.ppt_apu_sppt = cpu_default;
  4549	
  4550		asus->rog_tunables.ppt_platform_sppt = platform_default;
  4551		asus->rog_tunables.ppt_fppt = platform_default;
  4552	
  4553		asus->rog_tunables.nv_boost_default = NVIDIA_BOOST_MAX;
  4554		asus->rog_tunables.nv_boost_max = max_boost;
  4555		asus->rog_tunables.nv_dynamic_boost = NVIDIA_BOOST_MIN;
  4556	
  4557		asus->rog_tunables.nv_temp_default = NVIDIA_TEMP_MAX;
  4558		asus->rog_tunables.nv_temp_max = NVIDIA_TEMP_MAX;
  4559		asus->rog_tunables.nv_temp_target = NVIDIA_TEMP_MIN;
  4560	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 4/9] platform/x86: asus-wmi: reduce code duplication with macros

[Luke D. Jones]

Over time many default patterns have emerged while adding functionality.
This patch consolidates those patterns in to a few macros to remove a lot
of copy/paste, and make it easier to add more of the same style of
features in the future.

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

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index d016acb23789..5c03e28ff252 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -676,7 +676,7 @@ static void asus_wmi_input_exit(struct asus_wmi *asus)
 static ssize_t rog_tunable_store(struct asus_wmi *asus,
 				struct attribute *attr,
 				const char *buf, size_t count,
-				u32 min, u32 max, u32 defaultv,
+				u32 min, u32 max, int defaultv,
 				u32 *store_value, u32 wmi_dev)
 {
 	int result, err, value;
@@ -685,7 +685,7 @@ static ssize_t rog_tunable_store(struct asus_wmi *asus,
 	if (result)
 		return result;
 
-	if (value == -1 )
+	if (value == -1 && defaultv != -1)
 		value = defaultv;
 	if (value < min || value > max)
 		return -EINVAL;
@@ -708,6 +708,36 @@ static ssize_t rog_tunable_store(struct asus_wmi *asus,
 	return count;
 }
 
+#define WMI_SIMPLE_STORE(_fname, _min, _max, _wmi) \
+static ssize_t _fname##_store(struct device *dev, \
+	struct device_attribute *attr, const char *buf, size_t count) \
+{ \
+	struct asus_wmi *asus = dev_get_drvdata(dev); \
+	return rog_tunable_store(asus, &attr->attr, buf, count, \
+				_min, _max, -1, NULL, _wmi); \
+}
+
+#define WMI_SIMPLE_SHOW(_fname, _fmt, _wmi) \
+static ssize_t _fname##_show(struct device *dev, \
+	struct device_attribute *attr, char *buf) \
+{ \
+	struct asus_wmi *asus = dev_get_drvdata(dev); \
+	u32 result; \
+	asus_wmi_get_devstate(asus, _wmi, &result); \
+	if (result < 0) \
+		return result; \
+	return sysfs_emit(buf, _fmt, result & ~ASUS_WMI_DSTS_PRESENCE_BIT); \
+}
+
+#define WMI_ATTR_SIMPLE_RW(_fname, _minv, _maxv, _wmi) \
+WMI_SIMPLE_STORE(_fname, _minv, _maxv, _wmi); \
+WMI_SIMPLE_SHOW(_fname, "%d\n", _wmi); \
+static DEVICE_ATTR_RW(_fname)
+
+#define WMI_ATTR_SIMPLE_RO(_fname, _wmi) \
+WMI_SIMPLE_SHOW(_fname, "%d\n", _wmi); \
+static DEVICE_ATTR_RO(_fname)
+
 #define ROG_TUNABLE_STORE(_fname, _min, _max, _default, _wmi) \
 static ssize_t _fname##_store(struct device *dev, \
 	struct device_attribute *attr, const char *buf, size_t count) \
@@ -761,6 +791,12 @@ ROG_ATTR_RW(nv_dynamic_boost,
 	NVIDIA_BOOST_MIN, nv_boost_max, nv_boost_default, ASUS_WMI_DEVID_NV_DYN_BOOST);
 ROG_ATTR_RW(nv_temp_target,
 	NVIDIA_TEMP_MIN, nv_temp_max, nv_temp_default, ASUS_WMI_DEVID_NV_THERM_TARGET);
+/* Ally MCU Powersave */
+WMI_ATTR_SIMPLE_RW(mcu_powersave, 0, 1, ASUS_WMI_DEVID_MCU_POWERSAVE);
+WMI_ATTR_SIMPLE_RO(egpu_connected, ASUS_WMI_DEVID_EGPU_CONNECTED);
+WMI_ATTR_SIMPLE_RW(panel_od, 0, 1, ASUS_WMI_DEVID_PANEL_OD);
+WMI_ATTR_SIMPLE_RW(boot_sound, 0, 1, ASUS_WMI_DEVID_BOOT_SOUND);
+WMI_ATTR_SIMPLE_RO(charge_mode, ASUS_WMI_DEVID_CHARGE_MODE);
 
 /* Tablet mode ****************************************************************/
 
@@ -776,22 +812,6 @@ static void asus_wmi_tablet_mode_get_state(struct asus_wmi *asus)
 		asus_wmi_tablet_sw_report(asus, result);
 }
 
-/* Charging mode, 1=Barrel, 2=USB ******************************************/
-static ssize_t charge_mode_show(struct device *dev,
-				   struct device_attribute *attr, char *buf)
-{
-	struct asus_wmi *asus = dev_get_drvdata(dev);
-	int result, value;
-
-	result = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_CHARGE_MODE, &value);
-	if (result < 0)
-		return result;
-
-	return sysfs_emit(buf, "%d\n", value & 0xff);
-}
-
-static DEVICE_ATTR_RO(charge_mode);
-
 /* dGPU ********************************************************************/
 static ssize_t dgpu_disable_show(struct device *dev,
 				   struct device_attribute *attr, char *buf)
@@ -925,22 +945,6 @@ static ssize_t egpu_enable_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(egpu_enable);
 
-/* Is eGPU connected? *********************************************************/
-static ssize_t egpu_connected_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_EGPU_CONNECTED);
-	if (result < 0)
-		return result;
-
-	return sysfs_emit(buf, "%d\n", result);
-}
-
-static DEVICE_ATTR_RO(egpu_connected);
-
 /* gpu mux switch *************************************************************/
 static ssize_t gpu_mux_mode_show(struct device *dev,
 				 struct device_attribute *attr, char *buf)
@@ -1128,53 +1132,6 @@ static const struct attribute_group *kbd_rgb_mode_groups[] = {
 	NULL,
 };
 
-/* 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 */
@@ -2002,102 +1959,6 @@ static int asus_wmi_rfkill_init(struct asus_wmi *asus)
 	return result;
 }
 
-/* Panel Overdrive ************************************************************/
-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;
-
-	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)
-{
-	int result, err;
-	u32 overdrive;
-
-	struct asus_wmi *asus = dev_get_drvdata(dev);
-
-	result = kstrtou32(buf, 10, &overdrive);
-	if (result)
-		return result;
-
-	if (overdrive > 1)
-		return -EINVAL;
-
-	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);
-
-/* 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)
-- 
2.45.1

Re: [PATCH 4/9] platform/x86: asus-wmi: reduce code duplication with macros

[Ilpo Järvinen]

On Tue, 28 May 2024, Luke D. Jones wrote:

> Over time many default patterns have emerged while adding functionality.
> This patch consolidates those patterns in to a few macros to remove a lot
> of copy/paste, and make it easier to add more of the same style of
> features in the future.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>  drivers/platform/x86/asus-wmi.c | 215 ++++++--------------------------
>  1 file changed, 38 insertions(+), 177 deletions(-)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index d016acb23789..5c03e28ff252 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -676,7 +676,7 @@ static void asus_wmi_input_exit(struct asus_wmi *asus)
>  static ssize_t rog_tunable_store(struct asus_wmi *asus,
>  				struct attribute *attr,
>  				const char *buf, size_t count,
> -				u32 min, u32 max, u32 defaultv,
> +				u32 min, u32 max, int defaultv,

You just introduced this code in the previous patch and you're already 
changing the type, please don't do that but go to the right type right 
from the start.

Now that I of this more. This "reset to default" is a new feature and IMO 
it should be put into own patch and not mixed with the other refactoring.

-- 
 i.

>  				u32 *store_value, u32 wmi_dev)
>  {
>  	int result, err, value;
> @@ -685,7 +685,7 @@ static ssize_t rog_tunable_store(struct asus_wmi *asus,
>  	if (result)
>  		return result;
>  
> -	if (value == -1 )
> +	if (value == -1 && defaultv != -1)
>  		value = defaultv;
>  	if (value < min || value > max)
>  		return -EINVAL;
> @@ -708,6 +708,36 @@ static ssize_t rog_tunable_store(struct asus_wmi *asus,
>  	return count;
>  }
>  
> +#define WMI_SIMPLE_STORE(_fname, _min, _max, _wmi) \
> +static ssize_t _fname##_store(struct device *dev, \
> +	struct device_attribute *attr, const char *buf, size_t count) \
> +{ \
> +	struct asus_wmi *asus = dev_get_drvdata(dev); \
> +	return rog_tunable_store(asus, &attr->attr, buf, count, \
> +				_min, _max, -1, NULL, _wmi); \
> +}
> +
> +#define WMI_SIMPLE_SHOW(_fname, _fmt, _wmi) \
> +static ssize_t _fname##_show(struct device *dev, \
> +	struct device_attribute *attr, char *buf) \
> +{ \
> +	struct asus_wmi *asus = dev_get_drvdata(dev); \
> +	u32 result; \
> +	asus_wmi_get_devstate(asus, _wmi, &result); \
> +	if (result < 0) \
> +		return result; \
> +	return sysfs_emit(buf, _fmt, result & ~ASUS_WMI_DSTS_PRESENCE_BIT); \
> +}
> +
> +#define WMI_ATTR_SIMPLE_RW(_fname, _minv, _maxv, _wmi) \
> +WMI_SIMPLE_STORE(_fname, _minv, _maxv, _wmi); \
> +WMI_SIMPLE_SHOW(_fname, "%d\n", _wmi); \
> +static DEVICE_ATTR_RW(_fname)
> +
> +#define WMI_ATTR_SIMPLE_RO(_fname, _wmi) \
> +WMI_SIMPLE_SHOW(_fname, "%d\n", _wmi); \
> +static DEVICE_ATTR_RO(_fname)
> +
>  #define ROG_TUNABLE_STORE(_fname, _min, _max, _default, _wmi) \
>  static ssize_t _fname##_store(struct device *dev, \
>  	struct device_attribute *attr, const char *buf, size_t count) \
> @@ -761,6 +791,12 @@ ROG_ATTR_RW(nv_dynamic_boost,
>  	NVIDIA_BOOST_MIN, nv_boost_max, nv_boost_default, ASUS_WMI_DEVID_NV_DYN_BOOST);
>  ROG_ATTR_RW(nv_temp_target,
>  	NVIDIA_TEMP_MIN, nv_temp_max, nv_temp_default, ASUS_WMI_DEVID_NV_THERM_TARGET);
> +/* Ally MCU Powersave */
> +WMI_ATTR_SIMPLE_RW(mcu_powersave, 0, 1, ASUS_WMI_DEVID_MCU_POWERSAVE);
> +WMI_ATTR_SIMPLE_RO(egpu_connected, ASUS_WMI_DEVID_EGPU_CONNECTED);
> +WMI_ATTR_SIMPLE_RW(panel_od, 0, 1, ASUS_WMI_DEVID_PANEL_OD);
> +WMI_ATTR_SIMPLE_RW(boot_sound, 0, 1, ASUS_WMI_DEVID_BOOT_SOUND);
> +WMI_ATTR_SIMPLE_RO(charge_mode, ASUS_WMI_DEVID_CHARGE_MODE);
>  
>  /* Tablet mode ****************************************************************/
>  
> @@ -776,22 +812,6 @@ static void asus_wmi_tablet_mode_get_state(struct asus_wmi *asus)
>  		asus_wmi_tablet_sw_report(asus, result);
>  }
>  
> -/* Charging mode, 1=Barrel, 2=USB ******************************************/
> -static ssize_t charge_mode_show(struct device *dev,
> -				   struct device_attribute *attr, char *buf)
> -{
> -	struct asus_wmi *asus = dev_get_drvdata(dev);
> -	int result, value;
> -
> -	result = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_CHARGE_MODE, &value);
> -	if (result < 0)
> -		return result;
> -
> -	return sysfs_emit(buf, "%d\n", value & 0xff);
> -}
> -
> -static DEVICE_ATTR_RO(charge_mode);
> -
>  /* dGPU ********************************************************************/
>  static ssize_t dgpu_disable_show(struct device *dev,
>  				   struct device_attribute *attr, char *buf)
> @@ -925,22 +945,6 @@ static ssize_t egpu_enable_store(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(egpu_enable);
>  
> -/* Is eGPU connected? *********************************************************/
> -static ssize_t egpu_connected_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_EGPU_CONNECTED);
> -	if (result < 0)
> -		return result;
> -
> -	return sysfs_emit(buf, "%d\n", result);
> -}
> -
> -static DEVICE_ATTR_RO(egpu_connected);
> -
>  /* gpu mux switch *************************************************************/
>  static ssize_t gpu_mux_mode_show(struct device *dev,
>  				 struct device_attribute *attr, char *buf)
> @@ -1128,53 +1132,6 @@ static const struct attribute_group *kbd_rgb_mode_groups[] = {
>  	NULL,
>  };
>  
> -/* 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 */
> @@ -2002,102 +1959,6 @@ static int asus_wmi_rfkill_init(struct asus_wmi *asus)
>  	return result;
>  }
>  
> -/* Panel Overdrive ************************************************************/
> -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;
> -
> -	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)
> -{
> -	int result, err;
> -	u32 overdrive;
> -
> -	struct asus_wmi *asus = dev_get_drvdata(dev);
> -
> -	result = kstrtou32(buf, 10, &overdrive);
> -	if (result)
> -		return result;
> -
> -	if (overdrive > 1)
> -		return -EINVAL;
> -
> -	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);
> -
> -/* 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)
> 

Re: [PATCH 4/9] platform/x86: asus-wmi: reduce code duplication with macros

[Ilpo Järvinen]

On Tue, 28 May 2024, Luke D. Jones wrote:

> Over time many default patterns have emerged while adding functionality.
> This patch consolidates those patterns in to a few macros to remove a lot
> of copy/paste, and make it easier to add more of the same style of
> features in the future.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---

> +#define WMI_SIMPLE_SHOW(_fname, _fmt, _wmi) \
> +static ssize_t _fname##_show(struct device *dev, \
> +	struct device_attribute *attr, char *buf) \
> +{ \
> +	struct asus_wmi *asus = dev_get_drvdata(dev); \
> +	u32 result; \
> +	asus_wmi_get_devstate(asus, _wmi, &result); \
> +	if (result < 0) \
> +		return result; \
> +	return sysfs_emit(buf, _fmt, result & ~ASUS_WMI_DSTS_PRESENCE_BIT); \
> +}

One more thing. To improve readability further, add also normal newlines 
into macros like this (obviously with the continuation backslashes far 
right as already mentioned).

-- 
 i.

[PATCH 5/9] platform/x86: asus-wmi: use WMI_SIMPLE_SHOW in more places

[Luke D. Jones]

Further reduce code duplication by using the new macros in a few more
places.

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

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 5c03e28ff252..260548aa6a42 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -813,19 +813,6 @@ static void asus_wmi_tablet_mode_get_state(struct asus_wmi *asus)
 }
 
 /* dGPU ********************************************************************/
-static ssize_t dgpu_disable_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_DGPU);
-	if (result < 0)
-		return result;
-
-	return sysfs_emit(buf, "%d\n", result);
-}
-
 /*
  * A user may be required to store the value twice, typcial store first, then
  * rescan PCI bus to activate power, then store a second time to save correctly.
@@ -875,22 +862,11 @@ static ssize_t dgpu_disable_store(struct device *dev,
 
 	return count;
 }
+
+WMI_SIMPLE_SHOW(dgpu_disable, "%d\n", ASUS_WMI_DEVID_DGPU);
 static DEVICE_ATTR_RW(dgpu_disable);
 
 /* eGPU ********************************************************************/
-static ssize_t egpu_enable_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_EGPU);
-	if (result < 0)
-		return result;
-
-	return sysfs_emit(buf, "%d\n", result);
-}
-
 /* The ACPI call to enable the eGPU also disables the internal dGPU */
 static ssize_t egpu_enable_store(struct device *dev,
 				    struct device_attribute *attr,
@@ -943,6 +919,8 @@ static ssize_t egpu_enable_store(struct device *dev,
 
 	return count;
 }
+
+WMI_SIMPLE_SHOW(egpu_enable, "%d\n", ASUS_WMI_DEVID_EGPU);
 static DEVICE_ATTR_RW(egpu_enable);
 
 /* gpu mux switch *************************************************************/
-- 
2.45.1

Re: [PATCH 5/9] platform/x86: asus-wmi: use WMI_SIMPLE_SHOW in more places

[Ilpo Järvinen]

On Tue, 28 May 2024, Luke D. Jones wrote:

> Further reduce code duplication by using the new macros in a few more
> places.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>  drivers/platform/x86/asus-wmi.c | 30 ++++--------------------------
>  1 file changed, 4 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 5c03e28ff252..260548aa6a42 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -813,19 +813,6 @@ static void asus_wmi_tablet_mode_get_state(struct asus_wmi *asus)
>  }
>  
>  /* dGPU ********************************************************************/
> -static ssize_t dgpu_disable_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_DGPU);
> -	if (result < 0)
> -		return result;
> -
> -	return sysfs_emit(buf, "%d\n", result);
> -}
> -
>  /*
>   * A user may be required to store the value twice, typcial store first, then
>   * rescan PCI bus to activate power, then store a second time to save correctly.
> @@ -875,22 +862,11 @@ static ssize_t dgpu_disable_store(struct device *dev,
>  
>  	return count;
>  }
> +
> +WMI_SIMPLE_SHOW(dgpu_disable, "%d\n", ASUS_WMI_DEVID_DGPU);
>  static DEVICE_ATTR_RW(dgpu_disable);
>  
>  /* eGPU ********************************************************************/
> -static ssize_t egpu_enable_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_EGPU);
> -	if (result < 0)
> -		return result;
> -
> -	return sysfs_emit(buf, "%d\n", result);
> -}
> -
>  /* The ACPI call to enable the eGPU also disables the internal dGPU */
>  static ssize_t egpu_enable_store(struct device *dev,
>  				    struct device_attribute *attr,
> @@ -943,6 +919,8 @@ static ssize_t egpu_enable_store(struct device *dev,
>  
>  	return count;
>  }
> +
> +WMI_SIMPLE_SHOW(egpu_enable, "%d\n", ASUS_WMI_DEVID_EGPU);
>  static DEVICE_ATTR_RW(egpu_enable);

It seems odd you do this subset in own patch. I think it would be more 
logical if you reorder patches 4 and 5.

-- 
 i.

[PATCH 6/9] platform/x86: asus-wmi: add panel-fhd functionality

[Luke D. Jones]

Exposes the FHD panel toggle avavilable on new ASUS Duo laptops.

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

diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index 984a04f32fd0..3b4eeea75b7b 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -217,3 +217,12 @@ Description:
 		Set if the MCU can go in to low-power mode on system sleep
 			* 0 - False,
 			* 1 - True
+
+What:		/sys/devices/platform/<platform>/panel_fhd
+Date:		Apr 2024
+KernelVersion:	6.11
+Contact:	"Luke Jones" <luke@ljones.dev>
+Description:
+		Set panel to UHD or FHD mode
+			* 0 - UHD,
+			* 1 - FHD
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 260548aa6a42..4b045f1828f1 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -798,6 +798,23 @@ WMI_ATTR_SIMPLE_RW(panel_od, 0, 1, ASUS_WMI_DEVID_PANEL_OD);
 WMI_ATTR_SIMPLE_RW(boot_sound, 0, 1, ASUS_WMI_DEVID_BOOT_SOUND);
 WMI_ATTR_SIMPLE_RO(charge_mode, ASUS_WMI_DEVID_CHARGE_MODE);
 
+static ssize_t panel_fhd_store(struct device *dev,
+	struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+	int err;
+
+	err = rog_tunable_store(asus, &attr->attr, buf, count,
+				0, 1, -1, NULL, ASUS_WMI_DEVID_PANEL_FHD);
+	if (err < 0)
+		return err;
+
+	pr_info("Panel UHD/FHD display mode changed, reboot required\n");
+	return count;
+}
+WMI_SIMPLE_SHOW(panel_fhd, "%d\n", ASUS_WMI_DEVID_PANEL_FHD);
+static DEVICE_ATTR_RW(panel_fhd);
+
 /* Tablet mode ****************************************************************/
 
 static void asus_wmi_tablet_mode_get_state(struct asus_wmi *asus)
@@ -4040,6 +4057,7 @@ static struct attribute *platform_attributes[] = {
 	&dev_attr_mcu_powersave.attr,
 	&dev_attr_boot_sound.attr,
 	&dev_attr_panel_od.attr,
+	&dev_attr_panel_fhd.attr,
 	&dev_attr_mini_led_mode.attr,
 	&dev_attr_available_mini_led_mode.attr,
 	NULL
@@ -4111,6 +4129,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
 		devid = ASUS_WMI_DEVID_BOOT_SOUND;
 	else if (attr == &dev_attr_panel_od.attr)
 		devid = ASUS_WMI_DEVID_PANEL_OD;
+	else if (attr == &dev_attr_panel_fhd.attr)
+		devid = ASUS_WMI_DEVID_PANEL_FHD;
 	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)
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 79a50102440d..6c51d41ffc20 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -72,6 +72,7 @@
 #define ASUS_WMI_DEVID_LID_FLIP_ROG	0x00060077
 #define ASUS_WMI_DEVID_MINI_LED_MODE	0x0005001E
 #define ASUS_WMI_DEVID_MINI_LED_MODE2	0x0005002E
+#define ASUS_WMI_DEVID_PANEL_FHD	0x0005001C
 
 /* Storage */
 #define ASUS_WMI_DEVID_CARDREADER	0x00080013
-- 
2.45.1

Re: [PATCH 6/9] platform/x86: asus-wmi: add panel-fhd functionality

[Ilpo Järvinen]

On Tue, 28 May 2024, Luke D. Jones wrote:

> Exposes the FHD panel toggle avavilable on new ASUS Duo laptops.

available

> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>  .../ABI/testing/sysfs-platform-asus-wmi       |  9 +++++++++
>  drivers/platform/x86/asus-wmi.c               | 20 +++++++++++++++++++
>  include/linux/platform_data/x86/asus-wmi.h    |  1 +
>  3 files changed, 30 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> index 984a04f32fd0..3b4eeea75b7b 100644
> --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> @@ -217,3 +217,12 @@ Description:
>  		Set if the MCU can go in to low-power mode on system sleep
>  			* 0 - False,
>  			* 1 - True
> +
> +What:		/sys/devices/platform/<platform>/panel_fhd
> +Date:		Apr 2024
> +KernelVersion:	6.11
> +Contact:	"Luke Jones" <luke@ljones.dev>
> +Description:
> +		Set panel to UHD or FHD mode
> +			* 0 - UHD,
> +			* 1 - FHD

I'd prefer this to be more forward-looking, the filename is based on fhd, 
which is one of the options and the values seem "wrong way" around too 
(FHD < UHD but here you have FHD=1 after UHD=0).

> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 260548aa6a42..4b045f1828f1 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -798,6 +798,23 @@ WMI_ATTR_SIMPLE_RW(panel_od, 0, 1, ASUS_WMI_DEVID_PANEL_OD);
>  WMI_ATTR_SIMPLE_RW(boot_sound, 0, 1, ASUS_WMI_DEVID_BOOT_SOUND);
>  WMI_ATTR_SIMPLE_RO(charge_mode, ASUS_WMI_DEVID_CHARGE_MODE);
>  
> +static ssize_t panel_fhd_store(struct device *dev,
> +	struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +	int err;
> +
> +	err = rog_tunable_store(asus, &attr->attr, buf, count,
> +				0, 1, -1, NULL, ASUS_WMI_DEVID_PANEL_FHD);
> +	if (err < 0)
> +		return err;
> +
> +	pr_info("Panel UHD/FHD display mode changed, reboot required\n");
> +	return count;
> +}
> +WMI_SIMPLE_SHOW(panel_fhd, "%d\n", ASUS_WMI_DEVID_PANEL_FHD);
> +static DEVICE_ATTR_RW(panel_fhd);
> +
>  /* Tablet mode ****************************************************************/
>  
>  static void asus_wmi_tablet_mode_get_state(struct asus_wmi *asus)
> @@ -4040,6 +4057,7 @@ static struct attribute *platform_attributes[] = {
>  	&dev_attr_mcu_powersave.attr,
>  	&dev_attr_boot_sound.attr,
>  	&dev_attr_panel_od.attr,
> +	&dev_attr_panel_fhd.attr,
>  	&dev_attr_mini_led_mode.attr,
>  	&dev_attr_available_mini_led_mode.attr,
>  	NULL
> @@ -4111,6 +4129,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
>  		devid = ASUS_WMI_DEVID_BOOT_SOUND;
>  	else if (attr == &dev_attr_panel_od.attr)
>  		devid = ASUS_WMI_DEVID_PANEL_OD;
> +	else if (attr == &dev_attr_panel_fhd.attr)
> +		devid = ASUS_WMI_DEVID_PANEL_FHD;
>  	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)
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index 79a50102440d..6c51d41ffc20 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -72,6 +72,7 @@
>  #define ASUS_WMI_DEVID_LID_FLIP_ROG	0x00060077
>  #define ASUS_WMI_DEVID_MINI_LED_MODE	0x0005001E
>  #define ASUS_WMI_DEVID_MINI_LED_MODE2	0x0005002E
> +#define ASUS_WMI_DEVID_PANEL_FHD	0x0005001C
>  
>  /* Storage */
>  #define ASUS_WMI_DEVID_CARDREADER	0x00080013
> 

-- 
 i.

[PATCH 7/9] platform/x86: asus-wmi: add enable/disable CPU cores

[Luke D. Jones]

Exposes the WMI functions for enable/disable of performance and
efficiency cores on some laptop models (largely Intel only).

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

diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index 3b4eeea75b7b..ac881e72e374 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -226,3 +226,22 @@ Description:
 		Set panel to UHD or FHD mode
 			* 0 - UHD,
 			* 1 - FHD
+
+What:		/sys/devices/platform/<platform>/cores_enabled
+Date:		Jun 2024
+KernelVersion:	6.11
+Contact:	"Luke Jones" <luke@ljones.dev>
+Description:
+		Enable/disable efficiency and performance cores. The format is
+		0x[E][P] where [E] is the efficiency core count, and [P] is
+		the perfromance core count. If the core count is a single digit
+		it is preceded by a 0 such as 0x0406; E=4, P=6, 0x1006; E=10, P=6
+
+What:		/sys/devices/platform/<platform>/cores_max
+Date:		Jun 2024
+KernelVersion:	6.11
+Contact:	"Luke Jones" <luke@ljones.dev>
+Description:
+		Show the maximum performance and efficiency core countin format
+		0x[E][P] where [E] is the efficiency core count, and [P] is
+		the perfromance core count.
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 4b045f1828f1..f62a36dfcd4b 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -815,6 +815,46 @@ static ssize_t panel_fhd_store(struct device *dev,
 WMI_SIMPLE_SHOW(panel_fhd, "%d\n", ASUS_WMI_DEVID_PANEL_FHD);
 static DEVICE_ATTR_RW(panel_fhd);
 
+/* Efficiency and Performance core control **********************************/
+static ssize_t cores_enabled_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 cores, max;
+
+	result = kstrtou32(buf, 16, &cores);
+	if (result)
+		return result;
+
+	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_CORES_MAX, &max);
+	if (err < 0)
+		return err;
+
+	if (cores > max) {
+		pr_warn("Core count 0x%x exceeds max: 0x%x\n", cores, max);
+		return -EIO;
+	}
+
+	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_CORES_SET, cores, &result);
+	if (err) {
+		pr_warn("Failed to set cores_enabled: %d\n", err);
+		return err;
+	}
+
+	pr_info("Enabled core count changed, reboot required\n");
+	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "cores_enabled");
+
+	return count;
+}
+
+WMI_SIMPLE_SHOW(cores_enabled, "0x%x\n", ASUS_WMI_DEVID_CORES_SET);
+static DEVICE_ATTR_RW(cores_enabled);
+
+WMI_SIMPLE_SHOW(cores_max, "0x%x\n", ASUS_WMI_DEVID_CORES_MAX);
+static DEVICE_ATTR_RO(cores_max);
+
 /* Tablet mode ****************************************************************/
 
 static void asus_wmi_tablet_mode_get_state(struct asus_wmi *asus)
@@ -4058,6 +4098,8 @@ static struct attribute *platform_attributes[] = {
 	&dev_attr_boot_sound.attr,
 	&dev_attr_panel_od.attr,
 	&dev_attr_panel_fhd.attr,
+	&dev_attr_cores_enabled.attr,
+	&dev_attr_cores_max.attr,
 	&dev_attr_mini_led_mode.attr,
 	&dev_attr_available_mini_led_mode.attr,
 	NULL
@@ -4131,6 +4173,9 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
 		devid = ASUS_WMI_DEVID_PANEL_OD;
 	else if (attr == &dev_attr_panel_fhd.attr)
 		devid = ASUS_WMI_DEVID_PANEL_FHD;
+	else if (attr == &dev_attr_cores_enabled.attr
+		|| attr == &dev_attr_cores_max.attr)
+		ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_CORES_SET);
 	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)
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 6c51d41ffc20..5a56e7e97785 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -116,6 +116,11 @@
 /* Charging mode - 1=Barrel, 2=USB */
 #define ASUS_WMI_DEVID_CHARGE_MODE	0x0012006C
 
+/* Intel E-core and P-core configuration in a format 0x0[E]0[P] */
+#define ASUS_WMI_DEVID_CORES_SET	0x001200D2
+ /* Maximum Intel E-core and P-core availability */
+#define ASUS_WMI_DEVID_CORES_MAX	0x001200D3
+
 /* MCU powersave mode */
 #define ASUS_WMI_DEVID_MCU_POWERSAVE   0x001200E2
 
-- 
2.45.1

Re: [PATCH 7/9] platform/x86: asus-wmi: add enable/disable CPU cores

[Ilpo Järvinen]

Hi,

Hans, please check my question below.

On Tue, 28 May 2024, Luke D. Jones wrote:

> Exposes the WMI functions for enable/disable of performance and
> efficiency cores on some laptop models (largely Intel only).
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---

> diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> index 3b4eeea75b7b..ac881e72e374 100644
> --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> @@ -226,3 +226,22 @@ Description:
>  		Set panel to UHD or FHD mode
>  			* 0 - UHD,
>  			* 1 - FHD
> +
> +What:		/sys/devices/platform/<platform>/cores_enabled
> +Date:		Jun 2024
> +KernelVersion:	6.11
> +Contact:	"Luke Jones" <luke@ljones.dev>
> +Description:
> +		Enable/disable efficiency and performance cores. The format is
> +		0x[E][P] where [E] is the efficiency core count, and [P] is
> +		the perfromance core count. If the core count is a single digit

performance

> +		it is preceded by a 0 such as 0x0406; E=4, P=6, 0x1006; E=10, P=6
> +
> +What:		/sys/devices/platform/<platform>/cores_max
> +Date:		Jun 2024
> +KernelVersion:	6.11
> +Contact:	"Luke Jones" <luke@ljones.dev>
> +Description:
> +		Show the maximum performance and efficiency core countin format
> +		0x[E][P] where [E] is the efficiency core count, and [P] is
> +		the perfromance core count.

performance

> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 4b045f1828f1..f62a36dfcd4b 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -815,6 +815,46 @@ static ssize_t panel_fhd_store(struct device *dev,
>  WMI_SIMPLE_SHOW(panel_fhd, "%d\n", ASUS_WMI_DEVID_PANEL_FHD);
>  static DEVICE_ATTR_RW(panel_fhd);
>  
> +/* Efficiency and Performance core control **********************************/
> +static ssize_t cores_enabled_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 cores, max;
> +
> +	result = kstrtou32(buf, 16, &cores);
> +	if (result)
> +		return result;
> +
> +	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_CORES_MAX, &max);
> +	if (err < 0)
> +		return err;
> +
> +	if (cores > max) {

This only checks one part of it and the P part can contain whatever 
garbage as long as E is small enough?

But I'm not sure if it's good idea to have these two changed through the 
same sysfs file, I'm leaning more on that it would be better to split the 
interface for P and E.

Hans, what you think about this?

> +		pr_warn("Core count 0x%x exceeds max: 0x%x\n", cores, max);
> +		return -EIO;
> +	}
> +
> +	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_CORES_SET, cores, &result);
> +	if (err) {
> +		pr_warn("Failed to set cores_enabled: %d\n", err);
> +		return err;
> +	}
> +
> +	pr_info("Enabled core count changed, reboot required\n");
> +	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "cores_enabled");
> +
> +	return count;
> +}

> @@ -4131,6 +4173,9 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
>  		devid = ASUS_WMI_DEVID_PANEL_OD;
>  	else if (attr == &dev_attr_panel_fhd.attr)
>  		devid = ASUS_WMI_DEVID_PANEL_FHD;
> +	else if (attr == &dev_attr_cores_enabled.attr
> +		|| attr == &dev_attr_cores_max.attr)

Wrong alignment.

-- 
 i.

Re: [PATCH 7/9] platform/x86: asus-wmi: add enable/disable CPU cores

[Luke Jones]

On Tue, 28 May 2024, at 9:27 PM, Ilpo Järvinen wrote:
> Hi,
> 
> Hans, please check my question below.
> 
> On Tue, 28 May 2024, Luke D. Jones wrote:
> 
> > Exposes the WMI functions for enable/disable of performance and
> > efficiency cores on some laptop models (largely Intel only).
> > 
> > Signed-off-by: Luke D. Jones <luke@ljones.dev>
> > ---
> 
> > diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> > index 3b4eeea75b7b..ac881e72e374 100644
> > --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> > +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> > @@ -226,3 +226,22 @@ Description:
> >  Set panel to UHD or FHD mode
> >  * 0 - UHD,
> >  * 1 - FHD
> > +
> > +What: /sys/devices/platform/<platform>/cores_enabled
> > +Date: Jun 2024
> > +KernelVersion: 6.11
> > +Contact: "Luke Jones" <luke@ljones.dev>
> > +Description:
> > + Enable/disable efficiency and performance cores. The format is
> > + 0x[E][P] where [E] is the efficiency core count, and [P] is
> > + the perfromance core count. If the core count is a single digit
> 
> performance
> 
> > + it is preceded by a 0 such as 0x0406; E=4, P=6, 0x1006; E=10, P=6
> > +
> > +What: /sys/devices/platform/<platform>/cores_max
> > +Date: Jun 2024
> > +KernelVersion: 6.11
> > +Contact: "Luke Jones" <luke@ljones.dev>
> > +Description:
> > + Show the maximum performance and efficiency core countin format
> > + 0x[E][P] where [E] is the efficiency core count, and [P] is
> > + the perfromance core count.
> 
> performance
> 
> > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> > index 4b045f1828f1..f62a36dfcd4b 100644
> > --- a/drivers/platform/x86/asus-wmi.c
> > +++ b/drivers/platform/x86/asus-wmi.c
> > @@ -815,6 +815,46 @@ static ssize_t panel_fhd_store(struct device *dev,
> >  WMI_SIMPLE_SHOW(panel_fhd, "%d\n", ASUS_WMI_DEVID_PANEL_FHD);
> >  static DEVICE_ATTR_RW(panel_fhd);
> >  
> > +/* Efficiency and Performance core control **********************************/
> > +static ssize_t cores_enabled_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 cores, max;
> > +
> > + result = kstrtou32(buf, 16, &cores);
> > + if (result)
> > + return result;
> > +
> > + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_CORES_MAX, &max);
> > + if (err < 0)
> > + return err;
> > +
> > + if (cores > max) {
> 
> This only checks one part of it and the P part can contain whatever 
> garbage as long as E is small enough?
> 
> But I'm not sure if it's good idea to have these two changed through the 
> same sysfs file, I'm leaning more on that it would be better to split the 
> interface for P and E.
> 
> Hans, what you think about this?

I'm inclined to agree. It's no issue to change it.

Ack all reviews. I'll work through them over the week but with a lower priority while I await both yours and Hans response in other replies with Mario regarding the firmware_attributes change/use.

> 
> > + pr_warn("Core count 0x%x exceeds max: 0x%x\n", cores, max);
> > + return -EIO;
> > + }
> > +
> > + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_CORES_SET, cores, &result);
> > + if (err) {
> > + pr_warn("Failed to set cores_enabled: %d\n", err);
> > + return err;
> > + }
> > +
> > + pr_info("Enabled core count changed, reboot required\n");
> > + sysfs_notify(&asus->platform_device->dev.kobj, NULL, "cores_enabled");
> > +
> > + return count;
> > +}
> 
> > @@ -4131,6 +4173,9 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
> >  devid = ASUS_WMI_DEVID_PANEL_OD;
> >  else if (attr == &dev_attr_panel_fhd.attr)
> >  devid = ASUS_WMI_DEVID_PANEL_FHD;
> > + else if (attr == &dev_attr_cores_enabled.attr
> > + || attr == &dev_attr_cores_max.attr)
> 
> Wrong alignment.
> 
> -- 
> i.
> 
> 

[PATCH 8/9] platform/x86: asus-wmi: add apu_mem setting

[Luke D. Jones]

Exposes the APU memory setting available on a few ASUS models such as
the ROG Ally.

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

diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index ac881e72e374..d221a3bc1a81 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -245,3 +245,11 @@ Description:
 		Show the maximum performance and efficiency core countin format
 		0x[E][P] where [E] is the efficiency core count, and [P] is
 		the perfromance core count.
+
+What:		/sys/devices/platform/<platform>/apu_mem
+Date:		Jun 2024
+KernelVersion:	6.11
+Contact:	"Luke Jones" <luke@ljones.dev>
+Description:
+		Set the maximum available system memory for the APU.
+		  * Min=0, Max=8
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index f62a36dfcd4b..4b5fbae8c563 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -855,6 +855,112 @@ static DEVICE_ATTR_RW(cores_enabled);
 WMI_SIMPLE_SHOW(cores_max, "0x%x\n", ASUS_WMI_DEVID_CORES_MAX);
 static DEVICE_ATTR_RO(cores_max);
 
+/* Device memory available to APU */
+
+static ssize_t apu_mem_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+	int err;
+	u32 mem;
+
+	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_APU_MEM, &mem);
+	if (err < 0)
+		return err;
+
+	switch (mem) {
+	case 256:
+		mem = 0;
+		break;
+	case 258:
+		mem = 1;
+		break;
+	case 259:
+		mem = 2;
+		break;
+	case 260:
+		mem = 3;
+		break;
+	case 261:
+		mem = 4;
+		break;
+	case 262:
+		mem = 8;
+		break;
+	case 263:
+		mem = 5;
+		break;
+	case 264:
+		mem = 6;
+		break;
+	case 265:
+		mem = 7;
+		break;
+	default:
+		mem = 4;
+		break;
+	}
+
+	return sysfs_emit(buf, "%d\n", mem);
+}
+
+static ssize_t apu_mem_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 mem;
+
+	result = kstrtou32(buf, 10, &mem);
+	if (result)
+		return result;
+
+	switch (mem) {
+	case 0:
+		mem = 0;
+		break;
+	case 1:
+		mem = 258;
+		break;
+	case 2:
+		mem = 259;
+		break;
+	case 3:
+		mem = 260;
+		break;
+	case 4:
+		mem = 261;
+		break;
+	case 5:
+		mem = 263;
+		break;
+	case 6:
+		mem = 264;
+		break;
+	case 7:
+		mem = 265;
+		break;
+	case 8:
+		mem = 262;
+		break;
+	default:
+		return -EIO;
+	}
+
+	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_APU_MEM, mem, &result);
+	if (err) {
+		pr_warn("Failed to set apu_mem: %d\n", err);
+		return err;
+	}
+
+	pr_info("APU memory changed, reboot required\n");
+	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "apu_mem");
+
+	return count;
+}
+static DEVICE_ATTR_RW(apu_mem);
+
 /* Tablet mode ****************************************************************/
 
 static void asus_wmi_tablet_mode_get_state(struct asus_wmi *asus)
@@ -4100,6 +4206,7 @@ static struct attribute *platform_attributes[] = {
 	&dev_attr_panel_fhd.attr,
 	&dev_attr_cores_enabled.attr,
 	&dev_attr_cores_max.attr,
+	&dev_attr_apu_mem.attr,
 	&dev_attr_mini_led_mode.attr,
 	&dev_attr_available_mini_led_mode.attr,
 	NULL
@@ -4176,6 +4283,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
 	else if (attr == &dev_attr_cores_enabled.attr
 		|| attr == &dev_attr_cores_max.attr)
 		ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_CORES_SET);
+	else if (attr == &dev_attr_apu_mem.attr)
+		ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_APU_MEM);
 	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)
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 5a56e7e97785..efe608861e55 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -121,6 +121,9 @@
  /* Maximum Intel E-core and P-core availability */
 #define ASUS_WMI_DEVID_CORES_MAX	0x001200D3
 
+/* Set the memory available to the APU */
+#define ASUS_WMI_DEVID_APU_MEM		0x000600C1
+
 /* MCU powersave mode */
 #define ASUS_WMI_DEVID_MCU_POWERSAVE   0x001200E2
 
-- 
2.45.1

Re: [PATCH 8/9] platform/x86: asus-wmi: add apu_mem setting

[Limonciello, Mario]

On 5/27/2024 8:36 PM, Luke D. Jones wrote:
> Exposes the APU memory setting available on a few ASUS models such as
> the ROG Ally.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>   .../ABI/testing/sysfs-platform-asus-wmi       |   8 ++
>   drivers/platform/x86/asus-wmi.c               | 109 ++++++++++++++++++
>   include/linux/platform_data/x86/asus-wmi.h    |   3 +
>   3 files changed, 120 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> index ac881e72e374..d221a3bc1a81 100644
> --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> @@ -245,3 +245,11 @@ Description:
>   		Show the maximum performance and efficiency core countin format
>   		0x[E][P] where [E] is the efficiency core count, and [P] is
>   		the perfromance core count.
> +
> +What:		/sys/devices/platform/<platform>/apu_mem
> +Date:		Jun 2024
> +KernelVersion:	6.11
> +Contact:	"Luke Jones" <luke@ljones.dev>
> +Description:
> +		Set the maximum available system memory for the APU.
> +		  * Min=0, Max=8

What is the unit?  It seems like multiples of something?

> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index f62a36dfcd4b..4b5fbae8c563 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -855,6 +855,112 @@ static DEVICE_ATTR_RW(cores_enabled);
>   WMI_SIMPLE_SHOW(cores_max, "0x%x\n", ASUS_WMI_DEVID_CORES_MAX);
>   static DEVICE_ATTR_RO(cores_max);
>   
> +/* Device memory available to APU */
> +
> +static ssize_t apu_mem_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +	int err;
> +	u32 mem;
> +
> +	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_APU_MEM, &mem);
> +	if (err < 0)
> +		return err;
> +
> +	switch (mem) {
> +	case 256:
> +		mem = 0;
> +		break;
> +	case 258:
> +		mem = 1;
> +		break;
> +	case 259:
> +		mem = 2;
> +		break;
> +	case 260:
> +		mem = 3;
> +		break;
> +	case 261:
> +		mem = 4;
> +		break;
> +	case 262:
> +		mem = 8;
> +		break;
> +	case 263:
> +		mem = 5;
> +		break;
> +	case 264:
> +		mem = 6;
> +		break;
> +	case 265:
> +		mem = 7;
> +		break;
> +	default:
> +		mem = 4;
> +		break;
> +	}
> +
> +	return sysfs_emit(buf, "%d\n", mem);
> +}
> +
> +static ssize_t apu_mem_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 mem;
> +
> +	result = kstrtou32(buf, 10, &mem);
> +	if (result)
> +		return result;
> +
> +	switch (mem) {
> +	case 0:
> +		mem = 0;
> +		break;
> +	case 1:
> +		mem = 258;
> +		break;
> +	case 2:
> +		mem = 259;
> +		break;
> +	case 3:
> +		mem = 260;
> +		break;
> +	case 4:
> +		mem = 261;
> +		break;
> +	case 5:
> +		mem = 263;
> +		break;
> +	case 6:
> +		mem = 264;
> +		break;
> +	case 7:
> +		mem = 265;
> +		break;
> +	case 8:
> +		mem = 262;

Is case 8 a mistake, or intentionally out of order?

> +		break;
> +	default:
> +		return -EIO;
> +	}
> +
> +	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_APU_MEM, mem, &result);
> +	if (err) {
> +		pr_warn("Failed to set apu_mem: %d\n", err);
> +		return err;
> +	}
> +
> +	pr_info("APU memory changed, reboot required\n");

If you're logging something into the logs for this, I'd say make it more 
useful.

"APU memory changed to %d MB"

> +	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "apu_mem");

So this is a case that the BIOS attributes API I mentioned before would 
be REALLY useful.  There is a pending_reboot sysfs file that userspace 
can query to know if a given setting requires a reboot or not.

Fwupd also uses this attribute to know to delay BIOS updates until the 
system has been rebooted.

> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(apu_mem);
> +
>   /* Tablet mode ****************************************************************/
>   
>   static void asus_wmi_tablet_mode_get_state(struct asus_wmi *asus)
> @@ -4100,6 +4206,7 @@ static struct attribute *platform_attributes[] = {
>   	&dev_attr_panel_fhd.attr,
>   	&dev_attr_cores_enabled.attr,
>   	&dev_attr_cores_max.attr,
> +	&dev_attr_apu_mem.attr,
>   	&dev_attr_mini_led_mode.attr,
>   	&dev_attr_available_mini_led_mode.attr,
>   	NULL
> @@ -4176,6 +4283,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
>   	else if (attr == &dev_attr_cores_enabled.attr
>   		|| attr == &dev_attr_cores_max.attr)
>   		ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_CORES_SET);
> +	else if (attr == &dev_attr_apu_mem.attr)
> +		ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_APU_MEM);
>   	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)
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index 5a56e7e97785..efe608861e55 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -121,6 +121,9 @@
>    /* Maximum Intel E-core and P-core availability */
>   #define ASUS_WMI_DEVID_CORES_MAX	0x001200D3
>   
> +/* Set the memory available to the APU */
> +#define ASUS_WMI_DEVID_APU_MEM		0x000600C1
> +
>   /* MCU powersave mode */
>   #define ASUS_WMI_DEVID_MCU_POWERSAVE   0x001200E2
>   

Re: [PATCH 8/9] platform/x86: asus-wmi: add apu_mem setting

[Luke Jones]

On Tue, 28 May 2024, at 2:19 PM, Limonciello, Mario wrote:
> 
> 
> On 5/27/2024 8:36 PM, Luke D. Jones wrote:
> > Exposes the APU memory setting available on a few ASUS models such as
> > the ROG Ally.
> > 
> > Signed-off-by: Luke D. Jones <luke@ljones.dev>
> > ---
> >   .../ABI/testing/sysfs-platform-asus-wmi       |   8 ++
> >   drivers/platform/x86/asus-wmi.c               | 109 ++++++++++++++++++
> >   include/linux/platform_data/x86/asus-wmi.h    |   3 +
> >   3 files changed, 120 insertions(+)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> > index ac881e72e374..d221a3bc1a81 100644
> > --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> > +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> > @@ -245,3 +245,11 @@ Description:
> >   Show the maximum performance and efficiency core countin format
> >   0x[E][P] where [E] is the efficiency core count, and [P] is
> >   the perfromance core count.
> > +
> > +What: /sys/devices/platform/<platform>/apu_mem
> > +Date: Jun 2024
> > +KernelVersion: 6.11
> > +Contact: "Luke Jones" <luke@ljones.dev>
> > +Description:
> > + Set the maximum available system memory for the APU.
> > +   * Min=0, Max=8
> 
> What is the unit?  It seems like multiples of something?

It's GB, looks like I didn't save my work when I did a rebase and update of this patch. I'll add to my todo list for next version

> > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> > index f62a36dfcd4b..4b5fbae8c563 100644
> > --- a/drivers/platform/x86/asus-wmi.c
> > +++ b/drivers/platform/x86/asus-wmi.c
> > @@ -855,6 +855,112 @@ static DEVICE_ATTR_RW(cores_enabled);
> >   WMI_SIMPLE_SHOW(cores_max, "0x%x\n", ASUS_WMI_DEVID_CORES_MAX);
> >   static DEVICE_ATTR_RO(cores_max);
> >   
> > +/* Device memory available to APU */
> > +
> > +static ssize_t apu_mem_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct asus_wmi *asus = dev_get_drvdata(dev);
> > + int err;
> > + u32 mem;
> > +
> > + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_APU_MEM, &mem);
> > + if (err < 0)
> > + return err;
> > +
> > + switch (mem) {
> > + case 256:
> > + mem = 0;
> > + break;
> > + case 258:
> > + mem = 1;
> > + break;
> > + case 259:
> > + mem = 2;
> > + break;
> > + case 260:
> > + mem = 3;
> > + break;
> > + case 261:
> > + mem = 4;
> > + break;
> > + case 262:
> > + mem = 8;
> > + break;
> > + case 263:
> > + mem = 5;
> > + break;
> > + case 264:
> > + mem = 6;
> > + break;
> > + case 265:
> > + mem = 7;
> > + break;
> > + default:
> > + mem = 4;
> > + break;
> > + }
> > +
> > + return sysfs_emit(buf, "%d\n", mem);
> > +}
> > +
> > +static ssize_t apu_mem_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 mem;
> > +
> > + result = kstrtou32(buf, 10, &mem);
> > + if (result)
> > + return result;
> > +
> > + switch (mem) {
> > + case 0:
> > + mem = 0;
> > + break;
> > + case 1:
> > + mem = 258;
> > + break;
> > + case 2:
> > + mem = 259;
> > + break;
> > + case 3:
> > + mem = 260;
> > + break;
> > + case 4:
> > + mem = 261;
> > + break;
> > + case 5:
> > + mem = 263;
> > + break;
> > + case 6:
> > + mem = 264;
> > + break;
> > + case 7:
> > + mem = 265;
> > + break;
> > + case 8:
> > + mem = 262;
> 
> Is case 8 a mistake, or intentionally out of order?

Do you mean the `mem = <val>`? Those aren't in order, and I thought it was easier to read if the switch was ordered.

> 
> > + break;
> > + default:
> > + return -EIO;
> > + }
> > +
> > + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_APU_MEM, mem, &result);
> > + if (err) {
> > + pr_warn("Failed to set apu_mem: %d\n", err);
> > + return err;
> > + }
> > +
> > + pr_info("APU memory changed, reboot required\n");
> 
> If you're logging something into the logs for this, I'd say make it more 
> useful.
> 
> "APU memory changed to %d MB"

Agreed. There's probably a few other spots I can do this also.

> 
> > + sysfs_notify(&asus->platform_device->dev.kobj, NULL, "apu_mem");
> 
> So this is a case that the BIOS attributes API I mentioned before would 
> be REALLY useful.  There is a pending_reboot sysfs file that userspace 
> can query to know if a given setting requires a reboot or not.
> 
> Fwupd also uses this attribute to know to delay BIOS updates until the 
> system has been rebooted.

Oh! Yes I'll queue that as an additional patch. There's at least 2 or 3 other spots where that would be good to have.

> > +
> > + return count;
> > +}
> > +static DEVICE_ATTR_RW(apu_mem);
> > +
> >   /* Tablet mode ****************************************************************/
> >   
> >   static void asus_wmi_tablet_mode_get_state(struct asus_wmi *asus)
> > @@ -4100,6 +4206,7 @@ static struct attribute *platform_attributes[] = {
> >   &dev_attr_panel_fhd.attr,
> >   &dev_attr_cores_enabled.attr,
> >   &dev_attr_cores_max.attr,
> > + &dev_attr_apu_mem.attr,
> >   &dev_attr_mini_led_mode.attr,
> >   &dev_attr_available_mini_led_mode.attr,
> >   NULL
> > @@ -4176,6 +4283,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
> >   else if (attr == &dev_attr_cores_enabled.attr
> >   || attr == &dev_attr_cores_max.attr)
> >   ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_CORES_SET);
> > + else if (attr == &dev_attr_apu_mem.attr)
> > + ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_APU_MEM);
> >   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)
> > diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> > index 5a56e7e97785..efe608861e55 100644
> > --- a/include/linux/platform_data/x86/asus-wmi.h
> > +++ b/include/linux/platform_data/x86/asus-wmi.h
> > @@ -121,6 +121,9 @@
> >    /* Maximum Intel E-core and P-core availability */
> >   #define ASUS_WMI_DEVID_CORES_MAX 0x001200D3
> >   
> > +/* Set the memory available to the APU */
> > +#define ASUS_WMI_DEVID_APU_MEM 0x000600C1
> > +
> >   /* MCU powersave mode */
> >   #define ASUS_WMI_DEVID_MCU_POWERSAVE   0x001200E2
> >   
> 

Re: [PATCH 8/9] platform/x86: asus-wmi: add apu_mem setting

[Mario Limonciello]

>>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
>>> index f62a36dfcd4b..4b5fbae8c563 100644
>>> --- a/drivers/platform/x86/asus-wmi.c
>>> +++ b/drivers/platform/x86/asus-wmi.c
>>> @@ -855,6 +855,112 @@ static DEVICE_ATTR_RW(cores_enabled);
>>>    WMI_SIMPLE_SHOW(cores_max, "0x%x\n", ASUS_WMI_DEVID_CORES_MAX);
>>>    static DEVICE_ATTR_RO(cores_max);
>>>    
>>> +/* Device memory available to APU */
>>> +
>>> +static ssize_t apu_mem_show(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct asus_wmi *asus = dev_get_drvdata(dev);
>>> + int err;
>>> + u32 mem;
>>> +
>>> + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_APU_MEM, &mem);
>>> + if (err < 0)
>>> + return err;
>>> +
>>> + switch (mem) {
>>> + case 256:
>>> + mem = 0;
>>> + break;
>>> + case 258:
>>> + mem = 1;
>>> + break;
>>> + case 259:
>>> + mem = 2;
>>> + break;
>>> + case 260:
>>> + mem = 3;
>>> + break;
>>> + case 261:
>>> + mem = 4;
>>> + break;
>>> + case 262:
>>> + mem = 8;
>>> + break;
>>> + case 263:
>>> + mem = 5;
>>> + break;
>>> + case 264:
>>> + mem = 6;
>>> + break;
>>> + case 265:
>>> + mem = 7;
>>> + break;
>>> + default:
>>> + mem = 4;
>>> + break;
>>> + }
>>> +
>>> + return sysfs_emit(buf, "%d\n", mem);
>>> +}
>>> +
>>> +static ssize_t apu_mem_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 mem;
>>> +
>>> + result = kstrtou32(buf, 10, &mem);
>>> + if (result)
>>> + return result;
>>> +
>>> + switch (mem) {
>>> + case 0:
>>> + mem = 0;
>>> + break;
>>> + case 1:
>>> + mem = 258;
>>> + break;
>>> + case 2:
>>> + mem = 259;
>>> + break;
>>> + case 3:
>>> + mem = 260;
>>> + break;
>>> + case 4:
>>> + mem = 261;
>>> + break;
>>> + case 5:
>>> + mem = 263;
>>> + break;
>>> + case 6:
>>> + mem = 264;
>>> + break;
>>> + case 7:
>>> + mem = 265;
>>> + break;
>>> + case 8:
>>> + mem = 262;
>>
>> Is case 8 a mistake, or intentionally out of order?
> 
> Do you mean the `mem = <val>`? Those aren't in order, and I thought it was easier to read if the switch was ordered.
> 

I'm wondering if case 5 should be 262, case 6 263, case 7 264 and case 8 
265.  It just stood out to me.

If that's all intended then no concerns and I agree sorting the case is 
better.

>>
>>> + break;
>>> + default:
>>> + return -EIO;
>>> + }
>>> +
>>> + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_APU_MEM, mem, &result);
>>> + if (err) {
>>> + pr_warn("Failed to set apu_mem: %d\n", err);
>>> + return err;
>>> + }
>>> +
>>> + pr_info("APU memory changed, reboot required\n");
>>
>> If you're logging something into the logs for this, I'd say make it more
>> useful.
>>
>> "APU memory changed to %d MB"
> 
> Agreed. There's probably a few other spots I can do this also.
> 
>>
>>> + sysfs_notify(&asus->platform_device->dev.kobj, NULL, "apu_mem");
>>
>> So this is a case that the BIOS attributes API I mentioned before would
>> be REALLY useful.  There is a pending_reboot sysfs file that userspace
>> can query to know if a given setting requires a reboot or not.
>>
>> Fwupd also uses this attribute to know to delay BIOS updates until the
>> system has been rebooted.
> 
> Oh! Yes I'll queue that as an additional patch. There's at least 2 or 3 other spots where that would be good to have.
> 

For any "new" attributes it's better to put them in that API than code 
duplication of the BIOS attributes API as well as a random sysfs file 
API that you can never discard.

>>> +
>>> + return count;
>>> +}
>>> +static DEVICE_ATTR_RW(apu_mem);
>>> +
>>>    /* Tablet mode ****************************************************************/
>>>    
>>>    static void asus_wmi_tablet_mode_get_state(struct asus_wmi *asus)
>>> @@ -4100,6 +4206,7 @@ static struct attribute *platform_attributes[] = {
>>>    &dev_attr_panel_fhd.attr,
>>>    &dev_attr_cores_enabled.attr,
>>>    &dev_attr_cores_max.attr,
>>> + &dev_attr_apu_mem.attr,
>>>    &dev_attr_mini_led_mode.attr,
>>>    &dev_attr_available_mini_led_mode.attr,
>>>    NULL
>>> @@ -4176,6 +4283,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
>>>    else if (attr == &dev_attr_cores_enabled.attr
>>>    || attr == &dev_attr_cores_max.attr)
>>>    ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_CORES_SET);
>>> + else if (attr == &dev_attr_apu_mem.attr)
>>> + ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_APU_MEM);
>>>    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)
>>> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
>>> index 5a56e7e97785..efe608861e55 100644
>>> --- a/include/linux/platform_data/x86/asus-wmi.h
>>> +++ b/include/linux/platform_data/x86/asus-wmi.h
>>> @@ -121,6 +121,9 @@
>>>     /* Maximum Intel E-core and P-core availability */
>>>    #define ASUS_WMI_DEVID_CORES_MAX 0x001200D3
>>>    
>>> +/* Set the memory available to the APU */
>>> +#define ASUS_WMI_DEVID_APU_MEM 0x000600C1
>>> +
>>>    /* MCU powersave mode */
>>>    #define ASUS_WMI_DEVID_MCU_POWERSAVE   0x001200E2
>>>    
>>

Re: [PATCH 8/9] platform/x86: asus-wmi: add apu_mem setting

[Luke Jones]

On Wed, 29 May 2024, at 1:27 AM, Mario Limonciello wrote:
> >>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> >>> index f62a36dfcd4b..4b5fbae8c563 100644
> >>> --- a/drivers/platform/x86/asus-wmi.c
> >>> +++ b/drivers/platform/x86/asus-wmi.c
> >>> @@ -855,6 +855,112 @@ static DEVICE_ATTR_RW(cores_enabled);
> >>>    WMI_SIMPLE_SHOW(cores_max, "0x%x\n", ASUS_WMI_DEVID_CORES_MAX);
> >>>    static DEVICE_ATTR_RO(cores_max);
> >>>    
> >>> +/* Device memory available to APU */
> >>> +
> >>> +static ssize_t apu_mem_show(struct device *dev,
> >>> + struct device_attribute *attr, char *buf)
> >>> +{
> >>> + struct asus_wmi *asus = dev_get_drvdata(dev);
> >>> + int err;
> >>> + u32 mem;
> >>> +
> >>> + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_APU_MEM, &mem);
> >>> + if (err < 0)
> >>> + return err;
> >>> +
> >>> + switch (mem) {
> >>> + case 256:
> >>> + mem = 0;
> >>> + break;
> >>> + case 258:
> >>> + mem = 1;
> >>> + break;
> >>> + case 259:
> >>> + mem = 2;
> >>> + break;
> >>> + case 260:
> >>> + mem = 3;
> >>> + break;
> >>> + case 261:
> >>> + mem = 4;
> >>> + break;
> >>> + case 262:
> >>> + mem = 8;
> >>> + break;
> >>> + case 263:
> >>> + mem = 5;
> >>> + break;
> >>> + case 264:
> >>> + mem = 6;
> >>> + break;
> >>> + case 265:
> >>> + mem = 7;
> >>> + break;
> >>> + default:
> >>> + mem = 4;
> >>> + break;
> >>> + }
> >>> +
> >>> + return sysfs_emit(buf, "%d\n", mem);
> >>> +}
> >>> +
> >>> +static ssize_t apu_mem_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 mem;
> >>> +
> >>> + result = kstrtou32(buf, 10, &mem);
> >>> + if (result)
> >>> + return result;
> >>> +
> >>> + switch (mem) {
> >>> + case 0:
> >>> + mem = 0;
> >>> + break;
> >>> + case 1:
> >>> + mem = 258;
> >>> + break;
> >>> + case 2:
> >>> + mem = 259;
> >>> + break;
> >>> + case 3:
> >>> + mem = 260;
> >>> + break;
> >>> + case 4:
> >>> + mem = 261;
> >>> + break;
> >>> + case 5:
> >>> + mem = 263;
> >>> + break;
> >>> + case 6:
> >>> + mem = 264;
> >>> + break;
> >>> + case 7:
> >>> + mem = 265;
> >>> + break;
> >>> + case 8:
> >>> + mem = 262;
> >>
> >> Is case 8 a mistake, or intentionally out of order?
> > 
> > Do you mean the `mem = <val>`? Those aren't in order, and I thought it was easier to read if the switch was ordered.
> > 
> 
> I'm wondering if case 5 should be 262, case 6 263, case 7 264 and case 8 
> 265.  It just stood out to me.

Yeah it's weird but that is what it is. Also verified in ghelper which calls the same WMI interfaces in Windows.

> 
> If that's all intended then no concerns and I agree sorting the case is 
> better.
> 
> >>
> >>> + break;
> >>> + default:
> >>> + return -EIO;
> >>> + }
> >>> +
> >>> + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_APU_MEM, mem, &result);
> >>> + if (err) {
> >>> + pr_warn("Failed to set apu_mem: %d\n", err);
> >>> + return err;
> >>> + }
> >>> +
> >>> + pr_info("APU memory changed, reboot required\n");
> >>
> >> If you're logging something into the logs for this, I'd say make it more
> >> useful.
> >>
> >> "APU memory changed to %d MB"
> > 
> > Agreed. There's probably a few other spots I can do this also.
> > 
> >>
> >>> + sysfs_notify(&asus->platform_device->dev.kobj, NULL, "apu_mem");
> >>
> >> So this is a case that the BIOS attributes API I mentioned before would
> >> be REALLY useful.  There is a pending_reboot sysfs file that userspace
> >> can query to know if a given setting requires a reboot or not.
> >>
> >> Fwupd also uses this attribute to know to delay BIOS updates until the
> >> system has been rebooted.
> > 
> > Oh! Yes I'll queue that as an additional patch. There's at least 2 or 3 other spots where that would be good to have.
> > 
> 
> For any "new" attributes it's better to put them in that API than code 
> duplication of the BIOS attributes API as well as a random sysfs file 
> API that you can never discard.

Do you mean the firmware_attributes API? If so, I'm not opposed to adding all the existing ROG attributes to it also.

If I'm understanding the docs correctly, for example this apu_mem attr would then become:
- /sys/class/firmware-attributes/asus-bios/attributes/apu_mem/type
- /sys/class/firmware-attributes/*/attributes/apu_mem/current_value
- /sys/class/firmware-attributes/*/attributes/apu_mem/default_value
- /sys/class/firmware-attributes/*/attributes/apu_mem/display_name
- /sys/class/firmware-attributes/*/attributes/apu_mem/possible_values
- ..etc

That's absolutely much better than what I've been doing and I wish I'd known about it sooner.

So if I go ahead and convert all the new attr to this are there any issues with also converting much of the previous attr? And I'm aware of "don't break userspace" so really I'm a bit unsure how best to manage that (would a new module be better here also? "asus-bios.c" perhaps).

What I don't want is a split between platform and firmware_attributes.

> 
> >>> +
> >>> + return count;
> >>> +}
> >>> +static DEVICE_ATTR_RW(apu_mem);
> >>> +
> >>>    /* Tablet mode ****************************************************************/
> >>>    
> >>>    static void asus_wmi_tablet_mode_get_state(struct asus_wmi *asus)
> >>> @@ -4100,6 +4206,7 @@ static struct attribute *platform_attributes[] = {
> >>>    &dev_attr_panel_fhd.attr,
> >>>    &dev_attr_cores_enabled.attr,
> >>>    &dev_attr_cores_max.attr,
> >>> + &dev_attr_apu_mem.attr,
> >>>    &dev_attr_mini_led_mode.attr,
> >>>    &dev_attr_available_mini_led_mode.attr,
> >>>    NULL
> >>> @@ -4176,6 +4283,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
> >>>    else if (attr == &dev_attr_cores_enabled.attr
> >>>    || attr == &dev_attr_cores_max.attr)
> >>>    ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_CORES_SET);
> >>> + else if (attr == &dev_attr_apu_mem.attr)
> >>> + ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_APU_MEM);
> >>>    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)
> >>> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> >>> index 5a56e7e97785..efe608861e55 100644
> >>> --- a/include/linux/platform_data/x86/asus-wmi.h
> >>> +++ b/include/linux/platform_data/x86/asus-wmi.h
> >>> @@ -121,6 +121,9 @@
> >>>     /* Maximum Intel E-core and P-core availability */
> >>>    #define ASUS_WMI_DEVID_CORES_MAX 0x001200D3
> >>>    
> >>> +/* Set the memory available to the APU */
> >>> +#define ASUS_WMI_DEVID_APU_MEM 0x000600C1
> >>> +
> >>>    /* MCU powersave mode */
> >>>    #define ASUS_WMI_DEVID_MCU_POWERSAVE   0x001200E2
> >>>    
> >>
> 
> 

Re: [PATCH 8/9] platform/x86: asus-wmi: add apu_mem setting

[Mario Limonciello]

>> For any "new" attributes it's better to put them in that API than code
>> duplication of the BIOS attributes API as well as a random sysfs file
>> API that you can never discard.
> 
> Do you mean the firmware_attributes API? If so, I'm not opposed to adding all the existing ROG attributes to it also.
> 
> If I'm understanding the docs correctly, for example this apu_mem attr would then become:
> - /sys/class/firmware-attributes/asus-bios/attributes/apu_mem/type
> - /sys/class/firmware-attributes/*/attributes/apu_mem/current_value
> - /sys/class/firmware-attributes/*/attributes/apu_mem/default_value
> - /sys/class/firmware-attributes/*/attributes/apu_mem/display_name
> - /sys/class/firmware-attributes/*/attributes/apu_mem/possible_values
> - ..etc
> 
> That's absolutely much better than what I've been doing and I wish I'd known about it sooner.

Yup!  And even better "fwupdmgr get-bios-setting" and "fwupdmgr 
set-bios-setting" [1] will work with bash completion too.

If you want to build a GUI around it, it could use the fwupd dbus 
interface for getting those settings and manipulating them.  The dbus 
interface policy is protected by PK.

> 
> So if I go ahead and convert all the new attr to this are there any issues with also converting much of the previous attr? And I'm aware of "don't break userspace" so really I'm a bit unsure how best to manage that (would a new module be better here also? "asus-bios.c" perhaps).
> 
> What I don't want is a split between platform and firmware_attributes.
> 

My personal thought is add everything to the new API and then hide the 
"old" interface behind some boolean Kconfig option that defaults to 
disabled.  If someone enables it then when it's "used" (IE read from / 
written to) then do something like a pr_warn_once() that it's a 
deprecated interface and it will go away.  Wait until next LTS kernel, 
and then rip it out after that.

Need Hans and/or Ilpo agree to make with that though before you spend 
too much effort on it though.

[1] https://fwupd.github.io/libfwupdplugin/bios-settings.html

Re: [PATCH 8/9] platform/x86: asus-wmi: add apu_mem setting

[Luke Jones]

On Wed, 29 May 2024, at 9:16 AM, Mario Limonciello wrote:
> >> For any "new" attributes it's better to put them in that API than code
> >> duplication of the BIOS attributes API as well as a random sysfs file
> >> API that you can never discard.
> > 
> > Do you mean the firmware_attributes API? If so, I'm not opposed to adding all the existing ROG attributes to it also.
> > 
> > If I'm understanding the docs correctly, for example this apu_mem attr would then become:
> > - /sys/class/firmware-attributes/asus-bios/attributes/apu_mem/type
> > - /sys/class/firmware-attributes/*/attributes/apu_mem/current_value
> > - /sys/class/firmware-attributes/*/attributes/apu_mem/default_value
> > - /sys/class/firmware-attributes/*/attributes/apu_mem/display_name
> > - /sys/class/firmware-attributes/*/attributes/apu_mem/possible_values
> > - ..etc
> > 
> > That's absolutely much better than what I've been doing and I wish I'd known about it sooner.
> 
> Yup!  And even better "fwupdmgr get-bios-setting" and "fwupdmgr 
> set-bios-setting" [1] will work with bash completion too.
> 
> If you want to build a GUI around it, it could use the fwupd dbus 
> interface for getting those settings and manipulating them. 

My existing GUI effort revolves around me adding these attrs to both kernel and a dbus interface. So if I can drop that then all the better

> The dbus 
> interface policy is protected by PK.

What is PK? And I'm guessing this restricts users to requiring permissions to change settings?
 
> > 
> > So if I go ahead and convert all the new attr to this are there any issues with also converting much of the previous attr? And I'm aware of "don't break userspace" so really I'm a bit unsure how best to manage that (would a new module be better here also? "asus-bios.c" perhaps).
> > 
> > What I don't want is a split between platform and firmware_attributes.
> > 
> 
> My personal thought is add everything to the new API and then hide the 
> "old" interface behind some boolean Kconfig option that defaults to 
> disabled.  If someone enables it then when it's "used" (IE read from / 
> written to) then do something like a pr_warn_once() that it's a 
> deprecated interface and it will go away.  Wait until next LTS kernel, 
> and then rip it out after that.

I'm happy to do this. On a serious note though, I should make a new module for this right? It would make it much much easier to manage the "conversion" I think, and then it would act as a separation of concerns.

> 
> Need Hans and/or Ilpo agree to make with that though before you spend 
> too much effort on it though.

Ack

> 
> [1] https://fwupd.github.io/libfwupdplugin/bios-settings.html
> 

Re: [PATCH 8/9] platform/x86: asus-wmi: add apu_mem setting

[Mario Limonciello]

On 5/28/2024 16:34, Luke Jones wrote:
> 
> 
> On Wed, 29 May 2024, at 9:16 AM, Mario Limonciello wrote:
>>>> For any "new" attributes it's better to put them in that API than code
>>>> duplication of the BIOS attributes API as well as a random sysfs file
>>>> API that you can never discard.
>>>
>>> Do you mean the firmware_attributes API? If so, I'm not opposed to adding all the existing ROG attributes to it also.
>>>
>>> If I'm understanding the docs correctly, for example this apu_mem attr would then become:
>>> - /sys/class/firmware-attributes/asus-bios/attributes/apu_mem/type
>>> - /sys/class/firmware-attributes/*/attributes/apu_mem/current_value
>>> - /sys/class/firmware-attributes/*/attributes/apu_mem/default_value
>>> - /sys/class/firmware-attributes/*/attributes/apu_mem/display_name
>>> - /sys/class/firmware-attributes/*/attributes/apu_mem/possible_values
>>> - ..etc
>>>
>>> That's absolutely much better than what I've been doing and I wish I'd known about it sooner.
>>
>> Yup!  And even better "fwupdmgr get-bios-setting" and "fwupdmgr
>> set-bios-setting" [1] will work with bash completion too.
>>
>> If you want to build a GUI around it, it could use the fwupd dbus
>> interface for getting those settings and manipulating them.
> 
> My existing GUI effort revolves around me adding these attrs to both kernel and a dbus interface. So if I can drop that then all the better
> 
>> The dbus
>> interface policy is protected by PK.
> 
> What is PK? And I'm guessing this restricts users to requiring permissions to change settings?
>   
>>>
>>> So if I go ahead and convert all the new attr to this are there any issues with also converting much of the previous attr? And I'm aware of "don't break userspace" so really I'm a bit unsure how best to manage that (would a new module be better here also? "asus-bios.c" perhaps).
>>>
>>> What I don't want is a split between platform and firmware_attributes.
>>>
>>
>> My personal thought is add everything to the new API and then hide the
>> "old" interface behind some boolean Kconfig option that defaults to
>> disabled.  If someone enables it then when it's "used" (IE read from /
>> written to) then do something like a pr_warn_once() that it's a
>> deprecated interface and it will go away.  Wait until next LTS kernel,
>> and then rip it out after that.
> 
> I'm happy to do this. On a serious note though, I should make a new module for this right? It would make it much much easier to manage the "conversion" I think, and then it would act as a separation of concerns.

Yeah; I agree with you I think this is a good idea.

> 
>>
>> Need Hans and/or Ilpo agree to make with that though before you spend
>> too much effort on it though.
> 
> Ack
> 
>>
>> [1] https://fwupd.github.io/libfwupdplugin/bios-settings.html
>>

Re: [PATCH 8/9] platform/x86: asus-wmi: add apu_mem setting

[Mario Limonciello]

On 5/28/2024 16:34, Luke Jones wrote:
> 
> 
> On Wed, 29 May 2024, at 9:16 AM, Mario Limonciello wrote:
>>>> For any "new" attributes it's better to put them in that API than code
>>>> duplication of the BIOS attributes API as well as a random sysfs file
>>>> API that you can never discard.
>>>
>>> Do you mean the firmware_attributes API? If so, I'm not opposed to adding all the existing ROG attributes to it also.
>>>
>>> If I'm understanding the docs correctly, for example this apu_mem attr would then become:
>>> - /sys/class/firmware-attributes/asus-bios/attributes/apu_mem/type
>>> - /sys/class/firmware-attributes/*/attributes/apu_mem/current_value
>>> - /sys/class/firmware-attributes/*/attributes/apu_mem/default_value
>>> - /sys/class/firmware-attributes/*/attributes/apu_mem/display_name
>>> - /sys/class/firmware-attributes/*/attributes/apu_mem/possible_values
>>> - ..etc
>>>
>>> That's absolutely much better than what I've been doing and I wish I'd known about it sooner.
>>
>> Yup!  And even better "fwupdmgr get-bios-setting" and "fwupdmgr
>> set-bios-setting" [1] will work with bash completion too.
>>
>> If you want to build a GUI around it, it could use the fwupd dbus
>> interface for getting those settings and manipulating them.
> 
> My existing GUI effort revolves around me adding these attrs to both kernel and a dbus interface. So if I can drop that then all the better

Awesome!  You can either use libfwupd or the dbus interface.  After 
you're done with the kernel work ping me or Richard if you have problems.

> 
>> The dbus
>> interface policy is protected by PK.
> 
> What is PK? And I'm guessing this restricts users to requiring permissions to change settings?
>   

Whoops forgot to respond to these.

PK = PolicyKit.

[PATCH 9/9] platform/x86: asus-wmi: add setting dGPU TGP

[Luke D. Jones]

The dGPU TGP can be set on a few ROG laptops. This gnerally needs some
extra userspace inference to find the base and limit:

- Find max-tgp from nvidia-smi
- Find max-boost from nv_dynamic_boost_max (asus_wmi)
- (max-tgp - max-boost - initial TGP) = base TGP
- (max-tgp - max-boost - base TGP) = max additional TGP

On some laptops the dgpu_base_tgp may return 0, in which case the base
TGP must be derived as above.

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

diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index d221a3bc1a81..46df3452c9da 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -253,3 +253,22 @@ Contact:	"Luke Jones" <luke@ljones.dev>
 Description:
 		Set the maximum available system memory for the APU.
 		  * Min=0, Max=8
+
+What:		/sys/devices/platform/<platform>/dgpu_tgp
+Date:		Jun 2024
+KernelVersion:	6.11
+Contact:	"Luke Jones" <luke@ljones.dev>
+Description:
+		Read and set the extra TGP applied to the dGPU. This is applied on
+		top of the dgpu_base_tgp.
+
+		If the dGPU maximum power including boost is 175 then we can calculate:
+		175 - 25 (dynamic boost) - 70 (initial dgpu_tgp) = 80 (dgpu_base_tgp).
+		For NVIDIA dGPU the max power can be found with nvidia-smi.
+
+What:		/sys/devices/platform/<platform>/dgpu_base_tgp
+Date:		Jun 2024
+KernelVersion:	6.11
+Contact:	"Luke Jones" <luke@ljones.dev>
+Description:
+		Read the absolute base TGP of the dGPU.
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 4b5fbae8c563..4d291429e7a1 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -123,6 +123,7 @@ module_param(fnlock_default, bool, 0444);
 #define NVIDIA_BOOST_MAX	25
 #define NVIDIA_TEMP_MIN		75
 #define NVIDIA_TEMP_MAX		87
+#define NVIDIA_GPU_POWER_MAX	70
 
 #define ASUS_SCREENPAD_BRIGHT_MIN 20
 #define ASUS_SCREENPAD_BRIGHT_MAX 255
@@ -797,6 +798,8 @@ WMI_ATTR_SIMPLE_RO(egpu_connected, ASUS_WMI_DEVID_EGPU_CONNECTED);
 WMI_ATTR_SIMPLE_RW(panel_od, 0, 1, ASUS_WMI_DEVID_PANEL_OD);
 WMI_ATTR_SIMPLE_RW(boot_sound, 0, 1, ASUS_WMI_DEVID_BOOT_SOUND);
 WMI_ATTR_SIMPLE_RO(charge_mode, ASUS_WMI_DEVID_CHARGE_MODE);
+WMI_ATTR_SIMPLE_RO(dgpu_base_tgp, ASUS_WMI_DEVID_DGPU_BASE_TGP);
+WMI_ATTR_SIMPLE_RW(dgpu_tgp, 0, NVIDIA_GPU_POWER_MAX, ASUS_WMI_DEVID_DGPU_SET_TGP);
 
 static ssize_t panel_fhd_store(struct device *dev,
 	struct device_attribute *attr, const char *buf, size_t count)
@@ -4204,6 +4207,8 @@ static struct attribute *platform_attributes[] = {
 	&dev_attr_boot_sound.attr,
 	&dev_attr_panel_od.attr,
 	&dev_attr_panel_fhd.attr,
+	&dev_attr_dgpu_base_tgp.attr,
+	&dev_attr_dgpu_tgp.attr,
 	&dev_attr_cores_enabled.attr,
 	&dev_attr_cores_max.attr,
 	&dev_attr_apu_mem.attr,
@@ -4280,6 +4285,10 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
 		devid = ASUS_WMI_DEVID_PANEL_OD;
 	else if (attr == &dev_attr_panel_fhd.attr)
 		devid = ASUS_WMI_DEVID_PANEL_FHD;
+	else if (attr == &dev_attr_dgpu_base_tgp.attr)
+		devid = ASUS_WMI_DEVID_DGPU_BASE_TGP;
+	else if (attr == &dev_attr_dgpu_tgp.attr)
+		ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_DGPU_SET_TGP);
 	else if (attr == &dev_attr_cores_enabled.attr
 		|| attr == &dev_attr_cores_max.attr)
 		ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_CORES_SET);
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index efe608861e55..71833bd60f27 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -135,6 +135,9 @@
 /* dgpu on/off */
 #define ASUS_WMI_DEVID_DGPU		0x00090020
 
+#define ASUS_WMI_DEVID_DGPU_BASE_TGP	0x00120099
+#define ASUS_WMI_DEVID_DGPU_SET_TGP	0x00120098
+
 /* gpu mux switch, 0 = dGPU, 1 = Optimus */
 #define ASUS_WMI_DEVID_GPU_MUX		0x00090016
 #define ASUS_WMI_DEVID_GPU_MUX_VIVO	0x00090026
-- 
2.45.1