| Message ID | 20250320220924.5023-12-lkml@antheas.dev (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | HID: Asus: Add RGB Support to Asus Z13, Ally, unify backlight asus-wmi, and Z13 QOL | expand |
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@antheas.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. 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". I'll write more on patch 10 Cheers, Luke.
On Sat, 22 Mar 2025 at 03:30, Luke D. Jones <luke@ljones.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@antheas.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.
On 22/03/25 20:56, Antheas Kapenekakis wrote: > On Sat, 22 Mar 2025 at 03:30, Luke D. Jones <luke@ljones.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@antheas.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.
On Sat, 22 Mar 2025 at 10:53, Luke D. Jones <luke@ljones.dev> wrote: > > On 22/03/25 20:56, Antheas Kapenekakis wrote: > > On Sat, 22 Mar 2025 at 03:30, Luke D. Jones <luke@ljones.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@antheas.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. 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. >
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 },
Apply the RGB quirk to the QOG Ally units to enable basic RGB support. Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> --- drivers/hid/hid-asus.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)