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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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.