Message-ID: 
 <CAGwozwGwtivKnC6ucCGwJiu2yLLbddFXG+jYTaMTwU4Zt=CAfQ@mail.gmail.com>
Date: Thu, 13 Nov 2025 22:17:36 +0100
From: Antheas Kapenekakis <lkml@...heas.dev>
To: luke@...nes.dev
Cc: Denis Benato <benato.denis96@...il.com>,
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
	platform-driver-x86@...r.kernel.org, linux-input@...r.kernel.org,
	LKML <linux-kernel@...r.kernel.org>, Jiri Kosina <jikos@...nel.org>,
	Benjamin Tissoires <bentiss@...nel.org>,
 Corentin Chary <corentin.chary@...il.com>,
	Hans de Goede <hansg@...nel.org>
Subject: Re: [PATCH v8 00/10] HID: asus: Fix ASUS ROG Laptop's Keyboard
 backlight handling

On Thu, 13 Nov 2025 at 21:23, <luke@...nes.dev> wrote:
>
>
> > On 13 Nov 2025, at 21:44, Antheas Kapenekakis <lkml@...heas.dev> wrote:
> >
> > On Thu, 13 Nov 2025 at 02:14, Denis Benato <benato.denis96@...il.com> wrote:
> >>
> >>
> >> On 11/12/25 23:08, Antheas Kapenekakis wrote:
> >>> On Wed, 12 Nov 2025 at 20:51, Denis Benato <benato.denis96@...il.com> wrote:
> >>>>
> >>>> On 11/12/25 14:41, Antheas Kapenekakis wrote:
> >>>>> On Wed, 12 Nov 2025 at 14:22, Ilpo Järvinen
> >>>>> <ilpo.jarvinen@...ux.intel.com> wrote:
> >>>>>> On Wed, 12 Nov 2025, Antheas Kapenekakis wrote:
> >>>>>>
> >>>>>>> On Sat, 1 Nov 2025 at 11:47, Antheas Kapenekakis <lkml@...heas.dev> wrote:
> >>>>>>>> This is a two part series which does the following:
> >>>>>>>>  - Clean-up init sequence
> >>>>>>>>  - Unify backlight handling to happen under asus-wmi so that all Aura
> >>>>>>>>    devices have synced brightness controls and the backlight button works
> >>>>>>>>    properly when it is on a USB laptop keyboard instead of one w/ WMI.
> >>>>>>>>
> >>>>>>>> For more context, see cover letter of V1. Since V5, I removed some patches
> >>>>>>>> to make this easier to merge.
> >>>>>>> Small bump for this.
> >>>>>> I looked at v8 earlier but then got an impression some of Denis' comments
> >>>>>> against v7 were not taken into account in v8, which implies there will be
> >>>>>> delay until I've time to delve into the details (I need to understand
> >>>>>> things pretty deeply in such a case, which does take lots of time).
> >>>>>>
> >>>>>> Alternatively, if Denis says v8 is acceptable, then I don't need to spend
> >>>>>> so much time on it, but somehow I've a feeling he isn't happy with v8
> >>>>>> but just hasn't voiced it again...
> >>>>>>
> >>>>>> Please do realize that ignoring reviewer feedback without a very very good
> >>>>>> reason always risks adding delay or friction into getting things
> >>>>>> upstreamed. Especially, when the review comes from a person who has been
> >>>>>> around for me to learn to trust their reviews or from a maintainer of the
> >>>>>> code in question.
> >>>>> Sure, sorry if it came out this way. Dennis had two comments on the V7
> >>>>> version of the series.
> >>>>>
> >>>>> The first one was that asusctl has a hang bug, which he hasn't had
> >>>>> time to look into yet. This should have been fixed by dropping the
> >>>>> HID_QUIRK_INPUT_PER_APP. I retested the series and that QUIRK was a
> >>>>> bit of a NOOP that does not need to be added in the future.
> >>>> So it is supposed to not regress it now, correct?
> >>>>> The second is he is concerned with dropping the 0x5d/0x5e inits. Luke
> >>>>> said (back in March) that it is fine to drop 0x5e because it is only
> >>>>> used for ANIME displays. However, for 0x5d, it is hard to verify some
> >>>>> of the older laptops because they have only been tested with 0x5d and
> >>>>> we do not have the hardware in question to test.
> >>>>>
> >>>>> For this series, I re-added "initialize LED endpoint early for old
> >>>>> NKEY keyboards" that re-adds 0x5d for the keyboards that cannot be
> >>>>> tested again so this comment should be resolved too. With that in
> >>>>> mind, we do end up with an additional quirk/command that may be
> >>>>> unneeded and will remain there forever, but since it was a point of
> >>>>> contention, it is not worth arguing over.
> >>>>>
> >>>>> So both comments should be resolved
> >>>> The driver should also not late-initialize anything.
> >>>>
> >>>> Windows doesn't do it and the official asus application
> >>>> can freely send LEDs changing commands to either WMI or USB
> >>>> so I don't see any reason to do things differently [than windows]
> >>>> and not prepare every USB endpoint to receive commands,
> >>>> this has not been addressed unless I confused v7 and v8?
> >>> Yes, it's been added on v8. 0x5d is init for the laptops it is
> >>> problematic for. Not because it does not work, but because it has not
> >>> been verified to work for those laptops.
> >> I am not sure I am reading this right:
> >> are you telling me that on recent models the windows driver
> >> doesn't issue 0x5d?
> >
> > Try to add spaces in your replies. This is hard to follow.
> >
> > Do not conflate driver with software. 0x5a (over the application
> > 0xff310076) has traditionally been used by a driver in Windows to
> > control the backlight level, as it is done in this driver. 0x5d (over
> > the application 0xff310079) is only used by laptops with RGB by
> > Armoury crate. But this driver does not do RGB. No device
> > functionality relies on it being sent for any device I've seen. The
> > device remembers its Windows settings, incl. the backlight color, in
> > the absence of a driver.
> >
> > Laptops without RGB such as the Duo series which I would like to add
> > support for next only have a 0x5a endpoint. But, they are sent garbage
> > inits for 0x5d and 0x5e currently. This should be fixed.
> >
> > Moreso, it seems that Armoury crate on the Xbox Ally series uses
> > exclusively 0x5a commands and if you use 0x5d it ignores them (perhaps
> > RGB still works though). With the previous generation, commands worked
> > for either report id.
> >
> >>>>> @Denis: can give an ack if this is the case?
> >>>>>
> >>>>> As for Derek's comment, he has a PR for his project where he removes
> >>>>> the name match for Ally devices with ample time for it to be merged
> >>>>> until kernel 6.19 is released. In addition, that specific software for
> >>>>> full functionality relies on OOT drivers on the distros it ships with,
> >>>>> so it is minimally affected in either case.
> >>>> The part we are talking about depends on this driver (hid-asus)
> >>>> and there are people on asus-linux community using inputplumber
> >>>> for non-ally devices (the OOT driver is only for ally devices)
> >>>> therefore it is very important to us (and various other distributions)
> >>>> not to break that software in any way.
> >>> This driver is only used for Ally devices. If you mean that people
> >>> remap their keyboards using inputplumber I guess they could but I have
> >>> not seen it.
> >> I meant people remap keyboards using IP. I am sure there were
> >> (and very probably still are) people doing that.
> >>>> Weighting pros and cons of changing the name I am not sure
> >>>> changing name brings any benefit? Or am I missing something here?
> >>>> It's simply used by userspace so the hardware should be loading
> >>>> regardless of the name...
> >>> Users see the name of their devices in their settings menu. They
> >>> should be accurate. Also, the early entry needs to be added anyway to
> >>> prevent kicking devices.
> >> If it's just aesthetics I don't see much reasons in changing the name.
> >>
> >> "the early entry needs to be added anyway ...." has no meaning to me,
> >> please rephrase. Sorry.
> >
> > Early exit-
> >
> >>>> Along with IP and your tool and asusctl there is also openrgb,
> >>>> and a newborn application for asus devices (I don't have contacts
> >>>> with that dev nor I remember the name of the tool)
> >>>> and I am not even that sure these are all asus-related
> >>>> applications.
> >>> My tool never checked for names, it briefly did for around a month
> >>> after its creation for some devices until capability matches. Around
> >>> 6.1 and 6.7 the kernel changed the names of most USB devices and that
> >>> caused issues. It currently only uses name matches for VID/PID 0/0
> >>> devices created by the kernel. Specifically, WMI and AT Keyboards. I
> >>> am not sure there is a workaround for those. Asusctl also does not use
> >>> names either.
> >> But IP does, so I would like to hear confirmation from at least Derek
> >> before the merge that there won't be future issues.
> >>
> >> Interpret what I say here as a broad topic, not just name/PER_APP flag:
> >> avoid changing data flow on older models...
> >
> > In [1] Derek removes the name matches
> >
> > There are no other name matches concerning this driver in it.
> >
> > The data flow is not changed in this series; you should go through the
> > patches once again if you think that. The only difference is 0x5e is
> > not sent, and 0x5d is not sent for newer devices.
> >
> > [1] https://github.com/ShadowBlip/InputPlumber/pull/453
> >
> >>>> Excercise EXTRA care touching this area as these are enough changes
> >>>> to make it difficult to understand what exactly is the problem if
> >>>> someone shows up with LEDs malfunctioning, laptop not entering sleep
> >>>> anymore or something else entirely. Plus over time
> >>>> ASUS has used various workarounds for windows problems
> >>>> and I am not eager to find out what those are since there is only
> >>>> a realistic way it's going to happen....
> >>> These changes are not doing something extraordinary. It's just a minor cleanup.
> >>>
> >>>>> Moreover, that specific commit is needed for Xbox Ally devices anyway,
> >>>>> as the kernel kicks one of the RGB endpoints because it does not
> >>>>> register an input device (the check skipped by early return) so
> >>>>> userspace becomes unable to control RGB on a stock kernel
> >>>>> (hidraw/hiddev nodes are gone). For more context here, this specific
> >>>>> endpoint implements the RGB Lamparray protocol for Windows dynamic
> >>>>> lighting, and I think in an attempt to make it work properly in
> >>>>> Windows, Asus made it so Windows has to first disable dynamic lighting
> >>>>> for armoury crate RGB commands to work (the 0x5a ones over the 0xff31
> >>>>> hid page).
> >>>> Yes once ASUS introduces something new it sticks with that for
> >>>> future models so it's expected more and more laptops will have
> >>>> the same problem: I am not questioning if these patches are needed
> >>>> as they very clearly are; I am questioning if everything that these
> >>>> patches are doing are worth doing and aren't breaking/regressing
> >>>> either tools or the flow of actions between the EC and these USB devices.
> >>> Well, this series is needed to account for that. Sending the disable
> >>> command is out of scope for now though.
> >> Here I apologize for confusion: my comments were mostly about
> >> older models: I absolutely don't want to break those, but if you find a way
> >> to distinguish them from newer models that would give you more freedom with those.
> >
> > Yes, we know their specific PIDs, so if you see the patch that adds
> > the 0x5d init, it is only added for those models.
>
> I’m only half keeping up to date on this. I do recall however that the 0x5D init was definitely required for the first ASUS laptop I worked on, and old GX502 - the PID for keyboard is 0x1866 and I think that was the last of that generation MCU. All the previous MCU also required it.

