[PATCH 0/3] platform/x86: asus-wmi: move keyboard control firmware attributes

[PATCH 0/3] platform/x86: asus-wmi: move keyboard control firmware attributes

[Denis Benato]

Hi all,

I was recently reading through the asusctl issue tracker and I found
out that some users have been having troubles with the keyboard RGB control
that was working before since the creation of asus-armoury and subequent
deprecation of old sysfs attributes.

This patch series aims to re-introduce those attributes in asus-armoury
so that userspace tools can still control keyboard RGB lighting
without having to rely on deprecated asus-wmi attributes.

In addition to that, since disabling OOBE is essential for controlling
LEDs on some models and it was incorrectly tied to deprecated attributes,
this patch series also fixes sending OOBE at probe time.

Link: https://gitlab.com/asus-linux/asusctl/-/issues/619

Denis Benato (3):
  platform/x86: asus-wmi: explicitly mark more code with
    CONFIG_ASUS_WMI_DEPRECATED_ATTRS
  platform/x86: asus-wmi: fix sending OOBE at probe
  platform/x86: asus-armoury: add keyboard control firmware attributes

 drivers/platform/x86/asus-armoury.c        | 258 +++++++++++++++++++++
 drivers/platform/x86/asus-wmi.c            |  13 +-
 include/linux/platform_data/x86/asus-wmi.h |  15 ++
 3 files changed, 285 insertions(+), 1 deletion(-)

-- 
2.52.0

[PATCH 1/3] platform/x86: asus-wmi: explicitly mark more code with CONFIG_ASUS_WMI_DEPRECATED_ATTRS

[Denis Benato]

Be more explicit in code that will be excluded when compiling
with CONFIG_ASUS_WMI_DEPRECATED_ATTRS disabled.

Signed-off-by: Denis Benato <denis.benato@linux.dev>
---
 drivers/platform/x86/asus-wmi.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 4aec7ec69250..a49447eff4f4 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -302,7 +302,11 @@ struct asus_wmi {
 	u32 nv_temp_target;
 
 	u32 kbd_rgb_dev;
+
+#if IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS)
 	bool kbd_rgb_state_available;
+#endif /* IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS) */
+
 	bool oobe_state_available;
 
 	u8 throttle_thermal_policy_mode;
@@ -1060,6 +1064,7 @@ static const struct attribute_group kbd_rgb_mode_group = {
 };
 
 /* TUF Laptop Keyboard RGB State **********************************************/
+#if IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS)
 static ssize_t kbd_rgb_state_store(struct device *dev,
 				 struct device_attribute *attr,
 				 const char *buf, size_t count)
@@ -1106,6 +1111,8 @@ static const struct attribute_group kbd_rgb_state_group = {
 	.attrs = kbd_rgb_state_attrs,
 };
 
+#endif /* IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS) */
+
 static const struct attribute_group *kbd_rgb_mode_groups[] = {
 	NULL,
 	NULL,
@@ -1861,8 +1868,11 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
 
 	if (asus->kbd_rgb_dev)
 		kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_mode_group;
+
+#if IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS)
 	if (asus->kbd_rgb_state_available)
 		kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_state_group;
+#endif /* IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS) */
 
 	asus->led_workqueue = create_singlethread_workqueue("led_workqueue");
 	if (!asus->led_workqueue)
-- 
2.52.0

[PATCH 2/3] platform/x86: asus-wmi: fix sending OOBE at probe

[Denis Benato]

Disabling oobe is an important step to be able to fully control
the hardware in TUF laptops that supports this feature,
but the command has been incorrectly tied to deprecated
attributes: restore sending the OOBE exit command.

Signed-off-by: Denis Benato <denis.benato@linux.dev>
---
 drivers/platform/x86/asus-wmi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index a49447eff4f4..8dfdde7877a8 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -4899,7 +4899,6 @@ static int asus_wmi_add(struct platform_device *pdev)
 	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);
-	asus->oobe_state_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_OOBE);
 
 	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE))
 		asus->mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE;
@@ -4912,6 +4911,8 @@ static int asus_wmi_add(struct platform_device *pdev)
 		asus->gpu_mux_dev = ASUS_WMI_DEVID_GPU_MUX_VIVO;
 #endif /* IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS) */
 
+	asus->oobe_state_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_OOBE);
+
 	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY))
 		asus->throttle_thermal_policy_dev = ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY;
 	else if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO))
-- 
2.52.0

[PATCH 3/3] platform/x86: asus-armoury: add keyboard control firmware attributes

[Denis Benato]

Implement keyboard status attributes in asus-armoury after
deprecating those attribute(s) in asus-wmi to avoid losing
the ability to control LEDs status.

Signed-off-by: Denis Benato <benato.denis96@gmail.com>
---
 drivers/platform/x86/asus-armoury.c        | 258 +++++++++++++++++++++
 include/linux/platform_data/x86/asus-wmi.h |  15 ++
 2 files changed, 273 insertions(+)

diff --git a/drivers/platform/x86/asus-armoury.c b/drivers/platform/x86/asus-armoury.c
index 9c1a9ad42bc4..a37713854691 100644
--- a/drivers/platform/x86/asus-armoury.c
+++ b/drivers/platform/x86/asus-armoury.c
@@ -76,10 +76,22 @@ struct rog_tunables {
 	u32 nv_tgp;
 };
 
