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

[Denis Benato]

On 10/20/26 15:08, Ilpo Järvinen wrote:
> On Thu, 8 Oct 2026, Denis Benato wrote:
>
>> 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.
> 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 didn't think of this... Well since no software currently exists that attempts to read it back
I guess we can do pretty much everything we want, so if you say n/a is acceptable that
goes for me.

Thanks for the input,
Denis B.
> --
>  i.