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 for Android: free password hash cracker in your pocket

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

Message-ID: 
 <CAGwozwGMNchNXckSZ=HPyi=sFEjrLDzayqjHDSOwwb8MQ=rJPg@mail.gmail.com>
Date: Sat, 22 Mar 2025 08:56:20 +0100
From: Antheas Kapenekakis <lkml@...heas.dev>
To: "Luke D. Jones" <luke@...nes.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 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 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).

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.

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.

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