+struct asus_armoury_kbd_status {
+	bool boot;
+	bool awake;
+	bool sleep;
+	bool shutdown;
+};
+
 struct asus_armoury_priv {
 	struct device *fw_attr_dev;
 	struct kset *fw_attr_kset;
 
+	struct mutex keyboard_mutex;
+
+	/* Current TUF keyboard RGB state tracking */
+	struct asus_armoury_kbd_status *kbd_state;
+
 	/*
 	 * Mutex to protect eGPU activation/deactivation
 	 * sequences and dGPU connection status:
@@ -97,6 +109,7 @@ struct asus_armoury_priv {
 
 static struct asus_armoury_priv asus_armoury = {
 	.egpu_mutex = __MUTEX_INITIALIZER(asus_armoury.egpu_mutex),
+	.keyboard_mutex = __MUTEX_INITIALIZER(asus_armoury.keyboard_mutex),
 };
 
 struct fw_attrs_group {
@@ -433,6 +446,169 @@ static ssize_t mini_led_mode_possible_values_show(struct kobject *kobj,
 }
 ASUS_ATTR_GROUP_ENUM(mini_led_mode, "mini_led_mode", "Set the mini-LED backlight mode");
 
+/* Keyboard power management **************************************************/
+
+static int armoury_kbd_state(struct kobj_attribute *attr,
+			     const struct asus_armoury_kbd_status *status)
+{
+	u32 kbd_state = 0xBD | BIT(2) << 8;
+
+	if (status->boot)
+		kbd_state |= BIT(1) << 16;
+	if (status->awake)
+		kbd_state |= BIT(3) << 16;
+	if (status->sleep)
+		kbd_state |= BIT(5) << 16;
+	if (status->shutdown)
+		kbd_state |= BIT(7) << 16;
+
+	return armoury_set_devstate(attr, kbd_state, NULL,
+				    ASUS_WMI_DEVID_TUF_RGB_STATE);
+}
+
+enum asus_armoury_kbd_state_field {
+	ASUS_ARMOURY_KBD_STATE_BOOT,
+	ASUS_ARMOURY_KBD_STATE_AWAKE,
+	ASUS_ARMOURY_KBD_STATE_SLEEP,
+	ASUS_ARMOURY_KBD_STATE_SHUTDOWN,
+};
+
+static ssize_t armoury_kbd_state_write(struct kobject *kobj, struct kobj_attribute *attr,
+				       const char *buf, size_t count,
+				       enum asus_armoury_kbd_state_field field)
+{
+	ssize_t err;
+	bool enable;
+	struct asus_armoury_kbd_status kbd_status;
+
+	err = kstrtobool(buf, &enable);
+	if (err)
+		return err;
+
+	scoped_guard(mutex, &asus_armoury.keyboard_mutex) {
+		memcpy(&kbd_status, asus_armoury.kbd_state, sizeof(kbd_status));
+
+		switch (field) {
+		case ASUS_ARMOURY_KBD_STATE_BOOT:
+			kbd_status.boot = enable;
+			break;
+		case ASUS_ARMOURY_KBD_STATE_AWAKE:
+			kbd_status.awake = enable;
+			break;
+		case ASUS_ARMOURY_KBD_STATE_SLEEP:
+			kbd_status.sleep = enable;
+			break;
+		case ASUS_ARMOURY_KBD_STATE_SHUTDOWN:
+			kbd_status.shutdown = enable;
+			break;
+		default:
+			return -EINVAL;
+		}
+
+		err = armoury_kbd_state(attr, &kbd_status);
+		if (err)
+			return err;
+
+		memcpy(asus_armoury.kbd_state, &kbd_status, sizeof(kbd_status));
+	}
+
+	sysfs_notify(kobj, NULL, attr->attr.name);
+
+	return count;
+}
+
+static ssize_t armoury_kbd_state_read(struct kobject *kobj, struct kobj_attribute *attr,
+				      char *buf, enum asus_armoury_kbd_state_field field)
+{
+	bool *field_ptr, field_enabled;
+
+	switch (field) {
+	case ASUS_ARMOURY_KBD_STATE_AWAKE:
+		field_ptr = &asus_armoury.kbd_state->awake;
+		break;
+	case ASUS_ARMOURY_KBD_STATE_SLEEP:
+		field_ptr = &asus_armoury.kbd_state->sleep;
+		break;
+	case ASUS_ARMOURY_KBD_STATE_BOOT:
+		field_ptr = &asus_armoury.kbd_state->boot;
+		break;
+	case ASUS_ARMOURY_KBD_STATE_SHUTDOWN:
+		field_ptr = &asus_armoury.kbd_state->shutdown;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	scoped_guard(mutex, &asus_armoury.keyboard_mutex)
+		field_enabled = *field_ptr;
+
+	return sysfs_emit(buf, field_enabled ? "1\n" : "0\n");
+}
+
+static ssize_t kbd_leds_sleep_current_value_store(struct kobject *kobj,
+						  struct kobj_attribute *attr,
+						  const char *buf, size_t count)
+{
+	return armoury_kbd_state_write(kobj, attr, buf, count, ASUS_ARMOURY_KBD_STATE_SLEEP);
+}
+
+static ssize_t kbd_leds_sleep_current_value_show(struct kobject *kobj,
+						 struct kobj_attribute *attr, char *buf)
+{
+	return armoury_kbd_state_read(kobj, attr, buf, ASUS_ARMOURY_KBD_STATE_SLEEP);
+}
+
+ASUS_ATTR_GROUP_BOOL(kbd_leds_sleep, "kbd_leds_sleep",
+		     "Keyboard backlight while system is sleeping");
+
+static ssize_t kbd_leds_boot_current_value_store(struct kobject *kobj,
+						 struct kobj_attribute *attr,
+						 const char *buf, size_t count)
+{
+	return armoury_kbd_state_write(kobj, attr, buf, count, ASUS_ARMOURY_KBD_STATE_BOOT);
+}
+
+static ssize_t kbd_leds_boot_current_value_show(struct kobject *kobj,
+						struct kobj_attribute *attr, char *buf)
+{
+	return armoury_kbd_state_read(kobj, attr, buf, ASUS_ARMOURY_KBD_STATE_BOOT);
+}
+
+ASUS_ATTR_GROUP_BOOL(kbd_leds_boot, "kbd_leds_boot",
+		     "Keyboard backlight while system is booting");
+
+static ssize_t kbd_leds_awake_current_value_store(struct kobject *kobj,
+						  struct kobj_attribute *attr,
+						  const char *buf, size_t count)
+{
+	return armoury_kbd_state_write(kobj, attr, buf, count, ASUS_ARMOURY_KBD_STATE_AWAKE);
+}
+
+static ssize_t kbd_leds_awake_current_value_show(struct kobject *kobj,
+						 struct kobj_attribute *attr, char *buf)
+{
+	return armoury_kbd_state_read(kobj, attr, buf, ASUS_ARMOURY_KBD_STATE_AWAKE);
+}
+
+ASUS_ATTR_GROUP_BOOL(kbd_leds_awake, "kbd_leds_awake",
+		     "Keyboard backlight while system is awake");
+
+static ssize_t kbd_leds_shutdown_current_value_store(struct kobject *kobj,
+						     struct kobj_attribute *attr,
+						     const char *buf, size_t count)
+{
+	return armoury_kbd_state_write(kobj, attr, buf, count, ASUS_ARMOURY_KBD_STATE_SHUTDOWN);
+}
+
+static ssize_t kbd_leds_shutdown_current_value_show(struct kobject *kobj,
+						    struct kobj_attribute *attr, char *buf)
+{
+	return armoury_kbd_state_read(kobj, attr, buf, ASUS_ARMOURY_KBD_STATE_SHUTDOWN);
+}
+
+ASUS_ATTR_GROUP_BOOL(kbd_leds_shutdown, "kbd_leds_shutdown",
+		     "Keyboard backlight while system is shutdown");
+
 static ssize_t gpu_mux_mode_current_value_store(struct kobject *kobj,
 						struct kobj_attribute *attr,
 						const char *buf, size_t count)
@@ -867,6 +1043,35 @@ static bool has_valid_limit(const char *name, const struct power_limits *limits)
 	return limit_value > 0;
 }
 
+static struct asus_armoury_kbd_status *asus_init_kbd_state(void)
+{
+	int err;
+	u32 kbd_status;
+	struct asus_armoury_kbd_status *kbd_state __free(kfree) = NULL;
+
+	err = armoury_get_devstate(NULL, &kbd_status, ASUS_WMI_DEVID_TUF_RGB_STATE);
+	if (err) {
+		pr_err("ACPI does not support keyboard power control: %d\n", err);
+		return ERR_PTR(-ENODEV);
+	}
+
+	pr_info("Detected keyboard backlight support\n");
+
+	kbd_state = kzalloc(sizeof(*kbd_state), GFP_KERNEL);
+	if (!kbd_state)
+		return ERR_PTR(-ENODEV);
+
+	/*
+	 * By default leds are off for all states (to spare power)
+	 * except for when laptop is awake, where leds color and
+	 * brightness are controllable by userspace.
+	 */
+	memset(kbd_state, 0, sizeof(*kbd_state));
+	kbd_state->awake = true;
+
+	return no_free_ptr(kbd_state);
+}
+
 static int asus_fw_attr_add(void)
 {
 	const struct rog_tunables *const ac_rog_tunables =
@@ -926,6 +1131,51 @@ static int asus_fw_attr_add(void)
 		}
 	}
 