You recall if it was needed to enable the RGB commands or was it only
for keyboard shortcuts? If it is needed for keyboard shortcuts it is
correct for it to stay. If RGB does not turn on where it has been
enabled before, it should also stay.

> I saw some messages in perhaps another thread where it was mentioned that 0x5E init should be removed? That I agreed with that?
> I know there are AniMe and Slash versions that use that init, and they are on the same MCU as the keyboard. I had expected that just one init (on 0x5A or whatever) would work but it doesn’t - what I don’t recall is if an incomplete init affected the keyboard features.

Well, the way these devices work is that there are three hiddev
devices, usually nested within the same hid endpoint under up to three
collections. Each has one report ID. 0x5a is for brightness controls,
0x5d is for RGB, and 0x5e is for anime. For the first two, I know the
usages are 76 and 79 (see above). I am not sure what the usage for
anime is because I do not have a hid descriptor for that device.

In order to start talking to one of the hiddev devices, you are
supposed to start with an init. The init is bidirectional, so after
reading it back software knows it is talking to an Asus device (as it
is done in this series). Likewise, even though it is not the case for
all MCUs, the MCUs themselves can use it to verify they are talking to
an Asus application (as you said) and reject commands if it is not
sent.

For this reason, I think it is a good idea before asusctl starts
controlling RGB, to always start with a 0x5d init to verify it is
talking to an Asus device. And before Anime, with a 0x5e init (if the
specific application for it is available). So since Dennis you are the
new maintainer, you should try to work that in. Sending it twice does
not hurt, even if not ideal.

