Re: [PATCH v2 2/2] platform/x86: asus-wmi: add TUF keyboard RGB readback support

[Denis Benato]

On 9/26/26 02:56, Bartu Alev wrote:
> TUF Gaming laptops expose kbd_rgb_mode and kbd_rgb_state as write-only
> attributes (DEVICE_ATTR_WO), preventing userspace from querying the
> active hardware configuration.
>
> Add readback support by querying ASUS_WMI_DEVID_TUF_RGB_READBACK
> (0x0010005B) via the WMI DSTS method. On supported platforms this
> evaluates the DSDT method EC0.KBLS(), which returns a 16-byte buffer
> containing the active lighting mode, RGB color channels, effect speed
> and power-state flags.
>
> Introduce kbd_rgb_read_status() to evaluate and validate the buffer,
> and convert both attributes to DEVICE_ATTR_RW. Map the hardware speed
> codes (0xe1, 0xeb, 0xf5) to their sysfs indices (0, 1, 2).
>
> The command field is not part of the status buffer: "immediate vs
> save-to-flash" is a property of the write verb (0xb3/0xb4), not of
> readable state, and the EC mirror is updated identically by both.
> Readback therefore emits a synthetic leading '1' - the canonical
> input form userspace writes - so that output matches input.
>
> Suggested-by: Denis Benato <denis.benato@xxxxxxxxx>
> Signed-off-by: Bartu Alev <bartualev@xxxxxxxxx>
> ---
>  drivers/platform/x86/asus-wmi.c            | 79 +++++++++++++++++++++-
>  include/linux/platform_data/x86/asus-wmi.h |  3 +
>  2 files changed, 80 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index db6ee1974838..fe1dcc7701ad 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -1046,7 +1046,58 @@ static ssize_t gpu_mux_mode_store(struct device *dev,
>  static DEVICE_ATTR_RW(gpu_mux_mode);
>  #endif /* IS_ENABLED(CONFIG_ASUS_WMI_DEPRECATED_ATTRS) */
>  
> +static int kbd_rgb_read_status(u8 data[16])
> +{
> +	int err;
> +
> +	err = asus_wmi_evaluate_method_buf(ASUS_WMI_METHODID_DSTS,
> +					   ASUS_WMI_DEVID_TUF_RGB_READBACK,
> +					   0, data, 16);
> +
> +	if (err)
> +		return err < 0 ? err : -ENODEV;
> +
> +	/* DUBF[0] is a constant 1 set by the AML: anything else is not KBLS */
> +	if (data[0] != 1)
> +		return -ENODEV;
> +

-ENODEV or -ENOTSUPP ? Which one is better suited for these kind of things?

If we go with two separate sysfs attrs you don't register the read one,
otherwise I am not sure.

> +	return 0;
> +}
> +
>  /* TUF Laptop Keyboard RGB Modes **********************************************/
> +static ssize_t kbd_rgb_mode_show(struct device *dev,
> +				 struct device_attribute *attr,
> +				 char *buf)
> +{
> +	u8 data[16] = {};
> +	u32 speed;
> +	int err;
> +
> +	err = kbd_rgb_read_status(data);
> +	if (err)
> +		return err;
> +
> +	/* Map hardware speed codes back to sysfs index:
> +	 * 0xe1 -> 0 (slow), 0xeb -> 1 (normal), 0xf5 -> 2 (fast)
> +	 */
> +	switch (data[5]) {
> +	case 0xe1:
> +		speed = 0;
> +		break;
> +	case 0xeb:
> +		speed = 1;
> +		break;
> +	case 0xf5:
> +		speed = 2;
> +		break;
> +	default:
> +		speed = 1;
> +		break;
> +	}
> +
> +	return sysfs_emit(buf, "1 %d %d %d %d %d\n",
> +			  data[1], data[2], data[3], data[4], speed);

We had this discussion in discord so I want to update everyone reading:
the status returned is the current one and both cmd=0 and cmd=1 on write
update the current status.

Therefore this is an asymmetry that doesn't really need to be,
what if we introduce another sysfs that is RO? Ilpo?

> +}
>  static ssize_t kbd_rgb_mode_store(struct device *dev,
>  				 struct device_attribute *attr,
>  				 const char *buf, size_t count)
> @@ -1099,7 +1150,7 @@ static ssize_t kbd_rgb_mode_store(struct device *dev,
>  
>  	return count;
>  }
> -static DEVICE_ATTR_WO(kbd_rgb_mode);
> +static DEVICE_ATTR_RW(kbd_rgb_mode);
>  
>  static DEVICE_STRING_ATTR_RO(kbd_rgb_mode_index, 0444,
>  			     "cmd mode red green blue speed");
> @@ -1115,6 +1166,30 @@ static const struct attribute_group kbd_rgb_mode_group = {
>  };
>  
>  /* TUF Laptop Keyboard RGB State **********************************************/
> +static ssize_t kbd_rgb_state_show(struct device *dev,
> +				  struct device_attribute *attr,
> +				  char *buf)
> +{
> +	u8 data[16] = {};
> +	u8 flags;
> +	int err;
> +
> +	err = kbd_rgb_read_status(data);
> +	if (err)
> +		return err;
> +
> +	/*
> +	 * data[6] power-state bitmask:
> +	 * BIT(1) boot, BIT(3) awake, BIT(5) sleep, BIT(7) shutdown
> +	 */
> +	flags = data[6];
> +
> +	return sysfs_emit(buf, "1 %d %d %d %d\n",
> +			  !!(flags & BIT(1)),
> +			  !!(flags & BIT(3)),
> +			  !!(flags & BIT(5)),
> +			  !!(flags & BIT(7)));
> +}
>  static ssize_t kbd_rgb_state_store(struct device *dev,
>  				 struct device_attribute *attr,
>  				 const char *buf, size_t count)
> @@ -1146,7 +1221,7 @@ static ssize_t kbd_rgb_state_store(struct device *dev,
>  
>  	return count;
>  }
> -static DEVICE_ATTR_WO(kbd_rgb_state);
> +static DEVICE_ATTR_RW(kbd_rgb_state);
>  
>  static DEVICE_STRING_ATTR_RO(kbd_rgb_state_index, 0444,
>  			     "cmd boot awake sleep shutdown");
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index b5ed8c83ace1..1447c7f354bc 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -161,6 +161,9 @@
>  /* TUF laptop RGB power/state */
>  #define ASUS_WMI_DEVID_TUF_RGB_STATE	0x00100057
>  

The pre-existing one should probably be renamed to make clear
it's write only and it is a command... In its own patch.

ASUS_WMI_DEVID_TUF_RGB_CMD probably?

> +/* TUF laptop RGB keyboard status readback*/
> +#define ASUS_WMI_DEVID_TUF_RGB_READBACK	0x0010005B
> +

ASUS_WMI_DEVID_TUF_RGB_READ_STATUS ?


>  /* Bootup sound control */
>  #define ASUS_WMI_DEVID_BOOT_SOUND	0x00130022
>