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

[Denis Benato]

Il 8 ottobre 2026 14:48:31 CEST, "Ilpo Järvinen" <ilpo.jarvinen@xxxxxxxxxxxxxxx> ha scritto:
>On Sat, 26 Sep 2026, Denis Benato wrote:
>> 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 something is not there, -ENODEV is appropriate.
>
>Unexpected comms is -EIO (-EINVAL is unfortunately often misused for 
>this but it -EINVAL is to say input parameter was wrong).
>
>-ENOTSUPP is not a standard error code (the correct one would be 
>-EOPNOTSUPP).
>
>TBH, I don't really know where the line between -ENODEV and -EOPNOTSUPP 
>is. I'd personally use the latter mostly for the case where software side 
>lacks something.
>
>In anycase, wrong errno's are endemic and hard to fix without running 
>afoul with something as they often relate also to userspace ABIs.
>

-ENODEV it is.... Or -ENODATA when we can write but not read back the status?


>> 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:
>
>Name literals with defines. When it comes to offsets, consider if a 
>struct would be viable instead of byte array + named define index.
>
>> > +		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?
>
>I'm not entirely sure what's the suggestion.
>

1 sysfs attribute for read and don't touch the write one since they are in two different formats.

Like one returns 4 values and one accepts 5: they are asymmetric hence my suggestion.

>> > +}
>> >  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)));
>
>These BIT(x) should be named with defines as you clearly know what they 
>mean (I assume the _store ones too match to these so do the addition and 
>conversion in own patch).
>
>A comment like the one above is almost always an indication of a naming 
>problem that, after fixed, makes the comment totally redundant.
>
>We try to leave comments for something that is tricky, non-intuitive, or
>complex.
>
>> > +}
>> >  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
>> >  
>> 
>