Similarly, because this driver does not do Anime currently, there is
no reason for it to send 0x5e. It also does not do RGB, so there is no
reason to send 0x5D (unless not sending it causes issues). For the RGB
patch I did, I delayed the init purposely until userspace interacts
with the sysfs RGB endpoint, partly to interfere with userspace
software less as well. So if the user does not use the sysfs RGB e.g.
asusctl is completely unaffected.

> In all reality unless the full set of init is causing issues it’s best to leave them in. If it is then I guess this driver is going to become a little more complex and have a few more quirks.
>
> Unfortunately I didn’t keep good records of my findings on this so it’s just my remembered observations that you’ll have to take at my word.
>
> It would be a good idea for you both to perhaps collaborate with Sergei from ghelper, he has put a huge amount of effort in to that tool and due to it being windows he gets a hell of a lot more use and bug reports/data than this driver does. There’s no shame in looking to others for inspiration, ideas, or guidance.

Good idea. From a quick look, indeed slash/anime is 0x5e. We could
interact with him more in the future.

Although looking into it, to find the correct endpoint he does a dirty
check for report length being more than 128[1]. SIgh

I think it would be productive to try to merge this for 6.19. So
Dennis can you try to be a bit more cooperative?

I already have 6 more patches for duo keyboards. Although the keyboard
brightness button on those seems to not work (?)[2]. I am waiting on a
reply for that. Perhaps it uses a slightly different ID code. However,
it seems that brightness works even when disconnecting and connecting
the keyboard. Which is great.

