[PATCH v2 0/2] platform/x86: asus-armoury: reorganize visibility and extend dgpu_disable

[PATCH v2 0/2] platform/x86: asus-armoury: reorganize visibility and extend dgpu_disable

[Denis Benato]

Hi all,

This patchset reorganizes the visibility of attribute groups in the Asus
Armoury driver and extends the dGPU disable functionality with a fallback
DEVID for the ProArt H7606 series.

This good idea comes from Ilpo and this patchset is the result of our
productive exchange (link below).

The original author of the patch has stated that he/she wants to remain
anonymous, and asked me to respect that, therefore I will only reuse
the DEVID information from that patch and the commit text as everything
else has become irrelevant anyway after the reorganization of the driver.

Link: https://lore.kernel.org/all/20260810-asus_armoury_dgpu_new_devid-v1-1-0a5c845414e4@gmail.com/

Changelog:

Link v1: https://lore.kernel.org/all/20260916154210.181441-1-denis.benato@linux.dev/

- v1
  - platform/x86: asus-armoury: let attribute groups decide their own visibility
    - simplify the .c source of the driver making extensive use of .is_visible

Cc: busybox11 <busybox11th@gmail.com>

Denis Benato (2):
  platform/x86: asus-armoury: let attribute groups decide their own
    visibility
  platform/x86: asus-armoury: add dGPU disable fallback DEVID for ProArt
    H7606 series

 drivers/platform/x86/asus-armoury.c        | 282 +++++++++------------
 drivers/platform/x86/asus-armoury.h        | 124 +++++++--
 include/linux/platform_data/x86/asus-wmi.h |   3 +
 3 files changed, 218 insertions(+), 191 deletions(-)

-- 
2.47.3

[PATCH v2 1/2] platform/x86: asus-armoury: let attribute groups decide their own visibility

[Denis Benato]

mini_led_mode, gpu_mux_mode and dgpu_disable are created by ad-hoc
conditional blocks in asus_fw_attr_add() which must be manually tracked
by the error and exit paths; those paths also re-probe WMI to know which
groups were actually created.

Give every attribute group its own .is_visible() callback: all groups
are created unconditionally through the common loop and sysfs hides the
unsupported ones entirely. The plain group macros now gate their
visibility on the WMI presence of their device, the
ASUS_ATTR_GROUP_BOOL() and ASUS_ATTR_GROUP_ENUM() macros declare groups
backed by a device ID resolved at probe time, and the power tunable
macros are additionally gated on the platform limits defining a max
value, replacing the special-casing in asus_fw_attr_add() and with it
is_power_tunable_attr().

The dgpu_disable attribute now goes through the same resolved device ID
scheme as mini_led_mode and gpu_mux_mode, storing the device ID to use
in asus_armoury.dgpu_disable_dev_id instead of always operating on
ASUS_WMI_DEVID_DGPU.

No functional change is intended.

Signed-off-by: Denis Benato <denis.benato@linux.dev>
---
 drivers/platform/x86/asus-armoury.c | 280 +++++++++++-----------------
 drivers/platform/x86/asus-armoury.h | 124 +++++++++---
 2 files changed, 213 insertions(+), 191 deletions(-)

diff --git a/drivers/platform/x86/asus-armoury.c b/drivers/platform/x86/asus-armoury.c
index 2d5ca75bc727..7812e93734b8 100644
--- a/drivers/platform/x86/asus-armoury.c
+++ b/drivers/platform/x86/asus-armoury.c
@@ -94,6 +94,7 @@ struct asus_armoury_priv {
 
 	u32 mini_led_dev_id;
 	u32 gpu_mux_dev_id;
+	u32 dgpu_disable_dev_id;
 
 	bool requires_fan_curve;
 };
@@ -110,11 +111,6 @@ static struct fw_attrs_group fw_attrs = {
 	.pending_reboot = false,
 };
 
-struct asus_attr_group {
-	const struct attribute_group *attr_group;
-	u32 wmi_devid;
-};
-
 static void asus_set_reboot_and_signal_event(void)
 {
 	fw_attrs.pending_reboot = true;
@@ -458,6 +454,12 @@ static ssize_t mini_led_mode_possible_values_show(struct kobject *kobj,
 		return -ENODEV;
 	}
 }
