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

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

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

On Thu, 8 Oct 2026, Denis Benato wrote:

> Il 8 ottobre 2026 14:48:31 CEST, "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com> 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@linux.dev>
> >> > Signed-off-by: Bartu Alev <bartualev@gmail.com>
> >> > ---
> >> >  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.

Hi again,

Does it have to return "1" or even a number? Could we just have return 
something like "n/a" in that field (which would be ~ "not available")?

--
 i.