Message-ID: <08e4df7e-47c3-4744-a53c-c098eba6f61b@ljones.dev>
Date: Fri, 21 Mar 2025 16:28:54 +1300
From: "Luke D. Jones" <luke@...nes.dev>
To: Antheas Kapenekakis <lkml@...heas.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 21/03/25 13:23, Antheas Kapenekakis wrote:
> 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.

Fair enough mate, know how that feels.

>> 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.

Personally I'd like to see that. I intended to do something similar last 
year but just never got a round to it. I'd rather move everything that's 
logical to move, from userspace to kernel. Custom ordering would be 
great to see, a logical profile progression etc, lets get this series 
landed first.

> 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

About that - I'm working on it also, as it needs to tie in with the PPT 
stuff, plus fan-curves. I'll keep you informed. The recent asus-armoury 
driver was a first step for it as I needed to get the PPT part in. Once 
it lands I'll submit work on the platform_profile stuff including custom 
profile.

> 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.

Don't spread yourself too thin. I can take care of most of the (new 
feature) asus stuff (that i have hardware for testing with) leaving you 
to focus on other things if you need. Anyhow, I'll keep you in sync - 
just be aware I'll be a little slow due to work and family demands.

> 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.

Ack

Cheers,
Luke.

> 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