On Sat, 22 Mar 2025 at 10:53, Luke D. Jones <luke@xxxxxxxxxx> wrote:
>
> On 22/03/25 20:56, Antheas Kapenekakis wrote:
> > On Sat, 22 Mar 2025 at 03:30, Luke D. Jones <luke@xxxxxxxxxx> 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@xxxxxxxxxxx>
> >>> ---
> >>> 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.
Sure, I will keep it for v3 as that is almost done now but afaik it
does not have to merge together.
> >> 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.
I would have to recheck, but I am pretty sure that as long as you are
in the solid mode, color changes apply instantly. There are no flashes
on my end.
> > 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 :|
I think when it comes to the kernel, doing just a solid color would go
a long way.
> > 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).
I recall one of the previous versions of your patch doing a dirty
brightness set during the controller init. If you fix that it should
be alright.
Antheas
> Cheers,
> Luke.
>
> >> I'll write more on patch 10
> >>
> >> Cheers,
> >> Luke.
>