Antheas

[1] https://github.com/seerge/g-helper/blob/610b11749b4da97346012e5d47f0a9bbc93b94af/app/AnimeMatrix/Communication/Platform/WindowsUsbProvider.cs#L37
[2] https://github.com/bazzite-org/kernel-bazzite/issues/35

> Cheers,
> Luke.
>
> >
> >> No disable commands unless we find hard evidence those are strictly needed.
> >
> > They are needed for the Xbox Ally series, but since this driver does
> > not do RGB it is out of scope.
> >
> >>> Antheas
> >>>
> >>>>> Hopefully this clears things up
> >>>>>
> >>>>> Antheas
> >>>>>
> >>>>>>> Unrelated but I was b4ing this series on Ubuntu 24 and got BADSIG:
> >>>>>>> DKIM/antheas.dev. Is there a reference for fixing this on my host?
> >>>>>>> Perhaps it would help with spam
> >>>>>> I see BADSIG very often these days from b4 (thanks to gmail expiring
> >>>>>> things after 7 days or so, I recall hearing somewhere), I just ignore them
> >>>>>> entirely.
> >>>>>>
> >>>>>> AFAIK, that has never caused any delay to any patch in pdx86 domain so if
> >>>>>> that is what you thought is happening here, it's not the case.
> >>>>>> If your patch does appear in the pdx86 patchwork, there's even less reason
> >>>>>> to worry as I mostly pick patches to process using patchwork's list.
> >>>>> Turns out I had to update my DNS records. It should be good now.
> >>>>>
> >>>>>> --
> >>>>>> i.
> >>>> snipp
> >>>>>>>> 2.51.2
>
>
>

Powered by blists - more mailing lists