+	asus_armoury.kbd_state = NULL;
+	if (armoury_has_devstate(ASUS_WMI_DEVID_TUF_RGB_STATE)) {
+		asus_armoury.kbd_state = asus_init_kbd_state();
+		if (IS_ERR(asus_armoury.kbd_state)) {
+			err = PTR_ERR(asus_armoury.kbd_state);
+			pr_err("Failed to get keyboard status: %d\n", err);
+			goto err_remove_kbd_state;
+		}
+
+		err = sysfs_create_group(&asus_armoury.fw_attr_kset->kobj, &kbd_leds_sleep_attr_group);
+		if (err) {
+			pr_err("Failed to create sysfs-group for keyboard backlight sleep state: %d\n", err);
+			goto err_remove_kbd_state;
+		}
+
+		err = sysfs_create_group(&asus_armoury.fw_attr_kset->kobj, &kbd_leds_boot_attr_group);
+		if (err) {
+			pr_err("Failed to create sysfs-group for keyboard backlight boot state: %d\n", err);
+			goto err_remove_kbd_state;
+		}
+
+		err = sysfs_create_group(&asus_armoury.fw_attr_kset->kobj, &kbd_leds_awake_attr_group);
+		if (err) {
+			pr_err("Failed to create sysfs-group for keyboard backlight awake state: %d\n", err);
+			goto err_remove_kbd_state;
+		}
+
+		err = sysfs_create_group(&asus_armoury.fw_attr_kset->kobj, &kbd_leds_shutdown_attr_group);
+		if (err) {
+			pr_err("Failed to create sysfs-group for keyboard backlight shutdown state: %d\n", err);
+			goto err_remove_kbd_state;
+		}
+
+		/*
+		 * The attribute is write-only and for the state to be coherent
+		 * a default state has to written: userspace is expected to
+		 * modify it based on user preference.
+		 */
+		err = armoury_kbd_state(&attr_kbd_leds_awake_current_value, asus_armoury.kbd_state);
+		if (err) {
+			pr_err("Failed to initialize keyboard backlight states: %d\n", err);
+			goto err_remove_kbd_state;
+		}
+	}
+
 	for (i = 0; i < ARRAY_SIZE(armoury_attr_groups); i++) {
 		if (!armoury_has_devstate(armoury_attr_groups[i].wmi_devid))
 			continue;
@@ -965,6 +1215,14 @@ static int asus_fw_attr_add(void)
 	}
 	if (asus_armoury.gpu_mux_dev_id)
 		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj, &gpu_mux_mode_attr_group);
+err_remove_kbd_state:
+	kfree(asus_armoury.kbd_state);
+	if (asus_armoury.kbd_state) {
+		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj, &kbd_leds_sleep_attr_group);
+		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj, &kbd_leds_boot_attr_group);
+		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj, &kbd_leds_awake_attr_group);
+		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj, &kbd_leds_shutdown_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);
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 419491d4abca..6667b3b474c2 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -153,6 +153,21 @@
 /* TUF laptop RGB power/state */
 #define ASUS_WMI_DEVID_TUF_RGB_STATE	0x00100057
 
+/*
+ * Flags for TUF RGB state to be used with
+ * ASUS_WMI_DEVID_TUF_RGB_STATE:
+ * 0xBD | BIT(1) << 8 | flags
+ *
+ * where 0xBD is required for the method call
+ * to not be discarded, BIT(2)<<8 specified
+ * this is a command and flags is a combination
+ * of one or more of the following flags.
+ */
+#define ASUS_WMI_DEVID_TUF_RGB_STATE_BOOT (BIT(1) << 16u)
+#define ASUS_WMI_DEVID_TUF_RGB_STATE_AWAKE (BIT(3) << 16u)
+#define ASUS_WMI_DEVID_TUF_RGB_STATE_SLEEP (BIT(5) << 16u)
+#define ASUS_WMI_DEVID_TUF_RGB_STATE_SHUTDOWN (BIT(7) << 16u)
+
 /* Bootup sound control */
 #define ASUS_WMI_DEVID_BOOT_SOUND	0x00130022
 
-- 
2.52.0

Re: [PATCH 3/3] platform/x86: asus-armoury: add keyboard control firmware attributes

[Krzysztof Kozlowski]

