linux-kernel - Re: [PATCH 11/11] HID: asus: add RGB support to the ROG Ally units

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fed995c3-5591-43d3-ac82-dcb6fe1c0c61@ljones.dev>
Date: Sat, 22 Mar 2025 22:15:56 +1300
From: "Luke D. Jones" <luke@...nes.dev>
To: Antheas Kapenekakis <lkml@...heas.dev>
Cc: platform-driver-x86@...r.kernel.org, linux-input@...r.kernel.org,
 linux-kernel@...r.kernel.org, Jiri Kosina <jikos@...nel.org>,
 Benjamin Tissoires <bentiss@...nel.org>,
 Corentin Chary <corentin.chary@...il.com>,
 Hans de Goede <hdegoede@...hat.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Subject: Re: [PATCH 11/11] HID: asus: add RGB support to the ROG Ally units

On 22/03/25 20:56, Antheas Kapenekakis wrote:
> On Sat, 22 Mar 2025 at 03:30, Luke D. Jones <luke@...nes.dev> wrote:
>>
>> On 21/03/25 11:09, Antheas Kapenekakis wrote:
>>> Apply the RGB quirk to the QOG Ally units to enable basic RGB support.
>>>
>>> Signed-off-by: Antheas Kapenekakis <lkml@...heas.dev>
>>> ---
>>>    drivers/hid/hid-asus.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>>> index 5e87923b35520..589b32b508bbf 100644
>>> --- a/drivers/hid/hid-asus.c
>>> +++ b/drivers/hid/hid-asus.c
>>> @@ -1449,10 +1449,10 @@ static const struct hid_device_id asus_devices[] = {
>>>          QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_RGB },
>>>        { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>            USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY),
>>> -       QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>> +       QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_RGB },
>>>        { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>            USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY_X),
>>> -       QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>> +       QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_NKEY_RGB },
>>>        { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>            USB_DEVICE_ID_ASUSTEK_ROG_CLAYMORE_II_KEYBOARD),
>>>          QUIRK_ROG_CLAYMORE_II_KEYBOARD },
>>
>> I need to NACK this one sorry, if only because I added the RGB control
>> in hid-asus-ally as a per-LED control and it works very well. You'll see
>> it once I submit that series upstream again.
> 
> Depending on when your driver is ready to merge, it might be
> beneficial for this to merge ahead of time for some basic support.
> Then you can yank it after your driver is in. For your driver, I think
> it would be good to make it separate from hid-asus and yank ally
> completely from here.

The driver is fully standalone and that is what I do yes.

I do think it would be better for you to do the RGB part separate to the 
main lot of patches as those can definitely be signed off and merged a 
lot quicker. You still have the bazzite kernel right? I hope it's 
acceptable to carry just the RGB there for a tiny bit longer.

>> The distinction between MCU mode and Software mode for RGB is frankly a
>> pain in the arse. For Ally we will want software mode (per-led) as that
>> allows just one USB write for all LEDs, and no need to do a set/apply to
>> make the LEDs change. The benefit being that the LEDs can change rapidly
>> and there will be no "blink".
> 
> The blink happens when the B4(/B5) command is sent. If they are not,
> it is perfectly fine. B4 just needs to be sent initially, as it
> switches the controller to solid mode if it is not there already. Then
> B4/B5 could be sent on shutdown to save the color to memory. I
> purposely did not do it as it would break the driver if userspace
> controls the leds inbetween led switches and it is needlessly
> complicated for what this support is meant for (basic RGB).

Hmm, I thought the colour wasn't actually applied until at least a "set" 
was sent. Maybe it's on older devices.. I haven't looked at that for a 
while now.

> Also, multizone is expected to be of little utility, so it is not
> worth making concessions over. I never found a use for it or anyone
> ask for it. If single zone has performance benefits, it should be used
> instead. A lot of devices do not support multizone, and when they do,
> it is in different forms, so it is not something that can be
> intuitively put into a kernel driver imo.

Would you like to know how many varieties of single, multi, and per key 
there are? I have a rather large spread sheet tracking everything so 
far. Per-key + bars is something like 12-13 packets to send :|

> Aura mode is especially buggy during boot and resume, since the
> controller briefly switches from the MCU mode to that, so it uses a
> stale color which is ugly. Even when you try to match the colors, as
> you did, it is not 1-1. You know this too. In addition, Aura mode will
> break userspace Aura programs running through Wine. Although they are
> already broken due to the hiddevs merging which I had to revert for
> V2. But we could fix that, and I will try to for V3.

Aura programs can set the device back to MCU mode. Or they should be 
able to. The RGB setup is done only when called through the mcled class, 
and on suspend (to colour match and set static).

Cheers,
Luke.

>> I'll write more on patch 10
>>
>> Cheers,
>> Luke.