Message-ID: 
 <CAGwozwF4BwRapZ2O0UR5-RyrdrO=_r29hWkSO5xEWc0aoKxJWA@mail.gmail.com>
Date: Fri, 21 Mar 2025 01:23:13 +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 00/11] HID: Asus: Add RGB Support to Asus Z13, Ally, unify
 backlight asus-wmi, and Z13 QOL

On Fri, 21 Mar 2025 at 01:03, Luke D. Jones <luke@...nes.dev> wrote:
>
> On 21/03/25 11:09, Antheas Kapenekakis wrote:
> > This is a three part series which does the following:
> >    - Clean init sequence, fix the keyboard of the Z13 (touchpad,fan button)
> >    - Unifies 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.
> >    - Adds RGB support to hid-asus, solid colors only, and for the ROG Ally
> >      units and the Asus Z13 2025 first.
> >
> > For context, see cover letter of V1.
> >
> > The last two patches are still a bit experimental, the rest is getting to
> > be pretty stable by now. I will test my Ally in the weekend. Also, I am
> > not a fan of the asus-0003:0B05:1A30.0001-led name, so suggestions would
> > be appreciated.
> >
> > ---
> > V1: https://lore.kernel.org/all/20250319191320.10092-1-lkml@antheas.dev/
>
> Hi Antheas, just a very quick note before I review - did you forget to
> add `-v2` to git format-patch? Don't do it now, it's just a reminder for
> next version.

Yes I did. It's been a long day.

> Also I guess asus_brt_ means asus_bright_, but maybe we can rename to
> asus_led_ or even asus_rgbw_? I think something a tad more descriptive
> while still keeping the length down would help future contributors
> quickly understand intent. I'll mention it again when I get to that
> actual patch during test and review.

I am not particularly happy with brt either, I chose it because the
events are named BRT. rgbw is a bit misleading, the notifier will
never do RGB. Since Aura devices can hotplug they need their own led
device. It will always passthrough brightness only. Perhaps led is
misleading for that reason as well. Bright seems a bit weird, and
brightness seems a bit long. So I am a bit stuck

What I think would be better is to refocus from leds, to maybe
hid_ref. As I'd like the fan key to passthrough to asus-wmi too to
cycle the profiles. I'd also like to tweak the profile cycling
behavior a bit and make it customizable. But very minor changes, just
to cycling behavior. Essentially, I want to get to a point where doing
Fn+Fan cycles the profiles properly without userspace, and then
userspace can take over the cycler and update the KDE slider live.

However, that refactor on 6.14 for platform profiles was brutal. So I
have to wait for fedora to get on 6.14 for me to even start thinking
about that. Otherwise I will not be able to deploy any changes on
Bazzite. I currently carry 287 patches (~100 is XDNA+NTSYNC+platform
profiles minus Kurt's series) for 6.13. I would also like try to
target Thinkpads too, and maybe the Legion Go with that, but it
depends on how much progress Derek makes on his driver by then.

As far as the Z13 is concerned, it will be this patch series + 1-3
patches to tweak the ROG button on the side to stop acting like a wifi
killswitch.

Antheas

> Looks like good progress so far.
>
> Cheers,
> Luke.
>
> > Changes since V1:
> >    - Add basic RGB support on hid-asus, (Z13/Ally) tested in KDE/Z13
> >    - Fix ifdef else having an invalid signature (reported by kernel robot)
> >    - Restore input arguments to init and keyboard function so they can
> >      be re-used for RGB controls.
> >    - Remove Z13 delay (it did not work to fix the touchpad) and replace it
> >      with a HID_GROUP_GENERIC quirk to allow hid-multitouch to load. Squash
> >      keyboard rename into it.
> >    - Unregister brightness listener before removing work queue to avoid
> >      a race condition causing corruption
> >    - Remove spurious mutex unlock in asus_brt_event
> >    - Place mutex lock in kbd_led_set after LED_UNREGISTERING check to avoid
> >      relocking the mutex and causing a deadlock when unregistering leds
> >    - Add extra check during unregistering to avoid calling unregister when
> >      no led device is registered.
> >    - Temporarily HID_QUIRK_INPUT_PER_APP from the ROG endpoint as it causes
> >      the driver to create 4 RGB handlers per device. I also suspect some
> >      extra events sneak through (KDE had the @@@@@@).
> >
> > Antheas Kapenekakis (11):
> >    HID: asus: refactor init sequence per spec
> >    HID: asus: prevent binding to all HID devices on ROG
> >    HID: asus: add Asus Z13 2025 Fan key
> >    HID: Asus: add Z13 folio to generic group for multitouch to work
> >    platform/x86: asus-wmi: Add support for multiple kbd RGB handlers
> >    HID: asus: listen to the asus-wmi brightness device instead of
> >      creating one
> >    platform/x86: asus-wmi: remove unused keyboard backlight quirk
> >    platform/x86: asus-wmi: add keyboard brightness event handler
> >    HID: asus: add support for the asus-wmi brightness handler
> >    HID: asus: add basic RGB support
> >    HID: asus: add RGB support to the ROG Ally units
> >
> >   drivers/hid/hid-asus.c                     | 342 ++++++++++++++++-----
> >   drivers/hid/hid-ids.h                      |   2 +-
> >   drivers/platform/x86/asus-wmi.c            | 138 ++++++++-
> >   include/linux/platform_data/x86/asus-wmi.h |  67 ++--
> >   4 files changed, 411 insertions(+), 138 deletions(-)
> >
> >
> > base-commit: 4701f33a10702d5fc577c32434eb62adde0a1ae1
>

Powered by blists - more mailing lists