On 25/12/2025 15:30, Denis Benato wrote:
> +ASUS_ATTR_GROUP_BOOL(kbd_leds_shutdown, "kbd_leds_shutdown",
> +		     "Keyboard backlight while system is shutdown");
> +
>  static ssize_t gpu_mux_mode_current_value_store(struct kobject *kobj,
>  						struct kobj_attribute *attr,
>  						const char *buf, size_t count)
> @@ -867,6 +1043,35 @@ static bool has_valid_limit(const char *name, const struct power_limits *limits)
>  	return limit_value > 0;
>  }
>  
> +static struct asus_armoury_kbd_status *asus_init_kbd_state(void)
> +{
> +	int err;
> +	u32 kbd_status;
> +	struct asus_armoury_kbd_status *kbd_state __free(kfree) = NULL;

This is an undesired syntax explicitly documented as one to avoid. You
need here proper assignment, not NULL. Please don't use cleanup.h if you
do not intend to follow it because it does not make the code simpler.

> +
> +	err = armoury_get_devstate(NULL, &kbd_status, ASUS_WMI_DEVID_TUF_RGB_STATE);
> +	if (err) {
> +		pr_err("ACPI does not support keyboard power control: %d\n", err);
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	pr_info("Detected keyboard backlight support\n");

This does not look like useful printk message. Drivers should be silent
on success:
https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/coding-style.rst#L913
https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/debugging/driver_development_debugging_guide.rst#L79


Best regards,
Krzysztof

Re: [PATCH 3/3] platform/x86: asus-armoury: add keyboard control firmware attributes

[Denis Benato]

On 12/26/25 12:06, Krzysztof Kozlowski wrote:
> On 25/12/2025 15:30, Denis Benato wrote:
>> +ASUS_ATTR_GROUP_BOOL(kbd_leds_shutdown, "kbd_leds_shutdown",
>> +		     "Keyboard backlight while system is shutdown");
>> +
>>  static ssize_t gpu_mux_mode_current_value_store(struct kobject *kobj,
>>  						struct kobj_attribute *attr,
>>  						const char *buf, size_t count)
>> @@ -867,6 +1043,35 @@ static bool has_valid_limit(const char *name, const struct power_limits *limits)
>>  	return limit_value > 0;
>>  }
>>  
>> +static struct asus_armoury_kbd_status *asus_init_kbd_state(void)
>> +{
>> +	int err;
>> +	u32 kbd_status;
>> +	struct asus_armoury_kbd_status *kbd_state __free(kfree) = NULL;
> This is an undesired syntax explicitly documented as one to avoid. You
> need here proper assignment, not NULL. Please don't use cleanup.h if you
> do not intend to follow it because it does not make the code simpler.
Hello and thank you for your feedback!

I have used __free here to match a previous comment from Ilpo:
https://lore.kernel.org/all/25bd0c90-2de0-ef66-c18d-661180b71fd4@linux.intel.com/
and I figured that since this is the same exact pattern as that it would have made
sense to use it.

May I ask you to elaborate further please? If there is a more effective way to take
advantage of cleanup.h I will very much consider it.
>> +
>> +	err = armoury_get_devstate(NULL, &kbd_status, ASUS_WMI_DEVID_TUF_RGB_STATE);
>> +	if (err) {
>> +		pr_err("ACPI does not support keyboard power control: %d\n", err);
>> +		return ERR_PTR(-ENODEV);
>> +	}
>> +
>> +	pr_info("Detected keyboard backlight support\n");
> This does not look like useful printk message. Drivers should be silent
> on success:
> https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/coding-style.rst#L913
> https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/debugging/driver_development_debugging_guide.rst#L79
>
I will remove the detected keyboard and make the pr_err a pr_info,
is this okay?

Thank you again,
Denis
> Best regards,
> Krzysztof

Re: [PATCH 3/3] platform/x86: asus-armoury: add keyboard control firmware attributes

[Krzysztof Kozlowski]

On 26/12/2025 12:46, Denis Benato wrote:
> 
> On 12/26/25 12:06, Krzysztof Kozlowski wrote:
>> On 25/12/2025 15:30, Denis Benato wrote:
>>> +ASUS_ATTR_GROUP_BOOL(kbd_leds_shutdown, "kbd_leds_shutdown",
>>> +		     "Keyboard backlight while system is shutdown");
>>> +
>>>  static ssize_t gpu_mux_mode_current_value_store(struct kobject *kobj,
>>>  						struct kobj_attribute *attr,
>>>  						const char *buf, size_t count)
>>> @@ -867,6 +1043,35 @@ static bool has_valid_limit(const char *name, const struct power_limits *limits)
>>>  	return limit_value > 0;
>>>  }
>>>  
>>> +static struct asus_armoury_kbd_status *asus_init_kbd_state(void)
>>> +{
>>> +	int err;
>>> +	u32 kbd_status;
>>> +	struct asus_armoury_kbd_status *kbd_state __free(kfree) = NULL;
>> This is an undesired syntax explicitly documented as one to avoid. You
>> need here proper assignment, not NULL. Please don't use cleanup.h if you
>> do not intend to follow it because it does not make the code simpler.
> Hello and thank you for your feedback!
> 
> I have used __free here to match a previous comment from Ilpo:
> https://lore.kernel.org/all/25bd0c90-2de0-ef66-c18d-661180b71fd4@linux.intel.com/
> and I figured that since this is the same exact pattern as that it would have made
> sense to use it.
> 
> May I ask you to elaborate further please? If there is a more effective way to take
> advantage of cleanup.h I will very much consider it.

Just read the doc before using it. It is very, very clear what is the
recommended syntax,


Best regards,
Krzysztof

Re: [PATCH 1/3] platform/x86: asus-wmi: explicitly mark more code with CONFIG_ASUS_WMI_DEPRECATED_ATTRS

[Ilpo Järvinen]

On Thu, 25 Dec 2025, Denis Benato wrote:

> Be more explicit in code that will be excluded when compiling
> with CONFIG_ASUS_WMI_DEPRECATED_ATTRS disabled.

Does this result in build fails or warnings? If that's the case, it should 
be mentioned in the changelog + Fixes tag added.

> Signed-off-by: Denis Benato <denis.benato@linux.dev>
> ---
>  drivers/platform/x86/asus-wmi.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 4aec7ec69250..a49447eff4f4 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -302,7 +302,11 @@ struct asus_wmi {
>  	u32 nv_temp_target;
>  
>  	u32 kbd_rgb_dev;
> +
> +#if IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS)
>  	bool kbd_rgb_state_available;
> +#endif /* IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS) */
> +
>  	bool oobe_state_available;
>  
>  	u8 throttle_thermal_policy_mode;
> @@ -1060,6 +1064,7 @@ static const struct attribute_group kbd_rgb_mode_group = {
>  };
>  
>  /* TUF Laptop Keyboard RGB State **********************************************/
> +#if IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS)
>  static ssize_t kbd_rgb_state_store(struct device *dev,
>  				 struct device_attribute *attr,
>  				 const char *buf, size_t count)
> @@ -1106,6 +1111,8 @@ static const struct attribute_group kbd_rgb_state_group = {
>  	.attrs = kbd_rgb_state_attrs,
>  };
>  
> +#endif /* IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS) */
> +
>  static const struct attribute_group *kbd_rgb_mode_groups[] = {
>  	NULL,
>  	NULL,
> @@ -1861,8 +1868,11 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
>  
>  	if (asus->kbd_rgb_dev)
>  		kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_mode_group;
> +
> +#if IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS)
>  	if (asus->kbd_rgb_state_available)
>  		kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_state_group;
> +#endif /* IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS) */
>  
>  	asus->led_workqueue = create_singlethread_workqueue("led_workqueue");
>  	if (!asus->led_workqueue)
> 

-- 
 i.

Re: [PATCH 2/3] platform/x86: asus-wmi: fix sending OOBE at probe

[Ilpo Järvinen]

On Thu, 25 Dec 2025, Denis Benato wrote:

> Disabling oobe is an important step to be able to fully control
> the hardware in TUF laptops that supports this feature,
> but the command has been incorrectly tied to deprecated
> attributes: restore sending the OOBE exit command.

Please reflow to 72 chars.

Fixes tag seems to be missing.

> Signed-off-by: Denis Benato <denis.benato@linux.dev>
> ---
>  drivers/platform/x86/asus-wmi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index a49447eff4f4..8dfdde7877a8 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -4899,7 +4899,6 @@ static int asus_wmi_add(struct platform_device *pdev)
>  	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);
> -	asus->oobe_state_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_OOBE);
>  
>  	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE))
>  		asus->mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE;
> @@ -4912,6 +4911,8 @@ static int asus_wmi_add(struct platform_device *pdev)
>  		asus->gpu_mux_dev = ASUS_WMI_DEVID_GPU_MUX_VIVO;
>  #endif /* IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS) */
>  
> +	asus->oobe_state_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_OOBE);
> +
>  	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY))
>  		asus->throttle_thermal_policy_dev = ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY;
>  	else if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO))
> 