+
+static bool mini_led_mode_group_visible(struct kobject *kobj)
+{
+	return asus_armoury.mini_led_dev_id;
+}
+
 ASUS_ATTR_GROUP_ENUM(mini_led_mode, "mini_led_mode", "Set the mini-LED backlight mode");
 
 static ssize_t gpu_mux_mode_current_value_store(struct kobject *kobj,
@@ -471,8 +473,8 @@ static ssize_t gpu_mux_mode_current_value_store(struct kobject *kobj,
 	if (err)
 		return err;
 
-	if (armoury_has_devstate(ASUS_WMI_DEVID_DGPU)) {
-		err = armoury_get_devstate(NULL, &result, ASUS_WMI_DEVID_DGPU);
+	if (asus_armoury.dgpu_disable_dev_id) {
+		err = armoury_get_devstate(NULL, &result, asus_armoury.dgpu_disable_dev_id);
 		if (err)
 			return err;
 		if (result && !optimus) {
@@ -502,6 +504,12 @@ static ssize_t gpu_mux_mode_current_value_store(struct kobject *kobj,
 	return count;
 }
 ASUS_WMI_SHOW_INT(gpu_mux_mode_current_value, asus_armoury.gpu_mux_dev_id);
+
+static bool gpu_mux_mode_group_visible(struct kobject *kobj)
+{
+	return asus_armoury.gpu_mux_dev_id;
+}
+
 ASUS_ATTR_GROUP_BOOL(gpu_mux_mode, "gpu_mux_mode", "Set the GPU display MUX mode");
 
 static ssize_t dgpu_disable_current_value_store(struct kobject *kobj,
@@ -538,7 +546,8 @@ static ssize_t dgpu_disable_current_value_store(struct kobject *kobj,
 	}
 
 	scoped_guard(mutex, &asus_armoury.egpu_mutex) {
-		err = armoury_set_devstate(attr, disable ? 1 : 0, NULL, ASUS_WMI_DEVID_DGPU);
+		err = armoury_set_devstate(attr, disable ? 1 : 0, NULL,
+					   asus_armoury.dgpu_disable_dev_id);
 		if (err)
 			return err;
 	}
@@ -547,7 +556,13 @@ static ssize_t dgpu_disable_current_value_store(struct kobject *kobj,
 
 	return count;
 }
-ASUS_WMI_SHOW_INT(dgpu_disable_current_value, ASUS_WMI_DEVID_DGPU);
+
+static bool dgpu_disable_group_visible(struct kobject *kobj)
+{
+	return asus_armoury.dgpu_disable_dev_id;
+}
+
+ASUS_WMI_SHOW_INT(dgpu_disable_current_value, asus_armoury.dgpu_disable_dev_id);
 ASUS_ATTR_GROUP_BOOL(dgpu_disable, "dgpu_disable", "Disable the dGPU");
 
 /* Values map for eGPU activation requests. */
@@ -695,6 +710,12 @@ static ssize_t egpu_enable_possible_values_show(struct kobject *kobj, struct kob
 {
 	return armoury_attr_enum_list(buf, ARRAY_SIZE(egpu_status_map));
 }
+
+static bool egpu_enable_group_visible(struct kobject *kobj)
+{
+	return armoury_has_devstate(ASUS_WMI_DEVID_EGPU);
+}
+
 ASUS_ATTR_GROUP_ENUM(egpu_enable, "egpu_enable", "Enable the eGPU (also disables dGPU)");
 
 /* Device memory available to APU */
@@ -771,6 +792,12 @@ static ssize_t apu_mem_possible_values_show(struct kobject *kobj, struct kobj_at
 {
 	return armoury_attr_enum_list(buf, ARRAY_SIZE(apu_mem_map));
 }
+
+static bool apu_mem_group_visible(struct kobject *kobj)
+{
+	return armoury_has_devstate(ASUS_WMI_DEVID_APU_MEM);
+}
+
 ASUS_ATTR_GROUP_ENUM(apu_mem, "apu_mem", "Set available system RAM (in GB) for the APU to use");
 
 /* Define helper to access the current power mode tunable values */
@@ -782,6 +809,44 @@ static inline struct rog_tunables *get_current_tunables(void)
 	return asus_armoury.rog_tunables[ASUS_ROG_TUNABLE_DC];
 }
 
+/**
+ * has_valid_limit - Checks if a power-related attribute has a valid limit value
+ * @name: The name of the attribute to check
+ * @limits: Pointer to the power_limits structure containing limit values
+ *
+ * This function checks if a power-related attribute has a valid limit value.
+ * It returns false if limits is NULL or if the corresponding limit value is zero.
+ *
+ * Return: true if the attribute has a valid limit value, false otherwise
+ */
+static bool has_valid_limit(const char *name, const struct power_limits *limits)
+{
+	u32 limit_value = 0;
+
+	if (!limits)
+		return false;
+
+	if (!strcmp(name, ATTR_PPT_PL1_SPL))
+		limit_value = limits->ppt_pl1_spl_max;
+	else if (!strcmp(name, ATTR_PPT_PL2_SPPT))
+		limit_value = limits->ppt_pl2_sppt_max;
+	else if (!strcmp(name, ATTR_PPT_PL3_FPPT))
+		limit_value = limits->ppt_pl3_fppt_max;
+	else if (!strcmp(name, ATTR_PPT_APU_SPPT))
+		limit_value = limits->ppt_apu_sppt_max;
+	else if (!strcmp(name, ATTR_PPT_PLATFORM_SPPT))
+		limit_value = limits->ppt_platform_sppt_max;
+	else if (!strcmp(name, ATTR_NV_DYNAMIC_BOOST))
+		limit_value = limits->nv_dynamic_boost_max;
+	else if (!strcmp(name, ATTR_NV_TEMP_TARGET))
+		limit_value = limits->nv_temp_target_max;
+	else if (!strcmp(name, ATTR_NV_BASE_TGP) ||
+		 !strcmp(name, ATTR_NV_TGP))
+		limit_value = limits->nv_tgp_max;
+
+	return limit_value > 0;
+}
+
 /* Simple attribute creation */
 ASUS_ATTR_GROUP_ENUM_INT_RO(charge_mode, "charge_mode", ASUS_WMI_DEVID_CHARGE_MODE, "0;1;2\n",
 			    "Show the current mode of charging");
@@ -819,103 +884,35 @@ ASUS_ATTR_GROUP_ROG_TUNABLE(nv_tgp, "nv_tgp", ASUS_WMI_DEVID_DGPU_SET_TGP,
 ASUS_ATTR_GROUP_INT_VALUE_ONLY_RO(nv_base_tgp, ATTR_NV_BASE_TGP, ASUS_WMI_DEVID_DGPU_BASE_TGP,
 				  "Read the base TGP value");
 
-/* If an attribute does not require any special case handling add it here */
-static const struct asus_attr_group armoury_attr_groups[] = {
-	{ &egpu_connected_attr_group, ASUS_WMI_DEVID_EGPU_CONNECTED },
-	{ &egpu_enable_attr_group, ASUS_WMI_DEVID_EGPU },
-	{ &dgpu_disable_attr_group, ASUS_WMI_DEVID_DGPU },
-	{ &dgpu_power_state_attr_group, ASUS_WMI_DEVID_DGPU_POWER_STATE },
-	{ &apu_mem_attr_group, ASUS_WMI_DEVID_APU_MEM },
-
-	{ &ppt_pl1_spl_attr_group, ASUS_WMI_DEVID_PPT_PL1_SPL },
-	{ &ppt_pl2_sppt_attr_group, ASUS_WMI_DEVID_PPT_PL2_SPPT },
-	{ &ppt_pl3_fppt_attr_group, ASUS_WMI_DEVID_PPT_PL3_FPPT },
-	{ &ppt_apu_sppt_attr_group, ASUS_WMI_DEVID_PPT_APU_SPPT },
-	{ &ppt_platform_sppt_attr_group, ASUS_WMI_DEVID_PPT_PLAT_SPPT },
-	{ &nv_dynamic_boost_attr_group, ASUS_WMI_DEVID_NV_DYN_BOOST },
-	{ &nv_temp_target_attr_group, ASUS_WMI_DEVID_NV_THERM_TARGET },
-	{ &nv_base_tgp_attr_group, ASUS_WMI_DEVID_DGPU_BASE_TGP },
-	{ &nv_tgp_attr_group, ASUS_WMI_DEVID_DGPU_SET_TGP },
-
-	{ &charge_mode_attr_group, ASUS_WMI_DEVID_CHARGE_MODE },
-	{ &boot_sound_attr_group, ASUS_WMI_DEVID_BOOT_SOUND },
-	{ &mcu_powersave_attr_group, ASUS_WMI_DEVID_MCU_POWERSAVE },
-	{ &panel_od_attr_group, ASUS_WMI_DEVID_PANEL_OD },
-	{ &panel_hd_mode_attr_group, ASUS_WMI_DEVID_PANEL_HD },
-	{ &screen_auto_brightness_attr_group, ASUS_WMI_DEVID_SCREEN_AUTO_BRIGHTNESS },
+static const struct attribute_group *armoury_attr_groups[] = {
+	&mini_led_mode_attr_group,
+	&gpu_mux_mode_attr_group,
+	&egpu_connected_attr_group,
+	&egpu_enable_attr_group,
+	&dgpu_disable_attr_group,
+	&dgpu_power_state_attr_group,
+	&apu_mem_attr_group,
+
+	&ppt_pl1_spl_attr_group,
+	&ppt_pl2_sppt_attr_group,
+	&ppt_pl3_fppt_attr_group,
+	&ppt_apu_sppt_attr_group,
+	&ppt_platform_sppt_attr_group,
+	&nv_dynamic_boost_attr_group,
+	&nv_temp_target_attr_group,
+	&nv_base_tgp_attr_group,
+	&nv_tgp_attr_group,
+
+	&charge_mode_attr_group,
+	&boot_sound_attr_group,
+	&mcu_powersave_attr_group,
+	&panel_od_attr_group,
+	&panel_hd_mode_attr_group,
+	&screen_auto_brightness_attr_group,
 };
 
-/**
- * is_power_tunable_attr - Determines if an attribute is a power-related tunable
- * @name: The name of the attribute to check
- *
- * This function checks if the given attribute name is related to power tuning.
- *
- * Return: true if the attribute is a power-related tunable, false otherwise
- */
-static bool is_power_tunable_attr(const char *name)
-{
-	static const char * const power_tunable_attrs[] = {
-		ATTR_PPT_PL1_SPL,	ATTR_PPT_PL2_SPPT,
-		ATTR_PPT_PL3_FPPT,	ATTR_PPT_APU_SPPT,
-		ATTR_PPT_PLATFORM_SPPT, ATTR_NV_DYNAMIC_BOOST,
-		ATTR_NV_TEMP_TARGET,	ATTR_NV_BASE_TGP,
-		ATTR_NV_TGP
-	};
-
-	for (unsigned int i = 0; i < ARRAY_SIZE(power_tunable_attrs); i++) {
-		if (!strcmp(name, power_tunable_attrs[i]))
-			return true;
-	}
-
-	return false;
-}
-
-/**
- * has_valid_limit - Checks if a power-related attribute has a valid limit value
- * @name: The name of the attribute to check
- * @limits: Pointer to the power_limits structure containing limit values
- *
- * This function checks if a power-related attribute has a valid limit value.
- * It returns false if limits is NULL or if the corresponding limit value is zero.
- *
- * Return: true if the attribute has a valid limit value, false otherwise
- */
-static bool has_valid_limit(const char *name, const struct power_limits *limits)
-{
-	u32 limit_value = 0;
-
-	if (!limits)
-		return false;
-
-	if (!strcmp(name, ATTR_PPT_PL1_SPL))
-		limit_value = limits->ppt_pl1_spl_max;
-	else if (!strcmp(name, ATTR_PPT_PL2_SPPT))
-		limit_value = limits->ppt_pl2_sppt_max;
-	else if (!strcmp(name, ATTR_PPT_PL3_FPPT))
-		limit_value = limits->ppt_pl3_fppt_max;
-	else if (!strcmp(name, ATTR_PPT_APU_SPPT))
-		limit_value = limits->ppt_apu_sppt_max;
-	else if (!strcmp(name, ATTR_PPT_PLATFORM_SPPT))
-		limit_value = limits->ppt_platform_sppt_max;
-	else if (!strcmp(name, ATTR_NV_DYNAMIC_BOOST))
-		limit_value = limits->nv_dynamic_boost_max;
-	else if (!strcmp(name, ATTR_NV_TEMP_TARGET))
-		limit_value = limits->nv_temp_target_max;
-	else if (!strcmp(name, ATTR_NV_BASE_TGP) ||
-		 !strcmp(name, ATTR_NV_TGP))
-		limit_value = limits->nv_tgp_max;
-
-	return limit_value > 0;
-}
-
 static int asus_fw_attr_add(void)
 {
-	const struct rog_tunables *const ac_rog_tunables =
-		asus_armoury.rog_tunables[ASUS_ROG_TUNABLE_AC];
-	const struct power_limits *limits;
-	bool should_create;
-	const char *name;
 	int err, i;
 
 	asus_armoury.fw_attr_dev = device_create(&firmware_attributes_class, NULL, MKDEV(0, 0),
@@ -944,73 +941,32 @@ static int asus_fw_attr_add(void)
 	else if (armoury_has_devstate(ASUS_WMI_DEVID_MINI_LED_MODE2))
 		asus_armoury.mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE2;
 
-	if (asus_armoury.mini_led_dev_id) {
-		err = sysfs_create_group(&asus_armoury.fw_attr_kset->kobj,
-					 &mini_led_mode_attr_group);
-		if (err) {
-			pr_err("Failed to create sysfs-group for mini_led\n");
-			goto err_remove_file;
-		}
-	}
-
 	asus_armoury.gpu_mux_dev_id = 0;
 	if (armoury_has_devstate(ASUS_WMI_DEVID_GPU_MUX))
 		asus_armoury.gpu_mux_dev_id = ASUS_WMI_DEVID_GPU_MUX;
 	else if (armoury_has_devstate(ASUS_WMI_DEVID_GPU_MUX_VIVO))
 		asus_armoury.gpu_mux_dev_id = ASUS_WMI_DEVID_GPU_MUX_VIVO;
 
-	if (asus_armoury.gpu_mux_dev_id) {
-		err = sysfs_create_group(&asus_armoury.fw_attr_kset->kobj,
-					 &gpu_mux_mode_attr_group);
-		if (err) {
-			pr_err("Failed to create sysfs-group for gpu_mux\n");
-			goto err_remove_mini_led_group;
-		}
-	}
+	asus_armoury.dgpu_disable_dev_id = 0;
+	if (armoury_has_devstate(ASUS_WMI_DEVID_DGPU))
+		asus_armoury.dgpu_disable_dev_id = ASUS_WMI_DEVID_DGPU;
 
 	for (i = 0; i < ARRAY_SIZE(armoury_attr_groups); i++) {
-		if (!armoury_has_devstate(armoury_attr_groups[i].wmi_devid))
-			continue;
-
-		/* Always create by default, unless PPT is not present */
-		should_create = true;
-		name = armoury_attr_groups[i].attr_group->name;
-
-		/* Check if this is a power-related tunable requiring limits */
-		if (ac_rog_tunables && ac_rog_tunables->power_limits &&
-		    is_power_tunable_attr(name)) {
-			limits = ac_rog_tunables->power_limits;
-			/* Check only AC: if not present then DC won't be either */
-			should_create = has_valid_limit(name, limits);
-			if (!should_create)
-				pr_debug("Missing max value for tunable %s\n", name);
-		}
-
-		if (should_create) {
-			err = sysfs_create_group(&asus_armoury.fw_attr_kset->kobj,
-						 armoury_attr_groups[i].attr_group);
-			if (err) {
-				pr_err("Failed to create sysfs-group for %s\n",
-				       armoury_attr_groups[i].attr_group->name);
-				goto err_remove_groups;
-			}
+		err = sysfs_create_group(&asus_armoury.fw_attr_kset->kobj,
+					 armoury_attr_groups[i]);
+		if (err) {
+			pr_err("Failed to create sysfs-group for %s\n",
+			       armoury_attr_groups[i]->name);
+			goto err_remove_groups;
 		}
 	}
 
 	return 0;
 
 err_remove_groups:
-	while (i--) {
-		if (armoury_has_devstate(armoury_attr_groups[i].wmi_devid))
-			sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj,
-					   armoury_attr_groups[i].attr_group);
-	}
-	if (asus_armoury.gpu_mux_dev_id)
-		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj, &gpu_mux_mode_attr_group);
-err_remove_mini_led_group:
-	if (asus_armoury.mini_led_dev_id)
-		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj, &mini_led_mode_attr_group);
-err_remove_file:
+	while (i--)
+		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj,
+				   armoury_attr_groups[i]);
 	sysfs_remove_file(&asus_armoury.fw_attr_kset->kobj, &pending_reboot.attr);
 err_destroy_kset:
 	kset_unregister(asus_armoury.fw_attr_kset);
@@ -1182,17 +1138,9 @@ static void __exit asus_fw_exit(void)
 {
 	int i;
 
-	for (i = ARRAY_SIZE(armoury_attr_groups) - 1; i >= 0; i--) {
-		if (armoury_has_devstate(armoury_attr_groups[i].wmi_devid))
-			sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj,
-					   armoury_attr_groups[i].attr_group);
-	}
-
-	if (asus_armoury.gpu_mux_dev_id)
-		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj, &gpu_mux_mode_attr_group);
-
-	if (asus_armoury.mini_led_dev_id)
-		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj, &mini_led_mode_attr_group);
+	for (i = ARRAY_SIZE(armoury_attr_groups) - 1; i >= 0; i--)
+		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj,
+				   armoury_attr_groups[i]);
 
 	sysfs_remove_file(&asus_armoury.fw_attr_kset->kobj, &pending_reboot.attr);
 	kset_unregister(asus_armoury.fw_attr_kset);
diff --git a/drivers/platform/x86/asus-armoury.h b/drivers/platform/x86/asus-armoury.h
index d605af2fdaa1..38509a086a02 100644
--- a/drivers/platform/x86/asus-armoury.h
+++ b/drivers/platform/x86/asus-armoury.h
@@ -100,6 +100,43 @@ ssize_t armoury_attr_uint_show(struct kobject *kobj, struct kobj_attribute *attr
 	static struct kobj_attribute attr_##_attrname##_##_prop =		\
 		__ASUS_ATTR_RO(_attrname, _prop)
 
+/*
+ * Every attribute group decides its own visibility through .is_visible():
+ * sysfs hides a named group entirely when its first attribute reports
+ * SYSFS_GROUP_INVISIBLE, so groups are always created and never leave
+ * empty directories behind.
+ */
+#define __ASUS_DEVSTATE_GROUP_VISIBLE(_attrname, _wmi)			\
+	static bool _attrname##_group_visible(struct kobject *kobj)	\
+	{								\
+		return armoury_has_devstate(_wmi);			\
+	}								\
+	DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(_attrname)
+
+/*
+ * Power tunables are additionally gated on the platform limits actually
+ * defining a max value for them. Only the AC limits are checked: if not
+ * present then DC won't be either.
+ */
+#define __ASUS_POWER_TUNABLE_GROUP_VISIBLE(_attrname, _fsname, _wmi)	\
+	static bool _attrname##_group_visible(struct kobject *kobj)	\
+	{								\
+		const struct rog_tunables *tunables =			\
+			asus_armoury.rog_tunables[ASUS_ROG_TUNABLE_AC];	\
+									\
+		if (!tunables || !tunables->power_limits)		\
+			return armoury_has_devstate(_wmi);		\
+									\
+		if (!has_valid_limit(_fsname, tunables->power_limits)) {\
+			pr_debug("Missing max value for tunable %s\n",	\
+				 _fsname);				\
+			return false;					\
+		}							\
+									\
+		return armoury_has_devstate(_wmi);			\
+	}								\
+	DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(_attrname)
+
 #define __ATTR_RO_INT_GROUP_ENUM(_attrname, _wmi, _fsname, _possible, _dispname)\
 	ASUS_WMI_SHOW_INT(_attrname##_current_value, _wmi);		\
 	static struct kobj_attribute attr_##_attrname##_current_value =		\
@@ -108,6 +145,7 @@ ssize_t armoury_attr_uint_show(struct kobject *kobj, struct kobj_attribute *attr
 	__ATTR_SHOW_FMT(possible_values, _attrname, "%s\n", _possible);		\
 	static struct kobj_attribute attr_##_attrname##_type =			\
 		__ASUS_ATTR_RO_AS(type, enum_type_show);			\
+	__ASUS_DEVSTATE_GROUP_VISIBLE(_attrname, _wmi);				\
 	static struct attribute *_attrname##_attrs[] = {			\
 		&attr_##_attrname##_current_value.attr,				\
 		&attr_##_attrname##_display_name.attr,				\
@@ -116,7 +154,9 @@ ssize_t armoury_attr_uint_show(struct kobject *kobj, struct kobj_attribute *attr
 		NULL								\
 	};									\
 	static const struct attribute_group _attrname##_attr_group = {		\
-		.name = _fsname, .attrs = _attrname##_attrs			\
+		.name = _fsname,						\
+		.is_visible = SYSFS_GROUP_VISIBLE(_attrname),			\
+		.attrs = _attrname##_attrs					\
 	}
 
 #define __ATTR_RW_INT_GROUP_ENUM(_attrname, _minv, _maxv, _wmi, _fsname,\
@@ -129,6 +169,7 @@ ssize_t armoury_attr_uint_show(struct kobject *kobj, struct kobj_attribute *attr
 	__ATTR_SHOW_FMT(possible_values, _attrname, "%s\n", _possible);	\
 	static struct kobj_attribute attr_##_attrname##_type =		\
 		__ASUS_ATTR_RO_AS(type, enum_type_show);		\
+	__ASUS_DEVSTATE_GROUP_VISIBLE(_attrname, _wmi);			\
 	static struct attribute *_attrname##_attrs[] = {		\
 		&attr_##_attrname##_current_value.attr,			\
 		&attr_##_attrname##_display_name.attr,			\
@@ -137,7 +178,9 @@ ssize_t armoury_attr_uint_show(struct kobject *kobj, struct kobj_attribute *attr
 		NULL							\
 	};								\
 	static const struct attribute_group _attrname##_attr_group = {	\
-		.name = _fsname, .attrs = _attrname##_attrs		\
+		.name = _fsname,					\
+		.is_visible = SYSFS_GROUP_VISIBLE(_attrname),		\
+		.attrs = _attrname##_attrs				\
 	}
 
 /* Boolean style enumeration, base macro. Requires adding show/store */
@@ -168,37 +211,63 @@ ssize_t armoury_attr_uint_show(struct kobject *kobj, struct kobj_attribute *attr
 	__ATTR_RO_INT_GROUP_ENUM(_attrname, _wmi, _fsname, _possible, _dispname)
 
 /*
- * Requires <name>_current_value_show(), <name>_current_value_show()
+ * Boolean style group whose whole visibility is decided by
+ * <name>_group_visible(), for attributes backed by a device ID resolved
+ * at probe time.
+ * Requires <name>_current_value_show(), <name>_current_value_store()
+ * and <name>_group_visible()
  */
 #define ASUS_ATTR_GROUP_BOOL(_attrname, _fsname, _dispname)		\
+	DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(_attrname)			\
 	static struct kobj_attribute attr_##_attrname##_current_value =	\
 		__ASUS_ATTR_RW(_attrname, current_value);		\
-	__ATTR_GROUP_ENUM(_attrname, _fsname, "0;1", _dispname)
+	__ATTR_SHOW_FMT(display_name, _attrname, "%s\n", _dispname);	\
+	__ATTR_SHOW_FMT(possible_values, _attrname, "%s\n", "0;1");	\
+	static struct kobj_attribute attr_##_attrname##_type =		\
+		__ASUS_ATTR_RO_AS(type, enum_type_show);		\
+	static struct attribute *_attrname##_attrs[] = {		\
+		&attr_##_attrname##_current_value.attr,			\
+		&attr_##_attrname##_display_name.attr,			\
+		&attr_##_attrname##_possible_values.attr,		\
+		&attr_##_attrname##_type.attr,				\
+		NULL							\
+	};								\
+	static const struct attribute_group _attrname##_attr_group = {	\
+		.name = _fsname,					\
+		.is_visible = SYSFS_GROUP_VISIBLE(_attrname),		\
+		.attrs = _attrname##_attrs				\
+	}
 
 /*
- * Requires <name>_current_value_show(), <name>_current_value_show()
- * and <name>_possible_values_show()
+ * Group whose whole visibility is decided by <name>_group_visible(),
+ * for attributes backed by a device ID resolved at probe time.
+ * Requires <name>_current_value_show(), <name>_current_value_store(),
+ * <name>_possible_values_show() and <name>_group_visible()
  */
-#define ASUS_ATTR_GROUP_ENUM(_attrname, _fsname, _dispname)			\
-	__ATTR_SHOW_FMT(display_name, _attrname, "%s\n", _dispname);		\
-	static struct kobj_attribute attr_##_attrname##_current_value =		\
-		__ASUS_ATTR_RW(_attrname, current_value);			\
-	static struct kobj_attribute attr_##_attrname##_possible_values =	\
-		__ASUS_ATTR_RO(_attrname, possible_values);			\
-	static struct kobj_attribute attr_##_attrname##_type =			\
-		__ASUS_ATTR_RO_AS(type, enum_type_show);			\
-	static struct attribute *_attrname##_attrs[] = {			\
-		&attr_##_attrname##_current_value.attr,				\
-		&attr_##_attrname##_display_name.attr,				\
-		&attr_##_attrname##_possible_values.attr,			\
-		&attr_##_attrname##_type.attr,					\
-		NULL								\
-	};									\
-	static const struct attribute_group _attrname##_attr_group = {		\
-		.name = _fsname, .attrs = _attrname##_attrs			\
+#define ASUS_ATTR_GROUP_ENUM(_attrname, _fsname, _dispname)		\
+	DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(_attrname)			\
+	static struct kobj_attribute attr_##_attrname##_current_value =	\
+		__ASUS_ATTR_RW(_attrname, current_value);		\
+	__ATTR_SHOW_FMT(display_name, _attrname, "%s\n", _dispname);	\
+	static struct kobj_attribute attr_##_attrname##_possible_values =\
+		__ASUS_ATTR_RO(_attrname, possible_values);		\
+	static struct kobj_attribute attr_##_attrname##_type =		\
+		__ASUS_ATTR_RO_AS(type, enum_type_show);		\
+	static struct attribute *_attrname##_attrs[] = {		\
+		&attr_##_attrname##_current_value.attr,			\
+		&attr_##_attrname##_display_name.attr,			\
+		&attr_##_attrname##_possible_values.attr,		\
+		&attr_##_attrname##_type.attr,				\
+		NULL							\
+	};								\
+	static const struct attribute_group _attrname##_attr_group = {	\
+		.name = _fsname,					\
+		.is_visible = SYSFS_GROUP_VISIBLE(_attrname),		\
+		.attrs = _attrname##_attrs				\
 	}
 
 #define ASUS_ATTR_GROUP_INT_VALUE_ONLY_RO(_attrname, _fsname, _wmi, _dispname)	\
+	__ASUS_POWER_TUNABLE_GROUP_VISIBLE(_attrname, _fsname, _wmi);		\
 	ASUS_WMI_SHOW_INT(_attrname##_current_value, _wmi);		\
 	static struct kobj_attribute attr_##_attrname##_current_value =		\
 		__ASUS_ATTR_RO(_attrname, current_value);			\
@@ -211,7 +280,9 @@ ssize_t armoury_attr_uint_show(struct kobject *kobj, struct kobj_attribute *attr
 		&attr_##_attrname##_type.attr, NULL				\
 	};									\
 	static const struct attribute_group _attrname##_attr_group = {		\
-		.name = _fsname, .attrs = _attrname##_attrs			\
+		.name = _fsname,						\
+		.is_visible = SYSFS_GROUP_VISIBLE(_attrname),			\
+		.attrs = _attrname##_attrs					\
 	}
 
 /*
@@ -284,6 +355,7 @@ ssize_t armoury_attr_uint_show(struct kobject *kobj, struct kobj_attribute *attr
 		__ASUS_ATTR_RW(_attr, current_value)
 
 #define ASUS_ATTR_GROUP_ROG_TUNABLE(_attrname, _fsname, _wmi, _dispname)	\
+	__ASUS_POWER_TUNABLE_GROUP_VISIBLE(_attrname, _fsname, _wmi);	\
 	__ROG_TUNABLE_RW(_attrname, _wmi);				\
 	__ROG_TUNABLE_SHOW_DEFAULT(_attrname);				\
 	__ROG_TUNABLE_SHOW(min_value, _attrname, _attrname##_min);	\
@@ -303,7 +375,9 @@ ssize_t armoury_attr_uint_show(struct kobject *kobj, struct kobj_attribute *attr
 		NULL							\
 	};								\
 	static const struct attribute_group _attrname##_attr_group = {	\
-		.name = _fsname, .attrs = _attrname##_attrs		\
+		.name = _fsname,					\
+		.is_visible = SYSFS_GROUP_VISIBLE(_attrname),		\
+		.attrs = _attrname##_attrs				\
 	}
 
 /* Default is always the maximum value unless *_def is specified */
-- 
2.47.3

[PATCH v2 2/2] platform/x86: asus-armoury: add dGPU disable fallback DEVID for ProArt H7606 series

[Denis Benato]

Newer ASUS ProArt laptops (e.g. H7606 series) implement dGPU power
control on WMI DEVID 0x00090120 instead of the usual 0x00090020.
The default DEVID returns 0xFFFFFFE2 on these models, so the
dgpu_disable firmware attribute is never created and the dGPU cannot
be re-enabled from Linux at all.

Add a fallback probe for device ID 0x00090120 following the same
approach as the gpu_mux_dev_id probe.

Details for 0x00090120 (confirmed on H7606W using direct WMNB calls
and inspecting the DSDT DEVS handler):
  - Reading DSTS returns 0x00010001 when the dGPU is off (CUMA=1),
	or 0x00010000 when it's on.
  - Writing DEVS: 0 enables the dGPU (calls PG00._ON() + Notifies
	PEGP, Device Check), and 1 disables or ejects it.

This behavior matches the dgpu_disable attribute (1 = disabled).

Signed-off-by: Denis Benato <denis.benato@linux.dev>
---
 drivers/platform/x86/asus-armoury.c        | 2 ++
 include/linux/platform_data/x86/asus-wmi.h | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/drivers/platform/x86/asus-armoury.c b/drivers/platform/x86/asus-armoury.c
index 7812e93734b8..7c54f616d701 100644
--- a/drivers/platform/x86/asus-armoury.c
+++ b/drivers/platform/x86/asus-armoury.c
@@ -950,6 +950,8 @@ static int asus_fw_attr_add(void)
 	asus_armoury.dgpu_disable_dev_id = 0;
 	if (armoury_has_devstate(ASUS_WMI_DEVID_DGPU))
 		asus_armoury.dgpu_disable_dev_id = ASUS_WMI_DEVID_DGPU;
+	else if (armoury_has_devstate(ASUS_WMI_DEVID_GPU_MODE))
+		asus_armoury.dgpu_disable_dev_id = ASUS_WMI_DEVID_GPU_MODE;
 
 	for (i = 0; i < ARRAY_SIZE(armoury_attr_groups); i++) {
 		err = sysfs_create_group(&asus_armoury.fw_attr_kset->kobj,
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index be4d2873ffc9..4ea1ccc1d0ee 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -139,6 +139,9 @@
 /* dgpu on/off */
 #define ASUS_WMI_DEVID_DGPU		0x00090020
 
+/* dgpu on/off - alternative to ASUS_WMI_DEVID_DGPU */
+#define ASUS_WMI_DEVID_GPU_MODE		0x00090120
+
 #define ASUS_WMI_DEVID_APU_MEM		0x000600C1
 
 #define ASUS_WMI_DEVID_DGPU_POWER_STATE	0x00120097
-- 
2.47.3

Re: [PATCH v2 1/2] platform/x86: asus-armoury: let attribute groups decide their own visibility

[Ilpo Järvinen]

On Wed, 23 Sep 2026, Denis Benato wrote:

> mini_led_mode, gpu_mux_mode and dgpu_disable are created by ad-hoc
> conditional blocks in asus_fw_attr_add() which must be manually tracked
> by the error and exit paths; those paths also re-probe WMI to know which
> groups were actually created.
> 
> Give every attribute group its own .is_visible() callback: all groups
> are created unconditionally through the common loop and sysfs hides the
> unsupported ones entirely. The plain group macros now gate their
> visibility on the WMI presence of their device, the
> ASUS_ATTR_GROUP_BOOL() and ASUS_ATTR_GROUP_ENUM() macros declare groups
> backed by a device ID resolved at probe time, and the power tunable
> macros are additionally gated on the platform limits defining a max
> value, replacing the special-casing in asus_fw_attr_add() and with it
> is_power_tunable_attr().
> 
> The dgpu_disable attribute now goes through the same resolved device ID
> scheme as mini_led_mode and gpu_mux_mode, storing the device ID to use
> in asus_armoury.dgpu_disable_dev_id instead of always operating on
> ASUS_WMI_DEVID_DGPU.
> 
> No functional change is intended.
> 
> Signed-off-by: Denis Benato <denis.benato@linux.dev>
> ---
>  drivers/platform/x86/asus-armoury.c | 280 +++++++++++-----------------
>  drivers/platform/x86/asus-armoury.h | 124 +++++++++---
>  2 files changed, 213 insertions(+), 191 deletions(-)
> 
> diff --git a/drivers/platform/x86/asus-armoury.c b/drivers/platform/x86/asus-armoury.c
> index 2d5ca75bc727..7812e93734b8 100644
> --- a/drivers/platform/x86/asus-armoury.c
> +++ b/drivers/platform/x86/asus-armoury.c
> @@ -94,6 +94,7 @@ struct asus_armoury_priv {
>  
>  	u32 mini_led_dev_id;
>  	u32 gpu_mux_dev_id;
> +	u32 dgpu_disable_dev_id;
>  
>  	bool requires_fan_curve;
>  };
> @@ -110,11 +111,6 @@ static struct fw_attrs_group fw_attrs = {
>  	.pending_reboot = false,
>  };
>  
> -struct asus_attr_group {
> -	const struct attribute_group *attr_group;
> -	u32 wmi_devid;
> -};
> -
>  static void asus_set_reboot_and_signal_event(void)
>  {
>  	fw_attrs.pending_reboot = true;
> @@ -458,6 +454,12 @@ static ssize_t mini_led_mode_possible_values_show(struct kobject *kobj,
>  		return -ENODEV;
>  	}
>  }
> +
> +static bool mini_led_mode_group_visible(struct kobject *kobj)
> +{
> +	return asus_armoury.mini_led_dev_id;
> +}
> +
>  ASUS_ATTR_GROUP_ENUM(mini_led_mode, "mini_led_mode", "Set the mini-LED backlight mode");
>  
>  static ssize_t gpu_mux_mode_current_value_store(struct kobject *kobj,
> @@ -471,8 +473,8 @@ static ssize_t gpu_mux_mode_current_value_store(struct kobject *kobj,
>  	if (err)
>  		return err;
>  
> -	if (armoury_has_devstate(ASUS_WMI_DEVID_DGPU)) {
> -		err = armoury_get_devstate(NULL, &result, ASUS_WMI_DEVID_DGPU);
> +	if (asus_armoury.dgpu_disable_dev_id) {
> +		err = armoury_get_devstate(NULL, &result, asus_armoury.dgpu_disable_dev_id);
>  		if (err)
>  			return err;
>  		if (result && !optimus) {
> @@ -502,6 +504,12 @@ static ssize_t gpu_mux_mode_current_value_store(struct kobject *kobj,
>  	return count;
>  }
>  ASUS_WMI_SHOW_INT(gpu_mux_mode_current_value, asus_armoury.gpu_mux_dev_id);
> +
> +static bool gpu_mux_mode_group_visible(struct kobject *kobj)
> +{
> +	return asus_armoury.gpu_mux_dev_id;
> +}
> +
>  ASUS_ATTR_GROUP_BOOL(gpu_mux_mode, "gpu_mux_mode", "Set the GPU display MUX mode");
>  
>  static ssize_t dgpu_disable_current_value_store(struct kobject *kobj,
> @@ -538,7 +546,8 @@ static ssize_t dgpu_disable_current_value_store(struct kobject *kobj,
>  	}
>  
>  	scoped_guard(mutex, &asus_armoury.egpu_mutex) {
> -		err = armoury_set_devstate(attr, disable ? 1 : 0, NULL, ASUS_WMI_DEVID_DGPU);
> +		err = armoury_set_devstate(attr, disable ? 1 : 0, NULL,
> +					   asus_armoury.dgpu_disable_dev_id);
>  		if (err)
>  			return err;
>  	}
> @@ -547,7 +556,13 @@ static ssize_t dgpu_disable_current_value_store(struct kobject *kobj,
>  
>  	return count;
>  }
> -ASUS_WMI_SHOW_INT(dgpu_disable_current_value, ASUS_WMI_DEVID_DGPU);
> +
> +static bool dgpu_disable_group_visible(struct kobject *kobj)
> +{
> +	return asus_armoury.dgpu_disable_dev_id;
> +}
> +
> +ASUS_WMI_SHOW_INT(dgpu_disable_current_value, asus_armoury.dgpu_disable_dev_id);
>  ASUS_ATTR_GROUP_BOOL(dgpu_disable, "dgpu_disable", "Disable the dGPU");
>  
>  /* Values map for eGPU activation requests. */
> @@ -695,6 +710,12 @@ static ssize_t egpu_enable_possible_values_show(struct kobject *kobj, struct kob
>  {
>  	return armoury_attr_enum_list(buf, ARRAY_SIZE(egpu_status_map));
>  }
> +
> +static bool egpu_enable_group_visible(struct kobject *kobj)
> +{
> +	return armoury_has_devstate(ASUS_WMI_DEVID_EGPU);
> +}
> +
>  ASUS_ATTR_GROUP_ENUM(egpu_enable, "egpu_enable", "Enable the eGPU (also disables dGPU)");
>  
>  /* Device memory available to APU */
> @@ -771,6 +792,12 @@ static ssize_t apu_mem_possible_values_show(struct kobject *kobj, struct kobj_at
>  {
>  	return armoury_attr_enum_list(buf, ARRAY_SIZE(apu_mem_map));
>  }
> +
> +static bool apu_mem_group_visible(struct kobject *kobj)
> +{
> +	return armoury_has_devstate(ASUS_WMI_DEVID_APU_MEM);
> +}
> +
>  ASUS_ATTR_GROUP_ENUM(apu_mem, "apu_mem", "Set available system RAM (in GB) for the APU to use");
>  
>  /* Define helper to access the current power mode tunable values */
> @@ -782,6 +809,44 @@ static inline struct rog_tunables *get_current_tunables(void)
>  	return asus_armoury.rog_tunables[ASUS_ROG_TUNABLE_DC];
>  }
>  
> +/**
> + * has_valid_limit - Checks if a power-related attribute has a valid limit value
> + * @name: The name of the attribute to check
> + * @limits: Pointer to the power_limits structure containing limit values
> + *
> + * This function checks if a power-related attribute has a valid limit value.
> + * It returns false if limits is NULL or if the corresponding limit value is zero.
> + *
> + * Return: true if the attribute has a valid limit value, false otherwise
> + */
> +static bool has_valid_limit(const char *name, const struct power_limits *limits)
> +{
> +	u32 limit_value = 0;
> +
> +	if (!limits)
> +		return false;
> +
> +	if (!strcmp(name, ATTR_PPT_PL1_SPL))
> +		limit_value = limits->ppt_pl1_spl_max;
> +	else if (!strcmp(name, ATTR_PPT_PL2_SPPT))
> +		limit_value = limits->ppt_pl2_sppt_max;
> +	else if (!strcmp(name, ATTR_PPT_PL3_FPPT))
> +		limit_value = limits->ppt_pl3_fppt_max;
> +	else if (!strcmp(name, ATTR_PPT_APU_SPPT))
> +		limit_value = limits->ppt_apu_sppt_max;
> +	else if (!strcmp(name, ATTR_PPT_PLATFORM_SPPT))
> +		limit_value = limits->ppt_platform_sppt_max;
> +	else if (!strcmp(name, ATTR_NV_DYNAMIC_BOOST))
> +		limit_value = limits->nv_dynamic_boost_max;
> +	else if (!strcmp(name, ATTR_NV_TEMP_TARGET))
> +		limit_value = limits->nv_temp_target_max;
> +	else if (!strcmp(name, ATTR_NV_BASE_TGP) ||
> +		 !strcmp(name, ATTR_NV_TGP))
> +		limit_value = limits->nv_tgp_max;
> +
> +	return limit_value > 0;
> +}

This is a plain move, right? Can you move it in a preparatory patch to 
cut the extra churn from what is already a very complicated diff.

> +
>  /* Simple attribute creation */
>  ASUS_ATTR_GROUP_ENUM_INT_RO(charge_mode, "charge_mode", ASUS_WMI_DEVID_CHARGE_MODE, "0;1;2\n",
>  			    "Show the current mode of charging");
> @@ -819,103 +884,35 @@ ASUS_ATTR_GROUP_ROG_TUNABLE(nv_tgp, "nv_tgp", ASUS_WMI_DEVID_DGPU_SET_TGP,
>  ASUS_ATTR_GROUP_INT_VALUE_ONLY_RO(nv_base_tgp, ATTR_NV_BASE_TGP, ASUS_WMI_DEVID_DGPU_BASE_TGP,
>  				  "Read the base TGP value");
>  
> -/* If an attribute does not require any special case handling add it here */
> -static const struct asus_attr_group armoury_attr_groups[] = {
> -	{ &egpu_connected_attr_group, ASUS_WMI_DEVID_EGPU_CONNECTED },
> -	{ &egpu_enable_attr_group, ASUS_WMI_DEVID_EGPU },
> -	{ &dgpu_disable_attr_group, ASUS_WMI_DEVID_DGPU },
> -	{ &dgpu_power_state_attr_group, ASUS_WMI_DEVID_DGPU_POWER_STATE },
> -	{ &apu_mem_attr_group, ASUS_WMI_DEVID_APU_MEM },
> -
> -	{ &ppt_pl1_spl_attr_group, ASUS_WMI_DEVID_PPT_PL1_SPL },
> -	{ &ppt_pl2_sppt_attr_group, ASUS_WMI_DEVID_PPT_PL2_SPPT },
> -	{ &ppt_pl3_fppt_attr_group, ASUS_WMI_DEVID_PPT_PL3_FPPT },
> -	{ &ppt_apu_sppt_attr_group, ASUS_WMI_DEVID_PPT_APU_SPPT },
> -	{ &ppt_platform_sppt_attr_group, ASUS_WMI_DEVID_PPT_PLAT_SPPT },
> -	{ &nv_dynamic_boost_attr_group, ASUS_WMI_DEVID_NV_DYN_BOOST },
> -	{ &nv_temp_target_attr_group, ASUS_WMI_DEVID_NV_THERM_TARGET },
> -	{ &nv_base_tgp_attr_group, ASUS_WMI_DEVID_DGPU_BASE_TGP },
> -	{ &nv_tgp_attr_group, ASUS_WMI_DEVID_DGPU_SET_TGP },
> -
> -	{ &charge_mode_attr_group, ASUS_WMI_DEVID_CHARGE_MODE },
> -	{ &boot_sound_attr_group, ASUS_WMI_DEVID_BOOT_SOUND },
> -	{ &mcu_powersave_attr_group, ASUS_WMI_DEVID_MCU_POWERSAVE },
> -	{ &panel_od_attr_group, ASUS_WMI_DEVID_PANEL_OD },
> -	{ &panel_hd_mode_attr_group, ASUS_WMI_DEVID_PANEL_HD },
> -	{ &screen_auto_brightness_attr_group, ASUS_WMI_DEVID_SCREEN_AUTO_BRIGHTNESS },
> +static const struct attribute_group *armoury_attr_groups[] = {
> +	&mini_led_mode_attr_group,
> +	&gpu_mux_mode_attr_group,
> +	&egpu_connected_attr_group,
> +	&egpu_enable_attr_group,
> +	&dgpu_disable_attr_group,
> +	&dgpu_power_state_attr_group,
> +	&apu_mem_attr_group,
> +
> +	&ppt_pl1_spl_attr_group,
> +	&ppt_pl2_sppt_attr_group,
> +	&ppt_pl3_fppt_attr_group,
> +	&ppt_apu_sppt_attr_group,
> +	&ppt_platform_sppt_attr_group,
> +	&nv_dynamic_boost_attr_group,
> +	&nv_temp_target_attr_group,
> +	&nv_base_tgp_attr_group,
> +	&nv_tgp_attr_group,
> +
> +	&charge_mode_attr_group,
> +	&boot_sound_attr_group,
> +	&mcu_powersave_attr_group,
> +	&panel_od_attr_group,
> +	&panel_hd_mode_attr_group,
> +	&screen_auto_brightness_attr_group,
>  };

This looks much better.

Now can you also add the terminating NULL to the array and try to use 
sysfs_create/remove_groups() so you can eliminate the create, rollback, 
and remove loops... I suggest you do it on top of this patch as this 
change is already quite complicated and logically 
sysfs_create/remove_group() -> sysfs_create/remove_groups() is a separate 
transition.

>  	for (i = 0; i < ARRAY_SIZE(armoury_attr_groups); i++) {
> -		if (!armoury_has_devstate(armoury_attr_groups[i].wmi_devid))
> -			continue;
> -
> -		/* Always create by default, unless PPT is not present */
> -		should_create = true;
> -		name = armoury_attr_groups[i].attr_group->name;
> -
> -		/* Check if this is a power-related tunable requiring limits */
> -		if (ac_rog_tunables && ac_rog_tunables->power_limits &&
> -		    is_power_tunable_attr(name)) {
> -			limits = ac_rog_tunables->power_limits;
> -			/* Check only AC: if not present then DC won't be either */
> -			should_create = has_valid_limit(name, limits);
> -			if (!should_create)
> -				pr_debug("Missing max value for tunable %s\n", name);
> -		}
> -
> -		if (should_create) {
> -			err = sysfs_create_group(&asus_armoury.fw_attr_kset->kobj,
> -						 armoury_attr_groups[i].attr_group);
> -			if (err) {
> -				pr_err("Failed to create sysfs-group for %s\n",
> -				       armoury_attr_groups[i].attr_group->name);
> -				goto err_remove_groups;
> -			}
> +		err = sysfs_create_group(&asus_armoury.fw_attr_kset->kobj,
> +					 armoury_attr_groups[i]);
> +		if (err) {
> +			pr_err("Failed to create sysfs-group for %s\n",
> +			       armoury_attr_groups[i]->name);
> +			goto err_remove_groups;
>  		}
>  	}
>  
>  	return 0;
>  
>  err_remove_groups:
> -	while (i--) {
> -		if (armoury_has_devstate(armoury_attr_groups[i].wmi_devid))
> -			sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj,
> -					   armoury_attr_groups[i].attr_group);
> -	}
> -	if (asus_armoury.gpu_mux_dev_id)
> -		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj, &gpu_mux_mode_attr_group);
> -err_remove_mini_led_group:
> -	if (asus_armoury.mini_led_dev_id)
> -		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj, &mini_led_mode_attr_group);
> -err_remove_file:
> +	while (i--)
> +		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj,
> +				   armoury_attr_groups[i]);
>  	sysfs_remove_file(&asus_armoury.fw_attr_kset->kobj, &pending_reboot.attr);
>  err_destroy_kset:
>  	kset_unregister(asus_armoury.fw_attr_kset);
> @@ -1182,17 +1138,9 @@ static void __exit asus_fw_exit(void)
>  {
>  	int i;
>  
> -	for (i = ARRAY_SIZE(armoury_attr_groups) - 1; i >= 0; i--) {
> -		if (armoury_has_devstate(armoury_attr_groups[i].wmi_devid))
> -			sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj,
> -					   armoury_attr_groups[i].attr_group);
> -	}
> -
> -	if (asus_armoury.gpu_mux_dev_id)
> -		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj, &gpu_mux_mode_attr_group);
> -
> -	if (asus_armoury.mini_led_dev_id)
> -		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj, &mini_led_mode_attr_group);
> +	for (i = ARRAY_SIZE(armoury_attr_groups) - 1; i >= 0; i--)
> +		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj,
> +				   armoury_attr_groups[i]);

-- 
 i.

Re: [PATCH v2 1/2] platform/x86: asus-armoury: let attribute groups decide their own visibility

[Denis Benato]

On 9/23/26 11:52, Ilpo Järvinen wrote:
> On Wed, 23 Sep 2026, Denis Benato wrote:
>
>> mini_led_mode, gpu_mux_mode and dgpu_disable are created by ad-hoc
>> conditional blocks in asus_fw_attr_add() which must be manually tracked
>> by the error and exit paths; those paths also re-probe WMI to know which
>> groups were actually created.
>>
>> Give every attribute group its own .is_visible() callback: all groups
>> are created unconditionally through the common loop and sysfs hides the
>> unsupported ones entirely. The plain group macros now gate their
>> visibility on the WMI presence of their device, the
>> ASUS_ATTR_GROUP_BOOL() and ASUS_ATTR_GROUP_ENUM() macros declare groups
>> backed by a device ID resolved at probe time, and the power tunable
>> macros are additionally gated on the platform limits defining a max
>> value, replacing the special-casing in asus_fw_attr_add() and with it
>> is_power_tunable_attr().
>>
>> The dgpu_disable attribute now goes through the same resolved device ID
>> scheme as mini_led_mode and gpu_mux_mode, storing the device ID to use
>> in asus_armoury.dgpu_disable_dev_id instead of always operating on
>> ASUS_WMI_DEVID_DGPU.
>>
>> No functional change is intended.
>>
>> Signed-off-by: Denis Benato <denis.benato@linux.dev>
>> ---
>>  drivers/platform/x86/asus-armoury.c | 280 +++++++++++-----------------
>>  drivers/platform/x86/asus-armoury.h | 124 +++++++++---
>>  2 files changed, 213 insertions(+), 191 deletions(-)
>>
>> diff --git a/drivers/platform/x86/asus-armoury.c b/drivers/platform/x86/asus-armoury.c
>> index 2d5ca75bc727..7812e93734b8 100644
>> --- a/drivers/platform/x86/asus-armoury.c
>> +++ b/drivers/platform/x86/asus-armoury.c
>> @@ -94,6 +94,7 @@ struct asus_armoury_priv {
>>  
>>  	u32 mini_led_dev_id;
>>  	u32 gpu_mux_dev_id;
>> +	u32 dgpu_disable_dev_id;
>>  
>>  	bool requires_fan_curve;
>>  };
>> @@ -110,11 +111,6 @@ static struct fw_attrs_group fw_attrs = {
>>  	.pending_reboot = false,
>>  };
>>  
>> -struct asus_attr_group {
>> -	const struct attribute_group *attr_group;
>> -	u32 wmi_devid;
>> -};
>> -
>>  static void asus_set_reboot_and_signal_event(void)
>>  {
>>  	fw_attrs.pending_reboot = true;
>> @@ -458,6 +454,12 @@ static ssize_t mini_led_mode_possible_values_show(struct kobject *kobj,
>>  		return -ENODEV;
>>  	}
>>  }
>> +
>> +static bool mini_led_mode_group_visible(struct kobject *kobj)
>> +{
>> +	return asus_armoury.mini_led_dev_id;
>> +}
>> +
>>  ASUS_ATTR_GROUP_ENUM(mini_led_mode, "mini_led_mode", "Set the mini-LED backlight mode");
>>  
>>  static ssize_t gpu_mux_mode_current_value_store(struct kobject *kobj,
>> @@ -471,8 +473,8 @@ static ssize_t gpu_mux_mode_current_value_store(struct kobject *kobj,
>>  	if (err)
>>  		return err;
>>  
>> -	if (armoury_has_devstate(ASUS_WMI_DEVID_DGPU)) {
>> -		err = armoury_get_devstate(NULL, &result, ASUS_WMI_DEVID_DGPU);
>> +	if (asus_armoury.dgpu_disable_dev_id) {
>> +		err = armoury_get_devstate(NULL, &result, asus_armoury.dgpu_disable_dev_id);
>>  		if (err)
>>  			return err;
>>  		if (result && !optimus) {
>> @@ -502,6 +504,12 @@ static ssize_t gpu_mux_mode_current_value_store(struct kobject *kobj,
>>  	return count;
>>  }
>>  ASUS_WMI_SHOW_INT(gpu_mux_mode_current_value, asus_armoury.gpu_mux_dev_id);
>> +
>> +static bool gpu_mux_mode_group_visible(struct kobject *kobj)
>> +{
>> +	return asus_armoury.gpu_mux_dev_id;
>> +}
>> +
>>  ASUS_ATTR_GROUP_BOOL(gpu_mux_mode, "gpu_mux_mode", "Set the GPU display MUX mode");
>>  
>>  static ssize_t dgpu_disable_current_value_store(struct kobject *kobj,
>> @@ -538,7 +546,8 @@ static ssize_t dgpu_disable_current_value_store(struct kobject *kobj,
>>  	}
>>  
>>  	scoped_guard(mutex, &asus_armoury.egpu_mutex) {
>> -		err = armoury_set_devstate(attr, disable ? 1 : 0, NULL, ASUS_WMI_DEVID_DGPU);
>> +		err = armoury_set_devstate(attr, disable ? 1 : 0, NULL,
>> +					   asus_armoury.dgpu_disable_dev_id);
>>  		if (err)
>>  			return err;
>>  	}
>> @@ -547,7 +556,13 @@ static ssize_t dgpu_disable_current_value_store(struct kobject *kobj,
>>  
>>  	return count;
>>  }
>> -ASUS_WMI_SHOW_INT(dgpu_disable_current_value, ASUS_WMI_DEVID_DGPU);
>> +
>> +static bool dgpu_disable_group_visible(struct kobject *kobj)
>> +{
>> +	return asus_armoury.dgpu_disable_dev_id;
>> +}
>> +
>> +ASUS_WMI_SHOW_INT(dgpu_disable_current_value, asus_armoury.dgpu_disable_dev_id);
>>  ASUS_ATTR_GROUP_BOOL(dgpu_disable, "dgpu_disable", "Disable the dGPU");
>>  
>>  /* Values map for eGPU activation requests. */
>> @@ -695,6 +710,12 @@ static ssize_t egpu_enable_possible_values_show(struct kobject *kobj, struct kob
>>  {
>>  	return armoury_attr_enum_list(buf, ARRAY_SIZE(egpu_status_map));
>>  }
>> +
>> +static bool egpu_enable_group_visible(struct kobject *kobj)
>> +{
>> +	return armoury_has_devstate(ASUS_WMI_DEVID_EGPU);
>> +}
>> +
>>  ASUS_ATTR_GROUP_ENUM(egpu_enable, "egpu_enable", "Enable the eGPU (also disables dGPU)");
>>  
>>  /* Device memory available to APU */
>> @@ -771,6 +792,12 @@ static ssize_t apu_mem_possible_values_show(struct kobject *kobj, struct kobj_at
>>  {
>>  	return armoury_attr_enum_list(buf, ARRAY_SIZE(apu_mem_map));
>>  }
>> +
>> +static bool apu_mem_group_visible(struct kobject *kobj)
>> +{
>> +	return armoury_has_devstate(ASUS_WMI_DEVID_APU_MEM);
>> +}
>> +
>>  ASUS_ATTR_GROUP_ENUM(apu_mem, "apu_mem", "Set available system RAM (in GB) for the APU to use");
>>  
>>  /* Define helper to access the current power mode tunable values */
>> @@ -782,6 +809,44 @@ static inline struct rog_tunables *get_current_tunables(void)
>>  	return asus_armoury.rog_tunables[ASUS_ROG_TUNABLE_DC];
>>  }
>>  
>> +/**
>> + * has_valid_limit - Checks if a power-related attribute has a valid limit value
>> + * @name: The name of the attribute to check
>> + * @limits: Pointer to the power_limits structure containing limit values
>> + *
>> + * This function checks if a power-related attribute has a valid limit value.
>> + * It returns false if limits is NULL or if the corresponding limit value is zero.
>> + *
>> + * Return: true if the attribute has a valid limit value, false otherwise
>> + */
>> +static bool has_valid_limit(const char *name, const struct power_limits *limits)
>> +{
>> +	u32 limit_value = 0;
>> +
>> +	if (!limits)
>> +		return false;
>> +
>> +	if (!strcmp(name, ATTR_PPT_PL1_SPL))
>> +		limit_value = limits->ppt_pl1_spl_max;
>> +	else if (!strcmp(name, ATTR_PPT_PL2_SPPT))
>> +		limit_value = limits->ppt_pl2_sppt_max;
>> +	else if (!strcmp(name, ATTR_PPT_PL3_FPPT))
>> +		limit_value = limits->ppt_pl3_fppt_max;
>> +	else if (!strcmp(name, ATTR_PPT_APU_SPPT))
>> +		limit_value = limits->ppt_apu_sppt_max;
>> +	else if (!strcmp(name, ATTR_PPT_PLATFORM_SPPT))
>> +		limit_value = limits->ppt_platform_sppt_max;
>> +	else if (!strcmp(name, ATTR_NV_DYNAMIC_BOOST))
>> +		limit_value = limits->nv_dynamic_boost_max;
>> +	else if (!strcmp(name, ATTR_NV_TEMP_TARGET))
>> +		limit_value = limits->nv_temp_target_max;
>> +	else if (!strcmp(name, ATTR_NV_BASE_TGP) ||
>> +		 !strcmp(name, ATTR_NV_TGP))
>> +		limit_value = limits->nv_tgp_max;
>> +
>> +	return limit_value > 0;
>> +}
> This is a plain move, right? Can you move it in a preparatory patch to 
> cut the extra churn from what is already a very complicated diff.

Yes: pure move because it is included in a macro now.

I'll move it in its own patch.

>> +
>>  /* Simple attribute creation */
>>  ASUS_ATTR_GROUP_ENUM_INT_RO(charge_mode, "charge_mode", ASUS_WMI_DEVID_CHARGE_MODE, "0;1;2\n",
>>  			    "Show the current mode of charging");
>> @@ -819,103 +884,35 @@ ASUS_ATTR_GROUP_ROG_TUNABLE(nv_tgp, "nv_tgp", ASUS_WMI_DEVID_DGPU_SET_TGP,
>>  ASUS_ATTR_GROUP_INT_VALUE_ONLY_RO(nv_base_tgp, ATTR_NV_BASE_TGP, ASUS_WMI_DEVID_DGPU_BASE_TGP,
>>  				  "Read the base TGP value");
>>  
>> -/* If an attribute does not require any special case handling add it here */
>> -static const struct asus_attr_group armoury_attr_groups[] = {
>> -	{ &egpu_connected_attr_group, ASUS_WMI_DEVID_EGPU_CONNECTED },
>> -	{ &egpu_enable_attr_group, ASUS_WMI_DEVID_EGPU },
>> -	{ &dgpu_disable_attr_group, ASUS_WMI_DEVID_DGPU },
>> -	{ &dgpu_power_state_attr_group, ASUS_WMI_DEVID_DGPU_POWER_STATE },
>> -	{ &apu_mem_attr_group, ASUS_WMI_DEVID_APU_MEM },
>> -
>> -	{ &ppt_pl1_spl_attr_group, ASUS_WMI_DEVID_PPT_PL1_SPL },
>> -	{ &ppt_pl2_sppt_attr_group, ASUS_WMI_DEVID_PPT_PL2_SPPT },
>> -	{ &ppt_pl3_fppt_attr_group, ASUS_WMI_DEVID_PPT_PL3_FPPT },
>> -	{ &ppt_apu_sppt_attr_group, ASUS_WMI_DEVID_PPT_APU_SPPT },
>> -	{ &ppt_platform_sppt_attr_group, ASUS_WMI_DEVID_PPT_PLAT_SPPT },
>> -	{ &nv_dynamic_boost_attr_group, ASUS_WMI_DEVID_NV_DYN_BOOST },
>> -	{ &nv_temp_target_attr_group, ASUS_WMI_DEVID_NV_THERM_TARGET },
>> -	{ &nv_base_tgp_attr_group, ASUS_WMI_DEVID_DGPU_BASE_TGP },
>> -	{ &nv_tgp_attr_group, ASUS_WMI_DEVID_DGPU_SET_TGP },
>> -
>> -	{ &charge_mode_attr_group, ASUS_WMI_DEVID_CHARGE_MODE },
>> -	{ &boot_sound_attr_group, ASUS_WMI_DEVID_BOOT_SOUND },
>> -	{ &mcu_powersave_attr_group, ASUS_WMI_DEVID_MCU_POWERSAVE },
>> -	{ &panel_od_attr_group, ASUS_WMI_DEVID_PANEL_OD },
>> -	{ &panel_hd_mode_attr_group, ASUS_WMI_DEVID_PANEL_HD },
>> -	{ &screen_auto_brightness_attr_group, ASUS_WMI_DEVID_SCREEN_AUTO_BRIGHTNESS },
>> +static const struct attribute_group *armoury_attr_groups[] = {
>> +	&mini_led_mode_attr_group,
>> +	&gpu_mux_mode_attr_group,
>> +	&egpu_connected_attr_group,
>> +	&egpu_enable_attr_group,
>> +	&dgpu_disable_attr_group,
>> +	&dgpu_power_state_attr_group,
>> +	&apu_mem_attr_group,
>> +
>> +	&ppt_pl1_spl_attr_group,
>> +	&ppt_pl2_sppt_attr_group,
>> +	&ppt_pl3_fppt_attr_group,
>> +	&ppt_apu_sppt_attr_group,
>> +	&ppt_platform_sppt_attr_group,
>> +	&nv_dynamic_boost_attr_group,
>> +	&nv_temp_target_attr_group,
>> +	&nv_base_tgp_attr_group,
>> +	&nv_tgp_attr_group,
>> +
>> +	&charge_mode_attr_group,
>> +	&boot_sound_attr_group,
>> +	&mcu_powersave_attr_group,
>> +	&panel_od_attr_group,
>> +	&panel_hd_mode_attr_group,
>> +	&screen_auto_brightness_attr_group,
>>  };
> This looks much better.

Yeah... at first I tried making things having the least amount of changes
to avoid this situation of "big changes all at once", but ultimately even
reading my own code having { attribute, maybe_not_correct_devid }
made my skin so itchy that I couldn't hold back myself from cleaning
that out.

> Now can you also add the terminating NULL to the array and try to use 
> sysfs_create/remove_groups() so you can eliminate the create, rollback, 
> and remove loops... I suggest you do it on top of this patch as this 
> change is already quite complicated and logically 
> sysfs_create/remove_group() -> sysfs_create/remove_groups() is a separate 
> transition.

Sure thing!

Thanks for your feedback, I may take some more days due to university
but I'll get it done as soon as possible.

Best,
Denis

>>  	for (i = 0; i < ARRAY_SIZE(armoury_attr_groups); i++) {
>> -		if (!armoury_has_devstate(armoury_attr_groups[i].wmi_devid))
>> -			continue;
>> -
>> -		/* Always create by default, unless PPT is not present */
>> -		should_create = true;
>> -		name = armoury_attr_groups[i].attr_group->name;
>> -
>> -		/* Check if this is a power-related tunable requiring limits */
>> -		if (ac_rog_tunables && ac_rog_tunables->power_limits &&
>> -		    is_power_tunable_attr(name)) {
>> -			limits = ac_rog_tunables->power_limits;
>> -			/* Check only AC: if not present then DC won't be either */
>> -			should_create = has_valid_limit(name, limits);
>> -			if (!should_create)
>> -				pr_debug("Missing max value for tunable %s\n", name);
>> -		}
>> -
>> -		if (should_create) {
>> -			err = sysfs_create_group(&asus_armoury.fw_attr_kset->kobj,
>> -						 armoury_attr_groups[i].attr_group);
>> -			if (err) {
>> -				pr_err("Failed to create sysfs-group for %s\n",
>> -				       armoury_attr_groups[i].attr_group->name);
>> -				goto err_remove_groups;
>> -			}
>> +		err = sysfs_create_group(&asus_armoury.fw_attr_kset->kobj,
>> +					 armoury_attr_groups[i]);
>> +		if (err) {
>> +			pr_err("Failed to create sysfs-group for %s\n",
>> +			       armoury_attr_groups[i]->name);
>> +			goto err_remove_groups;
>>  		}
>>  	}
>>  
>>  	return 0;
>>  
>>  err_remove_groups:
>> -	while (i--) {
>> -		if (armoury_has_devstate(armoury_attr_groups[i].wmi_devid))
>> -			sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj,
>> -					   armoury_attr_groups[i].attr_group);
>> -	}
>> -	if (asus_armoury.gpu_mux_dev_id)
>> -		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj, &gpu_mux_mode_attr_group);
>> -err_remove_mini_led_group:
>> -	if (asus_armoury.mini_led_dev_id)
>> -		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj, &mini_led_mode_attr_group);
>> -err_remove_file:
>> +	while (i--)
>> +		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj,
>> +				   armoury_attr_groups[i]);
>>  	sysfs_remove_file(&asus_armoury.fw_attr_kset->kobj, &pending_reboot.attr);
>>  err_destroy_kset:
>>  	kset_unregister(asus_armoury.fw_attr_kset);
>> @@ -1182,17 +1138,9 @@ static void __exit asus_fw_exit(void)
>>  {
>>  	int i;
>>  
>> -	for (i = ARRAY_SIZE(armoury_attr_groups) - 1; i >= 0; i--) {
>> -		if (armoury_has_devstate(armoury_attr_groups[i].wmi_devid))
>> -			sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj,
>> -					   armoury_attr_groups[i].attr_group);
>> -	}
>> -
>> -	if (asus_armoury.gpu_mux_dev_id)
>> -		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj, &gpu_mux_mode_attr_group);
>> -
>> -	if (asus_armoury.mini_led_dev_id)
>> -		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj, &mini_led_mode_attr_group);
>> +	for (i = ARRAY_SIZE(armoury_attr_groups) - 1; i >= 0; i--)
>> +		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj,
>> +				   armoury_attr_groups[i]);