-- 
 i.

Re: [PATCH 2/3] platform/x86: asus-wmi: fix sending OOBE at probe

[Ilpo Järvinen]

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

On Mon, 29 Dec 2025, Ilpo Järvinen wrote:

> On Thu, 25 Dec 2025, Denis Benato wrote:
> 
> > Disabling oobe is an important step to be able to fully control

Also, please capitalize OOBE here as well.

-- 
 i.

> > the hardware in TUF laptops that supports this feature,
> > but the command has been incorrectly tied to deprecated
> > attributes: restore sending the OOBE exit command.
> 
> Please reflow to 72 chars.
> 
> Fixes tag seems to be missing.
> 
> > Signed-off-by: Denis Benato <denis.benato@linux.dev>
> > ---
> >  drivers/platform/x86/asus-wmi.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> > index a49447eff4f4..8dfdde7877a8 100644
> > --- a/drivers/platform/x86/asus-wmi.c
> > +++ b/drivers/platform/x86/asus-wmi.c
> > @@ -4899,7 +4899,6 @@ static int asus_wmi_add(struct platform_device *pdev)
> >  	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);
> > -	asus->oobe_state_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_OOBE);
> >  
> >  	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE))
> >  		asus->mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE;
> > @@ -4912,6 +4911,8 @@ static int asus_wmi_add(struct platform_device *pdev)
> >  		asus->gpu_mux_dev = ASUS_WMI_DEVID_GPU_MUX_VIVO;
> >  #endif /* IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS) */
> >  
> > +	asus->oobe_state_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_OOBE);
> > +
> >  	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY))
> >  		asus->throttle_thermal_policy_dev = ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY;
> >  	else if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY_VIVO))
> > 
> 
> 

Re: [PATCH 3/3] platform/x86: asus-armoury: add keyboard control firmware attributes

[Ilpo Järvinen]

On Thu, 25 Dec 2025, Denis Benato wrote:

> Implement keyboard status attributes in asus-armoury after
> deprecating those attribute(s) in asus-wmi to avoid losing
> the ability to control LEDs status.
> 
> Signed-off-by: Denis Benato <benato.denis96@gmail.com>
> ---
>  drivers/platform/x86/asus-armoury.c        | 258 +++++++++++++++++++++
>  include/linux/platform_data/x86/asus-wmi.h |  15 ++
>  2 files changed, 273 insertions(+)
> 
> diff --git a/drivers/platform/x86/asus-armoury.c b/drivers/platform/x86/asus-armoury.c
> index 9c1a9ad42bc4..a37713854691 100644
> --- a/drivers/platform/x86/asus-armoury.c
> +++ b/drivers/platform/x86/asus-armoury.c
> @@ -76,10 +76,22 @@ struct rog_tunables {
>  	u32 nv_tgp;
>  };
>  
> +struct asus_armoury_kbd_status {
> +	bool boot;
> +	bool awake;
> +	bool sleep;
> +	bool shutdown;
> +};
> +
>  struct asus_armoury_priv {
>  	struct device *fw_attr_dev;
>  	struct kset *fw_attr_kset;
>  
> +	struct mutex keyboard_mutex;
> +
> +	/* Current TUF keyboard RGB state tracking */
> +	struct asus_armoury_kbd_status *kbd_state;
> +
>  	/*
>  	 * Mutex to protect eGPU activation/deactivation
>  	 * sequences and dGPU connection status:
> @@ -97,6 +109,7 @@ struct asus_armoury_priv {
>  
>  static struct asus_armoury_priv asus_armoury = {
>  	.egpu_mutex = __MUTEX_INITIALIZER(asus_armoury.egpu_mutex),
> +	.keyboard_mutex = __MUTEX_INITIALIZER(asus_armoury.keyboard_mutex),
>  };
>  
>  struct fw_attrs_group {
> @@ -433,6 +446,169 @@ static ssize_t mini_led_mode_possible_values_show(struct kobject *kobj,
>  }
>  ASUS_ATTR_GROUP_ENUM(mini_led_mode, "mini_led_mode", "Set the mini-LED backlight mode");
>  
> +/* Keyboard power management **************************************************/
> +
> +static int armoury_kbd_state(struct kobj_attribute *attr,
> +			     const struct asus_armoury_kbd_status *status)
> +{
> +	u32 kbd_state = 0xBD | BIT(2) << 8;
> +
> +	if (status->boot)
> +		kbd_state |= BIT(1) << 16;
> +	if (status->awake)
> +		kbd_state |= BIT(3) << 16;
> +	if (status->sleep)
> +		kbd_state |= BIT(5) << 16;
> +	if (status->shutdown)
> +		kbd_state |= BIT(7) << 16;

Add the fields with define + GENMASK()

Name the bits within the fields with defines.

Use FIELD_PREP() here instead of open-coding the shifting.

Hmm (returning to add text after reading all the way to the bottom of 
the patch), you seem to already have defines for these but are using 
literals here??

> +
> +	return armoury_set_devstate(attr, kbd_state, NULL,
> +				    ASUS_WMI_DEVID_TUF_RGB_STATE);
> +}
> +
> +enum asus_armoury_kbd_state_field {
> +	ASUS_ARMOURY_KBD_STATE_BOOT,
> +	ASUS_ARMOURY_KBD_STATE_AWAKE,
> +	ASUS_ARMOURY_KBD_STATE_SLEEP,
> +	ASUS_ARMOURY_KBD_STATE_SHUTDOWN,
> +};
> +
> +static ssize_t armoury_kbd_state_write(struct kobject *kobj, struct kobj_attribute *attr,
> +				       const char *buf, size_t count,
> +				       enum asus_armoury_kbd_state_field field)
> +{
> +	ssize_t err;
> +	bool enable;
> +	struct asus_armoury_kbd_status kbd_status;
> +
> +	err = kstrtobool(buf, &enable);
> +	if (err)
> +		return err;
> +
> +	scoped_guard(mutex, &asus_armoury.keyboard_mutex) {
> +		memcpy(&kbd_status, asus_armoury.kbd_state, sizeof(kbd_status));
> +
> +		switch (field) {
> +		case ASUS_ARMOURY_KBD_STATE_BOOT:
> +			kbd_status.boot = enable;
> +			break;
> +		case ASUS_ARMOURY_KBD_STATE_AWAKE:
> +			kbd_status.awake = enable;
> +			break;
> +		case ASUS_ARMOURY_KBD_STATE_SLEEP:
> +			kbd_status.sleep = enable;
> +			break;
> +		case ASUS_ARMOURY_KBD_STATE_SHUTDOWN:
> +			kbd_status.shutdown = enable;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +		err = armoury_kbd_state(attr, &kbd_status);
> +		if (err)
> +			return err;
> +
> +		memcpy(asus_armoury.kbd_state, &kbd_status, sizeof(kbd_status));
> +	}
> +
> +	sysfs_notify(kobj, NULL, attr->attr.name);
> +
> +	return count;
> +}
> +
> +static ssize_t armoury_kbd_state_read(struct kobject *kobj, struct kobj_attribute *attr,
> +				      char *buf, enum asus_armoury_kbd_state_field field)
> +{
> +	bool *field_ptr, field_enabled;
> +
> +	switch (field) {
> +	case ASUS_ARMOURY_KBD_STATE_AWAKE:
> +		field_ptr = &asus_armoury.kbd_state->awake;
> +		break;
> +	case ASUS_ARMOURY_KBD_STATE_SLEEP:
> +		field_ptr = &asus_armoury.kbd_state->sleep;
> +		break;
> +	case ASUS_ARMOURY_KBD_STATE_BOOT:
> +		field_ptr = &asus_armoury.kbd_state->boot;
> +		break;
> +	case ASUS_ARMOURY_KBD_STATE_SHUTDOWN:
> +		field_ptr = &asus_armoury.kbd_state->shutdown;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	scoped_guard(mutex, &asus_armoury.keyboard_mutex)
> +		field_enabled = *field_ptr;
> +
> +	return sysfs_emit(buf, field_enabled ? "1\n" : "0\n");
> +}
> +
> +static ssize_t kbd_leds_sleep_current_value_store(struct kobject *kobj,
> +						  struct kobj_attribute *attr,
> +						  const char *buf, size_t count)
> +{
> +	return armoury_kbd_state_write(kobj, attr, buf, count, ASUS_ARMOURY_KBD_STATE_SLEEP);
> +}
> +
> +static ssize_t kbd_leds_sleep_current_value_show(struct kobject *kobj,
> +						 struct kobj_attribute *attr, char *buf)
> +{
> +	return armoury_kbd_state_read(kobj, attr, buf, ASUS_ARMOURY_KBD_STATE_SLEEP);
> +}
> +
> +ASUS_ATTR_GROUP_BOOL(kbd_leds_sleep, "kbd_leds_sleep",
> +		     "Keyboard backlight while system is sleeping");
> +
> +static ssize_t kbd_leds_boot_current_value_store(struct kobject *kobj,
> +						 struct kobj_attribute *attr,
> +						 const char *buf, size_t count)
> +{
> +	return armoury_kbd_state_write(kobj, attr, buf, count, ASUS_ARMOURY_KBD_STATE_BOOT);
> +}
> +
> +static ssize_t kbd_leds_boot_current_value_show(struct kobject *kobj,
> +						struct kobj_attribute *attr, char *buf)
> +{
> +	return armoury_kbd_state_read(kobj, attr, buf, ASUS_ARMOURY_KBD_STATE_BOOT);
> +}
> +
> +ASUS_ATTR_GROUP_BOOL(kbd_leds_boot, "kbd_leds_boot",
> +		     "Keyboard backlight while system is booting");
> +
> +static ssize_t kbd_leds_awake_current_value_store(struct kobject *kobj,
> +						  struct kobj_attribute *attr,
> +						  const char *buf, size_t count)
> +{
> +	return armoury_kbd_state_write(kobj, attr, buf, count, ASUS_ARMOURY_KBD_STATE_AWAKE);
> +}
> +
> +static ssize_t kbd_leds_awake_current_value_show(struct kobject *kobj,
> +						 struct kobj_attribute *attr, char *buf)
> +{
> +	return armoury_kbd_state_read(kobj, attr, buf, ASUS_ARMOURY_KBD_STATE_AWAKE);
> +}
> +
> +ASUS_ATTR_GROUP_BOOL(kbd_leds_awake, "kbd_leds_awake",
> +		     "Keyboard backlight while system is awake");
> +
> +static ssize_t kbd_leds_shutdown_current_value_store(struct kobject *kobj,
> +						     struct kobj_attribute *attr,
> +						     const char *buf, size_t count)
> +{
> +	return armoury_kbd_state_write(kobj, attr, buf, count, ASUS_ARMOURY_KBD_STATE_SHUTDOWN);
> +}
> +
> +static ssize_t kbd_leds_shutdown_current_value_show(struct kobject *kobj,
> +						    struct kobj_attribute *attr, char *buf)
> +{
> +	return armoury_kbd_state_read(kobj, attr, buf, ASUS_ARMOURY_KBD_STATE_SHUTDOWN);
> +}
> +
> +ASUS_ATTR_GROUP_BOOL(kbd_leds_shutdown, "kbd_leds_shutdown",
> +		     "Keyboard backlight while system is shutdown");
> +
>  static ssize_t gpu_mux_mode_current_value_store(struct kobject *kobj,
>  						struct kobj_attribute *attr,
>  						const char *buf, size_t count)
> @@ -867,6 +1043,35 @@ static bool has_valid_limit(const char *name, const struct power_limits *limits)
>  	return limit_value > 0;
>  }
>  
> +static struct asus_armoury_kbd_status *asus_init_kbd_state(void)
> +{
> +	int err;
> +	u32 kbd_status;
> +	struct asus_armoury_kbd_status *kbd_state __free(kfree) = NULL;
> +
> +	err = armoury_get_devstate(NULL, &kbd_status, ASUS_WMI_DEVID_TUF_RGB_STATE);
> +	if (err) {
> +		pr_err("ACPI does not support keyboard power control: %d\n", err);
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	pr_info("Detected keyboard backlight support\n");
> +
> +	kbd_state = kzalloc(sizeof(*kbd_state), GFP_KERNEL);
> +	if (!kbd_state)
> +		return ERR_PTR(-ENODEV);
> +
> +	/*
> +	 * By default leds are off for all states (to spare power)
> +	 * except for when laptop is awake, where leds color and
> +	 * brightness are controllable by userspace.
> +	 */
> +	memset(kbd_state, 0, sizeof(*kbd_state));

Didn't you just alloc this with kzalloc()???

> +	kbd_state->awake = true;
> +
> +	return no_free_ptr(kbd_state);
> +}
> +
>  static int asus_fw_attr_add(void)
>  {
>  	const struct rog_tunables *const ac_rog_tunables =
> @@ -926,6 +1131,51 @@ static int asus_fw_attr_add(void)
>  		}
>  	}
>  
> +	asus_armoury.kbd_state = NULL;
> +	if (armoury_has_devstate(ASUS_WMI_DEVID_TUF_RGB_STATE)) {
> +		asus_armoury.kbd_state = asus_init_kbd_state();
> +		if (IS_ERR(asus_armoury.kbd_state)) {
> +			err = PTR_ERR(asus_armoury.kbd_state);
> +			pr_err("Failed to get keyboard status: %d\n", err);
> +			goto err_remove_kbd_state;
> +		}
> +
> +		err = sysfs_create_group(&asus_armoury.fw_attr_kset->kobj, &kbd_leds_sleep_attr_group);
> +		if (err) {
> +			pr_err("Failed to create sysfs-group for keyboard backlight sleep state: %d\n", err);
> +			goto err_remove_kbd_state;
> +		}
> +
> +		err = sysfs_create_group(&asus_armoury.fw_attr_kset->kobj, &kbd_leds_boot_attr_group);
> +		if (err) {
> +			pr_err("Failed to create sysfs-group for keyboard backlight boot state: %d\n", err);
> +			goto err_remove_kbd_state;
> +		}
> +
> +		err = sysfs_create_group(&asus_armoury.fw_attr_kset->kobj, &kbd_leds_awake_attr_group);
> +		if (err) {
> +			pr_err("Failed to create sysfs-group for keyboard backlight awake state: %d\n", err);
> +			goto err_remove_kbd_state;
> +		}
> +
> +		err = sysfs_create_group(&asus_armoury.fw_attr_kset->kobj, &kbd_leds_shutdown_attr_group);
> +		if (err) {
> +			pr_err("Failed to create sysfs-group for keyboard backlight shutdown state: %d\n", err);
> +			goto err_remove_kbd_state;
> +		}
> +
> +		/*
> +		 * The attribute is write-only and for the state to be coherent
> +		 * a default state has to written: userspace is expected to
> +		 * modify it based on user preference.
> +		 */
> +		err = armoury_kbd_state(&attr_kbd_leds_awake_current_value, asus_armoury.kbd_state);
> +		if (err) {
> +			pr_err("Failed to initialize keyboard backlight states: %d\n", err);
> +			goto err_remove_kbd_state;
> +		}
> +	}
> +
>  	for (i = 0; i < ARRAY_SIZE(armoury_attr_groups); i++) {
>  		if (!armoury_has_devstate(armoury_attr_groups[i].wmi_devid))
>  			continue;
> @@ -965,6 +1215,14 @@ static int asus_fw_attr_add(void)
>  	}
>  	if (asus_armoury.gpu_mux_dev_id)
>  		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj, &gpu_mux_mode_attr_group);
> +err_remove_kbd_state:
> +	kfree(asus_armoury.kbd_state);
> +	if (asus_armoury.kbd_state) {

This looks like a logic bug, as IS_ERR() implies != 0.

> +		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj, &kbd_leds_sleep_attr_group);
> +		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj, &kbd_leds_boot_attr_group);
> +		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj, &kbd_leds_awake_attr_group);
> +		sysfs_remove_group(&asus_armoury.fw_attr_kset->kobj, &kbd_leds_shutdown_attr_group);

Is it fine to make these calls if one of the sysfs_create_group() failed?

> +	}

Why you do this in this order? In general, the teardown order should be a 
reverse of the order on the init side.

>  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);
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index 419491d4abca..6667b3b474c2 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -153,6 +153,21 @@
>  /* TUF laptop RGB power/state */
>  #define ASUS_WMI_DEVID_TUF_RGB_STATE	0x00100057
>  
> +/*
> + * Flags for TUF RGB state to be used with
> + * ASUS_WMI_DEVID_TUF_RGB_STATE:
> + * 0xBD | BIT(1) << 8 | flags

Would this be more logical ordering:

* flags | BIT(1) << 8 | 0xBD

?

(With the current ordering, I assumed BD is the most highest bits due to 
the way we write numbers).

> + *
> + * where 0xBD is required for the method call
> + * to not be discarded, BIT(2)<<8 specified

Spaces around <<

Name 0xBD, BIT(1) << 8 and BIT(2) << 8 with defines.

> + * this is a command and flags is a combination
> + * of one or more of the following flags.

Why is this using so short lines?

> + */
> +#define ASUS_WMI_DEVID_TUF_RGB_STATE_BOOT (BIT(1) << 16u)

Add flags field define & use FIELD_PREP() as already mentioned with the 
relevant code.

And please use these in the code too. :-)

> +#define ASUS_WMI_DEVID_TUF_RGB_STATE_AWAKE (BIT(3) << 16u)
> +#define ASUS_WMI_DEVID_TUF_RGB_STATE_SLEEP (BIT(5) << 16u)
> +#define ASUS_WMI_DEVID_TUF_RGB_STATE_SHUTDOWN (BIT(7) << 16u)
> +
>  /* Bootup sound control */
>  #define ASUS_WMI_DEVID_BOOT_SOUND	0x00130022
>  
> 

-- 
 i.

Re: [PATCH 3/3] platform/x86: asus-armoury: add keyboard control firmware attributes

[Ilpo Järvinen]

On Fri, 26 Dec 2025, Krzysztof Kozlowski wrote:

> On 26/12/2025 12:46, Denis Benato wrote:
> > 
> > On 12/26/25 12:06, Krzysztof Kozlowski wrote:
> >> On 25/12/2025 15:30, Denis Benato wrote:
> >>> +ASUS_ATTR_GROUP_BOOL(kbd_leds_shutdown, "kbd_leds_shutdown",
> >>> +		     "Keyboard backlight while system is shutdown");
> >>> +
> >>>  static ssize_t gpu_mux_mode_current_value_store(struct kobject *kobj,
> >>>  						struct kobj_attribute *attr,
> >>>  						const char *buf, size_t count)
> >>> @@ -867,6 +1043,35 @@ static bool has_valid_limit(const char *name, const struct power_limits *limits)
> >>>  	return limit_value > 0;
> >>>  }
> >>>  
> >>> +static struct asus_armoury_kbd_status *asus_init_kbd_state(void)
> >>> +{
> >>> +	int err;
> >>> +	u32 kbd_status;
> >>> +	struct asus_armoury_kbd_status *kbd_state __free(kfree) = NULL;
> >> This is an undesired syntax explicitly documented as one to avoid. You
> >> need here proper assignment, not NULL. Please don't use cleanup.h if you
> >> do not intend to follow it because it does not make the code simpler.
> > Hello and thank you for your feedback!
> > 
> > I have used __free here to match a previous comment from Ilpo:
> > https://lore.kernel.org/all/25bd0c90-2de0-ef66-c18d-661180b71fd4@linux.intel.com/
> > and I figured that since this is the same exact pattern as that it would have made
> > sense to use it.
> > 
> > May I ask you to elaborate further please? If there is a more effective way to take
> > advantage of cleanup.h I will very much consider it.
> 
> Just read the doc before using it. It is very, very clear what is the
> recommended syntax,

The documentation is in include/linux/cleanup.h itself (search for "NULL" 
to find the relevant part towards the end of the long comment).

And please still use __free(), we just want to avoid using its 
anti-patterns.

-- 
 i.

Re: [PATCH 1/3] platform/x86: asus-wmi: explicitly mark more code with CONFIG_ASUS_WMI_DEPRECATED_ATTRS

[Denis Benato]

On 12/29/25 11:06, Ilpo Järvinen wrote:
> On Thu, 25 Dec 2025, Denis Benato wrote:
>
>> Be more explicit in code that will be excluded when compiling
>> with CONFIG_ASUS_WMI_DEPRECATED_ATTRS disabled.
> Does this result in build fails or warnings? If that's the case, it should 
> be mentioned in the changelog + Fixes tag added.
Hi,

Sadly no, despite a property in a struct instantiated only once
being forcefully set to false no warning is emitted by neither
clang nor gcc :(
>> Signed-off-by: Denis Benato <denis.benato@linux.dev>
>> ---
>>  drivers/platform/x86/asus-wmi.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
>> index 4aec7ec69250..a49447eff4f4 100644
>> --- a/drivers/platform/x86/asus-wmi.c
>> +++ b/drivers/platform/x86/asus-wmi.c
>> @@ -302,7 +302,11 @@ struct asus_wmi {
>>  	u32 nv_temp_target;
>>  
>>  	u32 kbd_rgb_dev;
>> +
>> +#if IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS)
>>  	bool kbd_rgb_state_available;
>> +#endif /* IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS) */
>> +
>>  	bool oobe_state_available;
>>  
>>  	u8 throttle_thermal_policy_mode;
>> @@ -1060,6 +1064,7 @@ static const struct attribute_group kbd_rgb_mode_group = {
>>  };
>>  
>>  /* TUF Laptop Keyboard RGB State **********************************************/
>> +#if IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS)
>>  static ssize_t kbd_rgb_state_store(struct device *dev,
>>  				 struct device_attribute *attr,
>>  				 const char *buf, size_t count)
>> @@ -1106,6 +1111,8 @@ static const struct attribute_group kbd_rgb_state_group = {
>>  	.attrs = kbd_rgb_state_attrs,
>>  };
>>  
>> +#endif /* IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS) */
>> +
>>  static const struct attribute_group *kbd_rgb_mode_groups[] = {
>>  	NULL,
>>  	NULL,
>> @@ -1861,8 +1868,11 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
>>  
>>  	if (asus->kbd_rgb_dev)
>>  		kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_mode_group;
>> +
>> +#if IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS)
>>  	if (asus->kbd_rgb_state_available)
>>  		kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_state_group;
>> +#endif /* IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS) */
>>  
>>  	asus->led_workqueue = create_singlethread_workqueue("led_workqueue");
>>  	if (!asus->led_workqueue)
>>

Re: [PATCH 1/3] platform/x86: asus-wmi: explicitly mark more code with CONFIG_ASUS_WMI_DEPRECATED_ATTRS

[Ilpo Järvinen]

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

On Tue, 30 Dec 2025, Denis Benato wrote:
> On 12/29/25 11:06, Ilpo Järvinen wrote:
> > On Thu, 25 Dec 2025, Denis Benato wrote:
> >
> >> Be more explicit in code that will be excluded when compiling
> >> with CONFIG_ASUS_WMI_DEPRECATED_ATTRS disabled.
> > Does this result in build fails or warnings? If that's the case, it should 
> > be mentioned in the changelog + Fixes tag added.
> Hi,
> 
> Sadly no, despite a property in a struct instantiated only once
> being forcefully set to false no warning is emitted by neither
> clang nor gcc :(

Ah, I was mainly expecting unused function warning but apparently since it 
was still being "used" it obviously won't trigger that warning.

-- 
 i.

> >> Signed-off-by: Denis Benato <denis.benato@linux.dev>
> >> ---
> >>  drivers/platform/x86/asus-wmi.c | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >>
> >> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> >> index 4aec7ec69250..a49447eff4f4 100644
> >> --- a/drivers/platform/x86/asus-wmi.c
> >> +++ b/drivers/platform/x86/asus-wmi.c
> >> @@ -302,7 +302,11 @@ struct asus_wmi {
> >>  	u32 nv_temp_target;
> >>  
> >>  	u32 kbd_rgb_dev;
> >> +
> >> +#if IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS)
> >>  	bool kbd_rgb_state_available;
> >> +#endif /* IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS) */
> >> +
> >>  	bool oobe_state_available;
> >>  
> >>  	u8 throttle_thermal_policy_mode;
> >> @@ -1060,6 +1064,7 @@ static const struct attribute_group kbd_rgb_mode_group = {
> >>  };
> >>  
> >>  /* TUF Laptop Keyboard RGB State **********************************************/
> >> +#if IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS)
> >>  static ssize_t kbd_rgb_state_store(struct device *dev,
> >>  				 struct device_attribute *attr,
> >>  				 const char *buf, size_t count)
> >> @@ -1106,6 +1111,8 @@ static const struct attribute_group kbd_rgb_state_group = {
> >>  	.attrs = kbd_rgb_state_attrs,
> >>  };
> >>  
> >> +#endif /* IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS) */
> >> +
> >>  static const struct attribute_group *kbd_rgb_mode_groups[] = {
> >>  	NULL,
> >>  	NULL,
> >> @@ -1861,8 +1868,11 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
> >>  
> >>  	if (asus->kbd_rgb_dev)
> >>  		kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_mode_group;
> >> +
> >> +#if IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS)
> >>  	if (asus->kbd_rgb_state_available)
> >>  		kbd_rgb_mode_groups[num_rgb_groups++] = &kbd_rgb_state_group;
> >> +#endif /* IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS) */
> >>  
> >>  	asus->led_workqueue = create_singlethread_workqueue("led_workqueue");
> >>  	if (!asus->led_workqueue)
> >>
> 
5~

Re: [PATCH 1/3] platform/x86: asus-wmi: explicitly mark more code with CONFIG_ASUS_WMI_DEPRECATED_ATTRS

[Denis Benato]

On 12/30/25 14:41, Ilpo Järvinen wrote:
> On Tue, 30 Dec 2025, Denis Benato wrote:
>> On 12/29/25 11:06, Ilpo Järvinen wrote:
>>> On Thu, 25 Dec 2025, Denis Benato wrote:
>>>
>>>> Be more explicit in code that will be excluded when compiling
>>>> with CONFIG_ASUS_WMI_DEPRECATED_ATTRS disabled.
>>> Does this result in build fails or warnings? If that's the case, it should 
>>> be mentioned in the changelog + Fixes tag added.
>> Hi,
>>
>> Sadly no, despite a property in a struct instantiated only once
>> being forcefully set to false no warning is emitted by neither
>> clang nor gcc :(
> Ah, I was mainly expecting unused function warning but apparently since it 
> was still being "used" it obviously won't trigger that warning.
>
Plus if gcc/clang could have catched it build bots would have done so
when posting the asus-armoury series as they catched other things.