Re: [PATCH 2/6] platform/x86/uniwill: Handle more WMI events required for TUXEDO devices

[Armin Wolf]

Am 20.11.25 um 23:06 schrieb Werner Sembach:

> Am 20.11.25 um 14:40 schrieb Armin Wolf:
> > Am 20.11.25 um 11:42 schrieb Werner Sembach:
> >
> > > Am 20.11.25 um 01:53 schrieb Armin Wolf:
> > > > Am 18.11.25 um 16:05 schrieb Werner Sembach:
> > > >
> > > > > Am 18.11.25 um 15:41 schrieb Armin Wolf:
> > > > > > Am 18.11.25 um 15:27 schrieb Werner Sembach:
> > > > > >
> > > > > > > Am 18.11.25 um 14:48 schrieb Armin Wolf:
> > > > > > > > Am 18.11.25 um 14:29 schrieb Werner Sembach:
> > > > > > > >
> > > > > > > > > Am 18.11.25 um 14:12 schrieb Armin Wolf:
> > > > > > > > > > Am 18.11.25 um 13:45 schrieb Werner Sembach:
> > > > > > > > > >
> > > > > > > > > > > Am 18.11.25 um 12:08 schrieb Armin Wolf:
> > > > > > > > > > > > Am 17.11.25 um 14:23 schrieb Werner Sembach:
> > > > > > > > > > > >
> > > > > > > > > > > > > Handle some more WMI events that are triggered on TUXEDO 
> > > > > > > > > > > > > devices.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Signed-off-by: Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx>
> > > > > > > > > > > > > ---
> > > > > > > > > > > > > drivers/platform/x86/uniwill/uniwill-acpi.c | 19 
> > > > > > > > > > > > > ++++++++++++++++++-
> > > > > > > > > > > > > drivers/platform/x86/uniwill/uniwill-wmi.h | 2 ++
> > > > > > > > > > > > >   2 files changed, 20 insertions(+), 1 deletion(-)
> > > > > > > > > > > > >
> > > > > > > > > > > > > diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c 
> > > > > > > > > > > > > b/drivers/platform/x86/uniwill/uniwill-acpi.c
> > > > > > > > > > > > > index 29bb3709bfcc8..0cb86a701b2e1 100644
> > > > > > > > > > > > > --- a/drivers/platform/x86/uniwill/uniwill-acpi.c
> > > > > > > > > > > > > +++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
> > > > > > > > > > > > > @@ -371,9 +371,11 @@ static const struct key_entry 
> > > > > > > > > > > > > uniwill_keymap[] = {
> > > > > > > > > > > > >         /* Reported in manual mode when toggling the 
> > > > > > > > > > > > > airplane mode status */
> > > > > > > > > > > > >       { KE_KEY,       UNIWILL_OSD_RFKILL, { KEY_RFKILL }},
> > > > > > > > > > > > > +    { KE_IGNORE,    UNIWILL_OSD_RADIOON, { KEY_UNKNOWN }},
> > > > > > > > > > > > > +    { KE_IGNORE,    UNIWILL_OSD_RADIOOFF, { KEY_UNKNOWN }},
> > > > > > > > > > > > >         /* Reported when user wants to cycle the platform 
> > > > > > > > > > > > > profile */
> > > > > > > > > > > > > -    { KE_IGNORE, UNIWILL_OSD_PERFORMANCE_MODE_TOGGLE, { 
> > > > > > > > > > > > > KEY_UNKNOWN }},
> > > > > > > > > > > > > +    { KE_KEY, UNIWILL_OSD_PERFORMANCE_MODE_TOGGLE, { 
> > > > > > > > > > > > > KEY_F14 }},
> > > > > > > > > > > >
> > > > > > > > > > > > I am currently working a patch adding platform profile 
> > > > > > > > > > > > support, so this event would
> > > > > > > > > > > > be handled inside the kernel on models with platform 
> > > > > > > > > > > > profile support.
> > > > > > > > > > >
> > > > > > > > > > > For tuxedo devices we have profiles managed in userspace 
> > > > > > > > > > > that do additional things. So we need a way to handle this 
> > > > > > > > > > > in userspace.
> > > > > > > > > > Do these things have something to do with the uniwill EC? If 
> > > > > > > > > > so then we should implement those inside the driver
> > > > > > > > > > itself. The control center can then poll the platform profile 
> > > > > > > > > > sysfs file to get notified when platform_profile_cycle()
> > > > > > > > > > is executed to perform additional actions.
> > > > > > > > > Not exclusively, e.g. one thing is display brightness.
> > > > > > > >
> > > > > > > > And you cannot poll the sysfs interface?
> > > > > > > I can't follow you atm?
> > > > > >
> > > > > > I meant to ask whether or not your application could poll the 
> > > > > > platform profile sysfs interface for changes instead of
> > > > > > listing for the F14 key.
> > > > > But the platform profiles are a fixed number? TCC currently allows 
> > > > > an arbitrary amount of profiles being created.
> > > >
> > > > With "poll the platform profile sysfs interface" i meant that you 
> > > > could use poll() (https://linux.die.net/man/2/poll)
> > > > or epoll() on the sysfs file containing the current platform profile.
> > > Sorry i think i still don't completely get what you mean with 
> > > platform profile. I assume you have a poc on github? If not can you 
> > > give me a short overview?
> >
> > Example code, might not work:
> >
> > from select import poll, POLLPRI
> >
> > fd = open("|/sys/firmware/acpi/platform_profile", "r") p = poll() 
> > p.register(fd.fileno(), POLLPRI) # Wait till platform profile changes 
> > p.poll() print("Platform profile changed") This however comes with 
> > the drawback that you cannot prevent the platform profile from 
> > cycling. If you want to do that manually depending on the settings 
> > inside your custom profiles, then maybe we can keep the F14 hack for 
> > now. I will then add a module option when adding platform profile 
> > support to select between platform_profile_cycle() and the F14 
> > keycode. Does this sound OK?|
>
> a sorry i was imprecise, i wanted to know the kernelspace implementation.
Take a look at https://github.com/Wer-Wolf/uniwill-laptop/tree/platform_profile for the prototype.

The function platform_profile_cycle() is defined inside drivers/acpi/platform_profile.

> But let me sum up what i think you mean:
>
> Platform profiles are in driver predefined profiles like: Power Save, 
> Balanced, Performance, and Custom.
>
> When you press the button you want to cycle through the profiles 
> (except custom I guess?).
>
> Only in Custom things like cTGP can be directly controlled by 
> userspace via sysfs (otherwise the sysfs value is ignored?)
Correct.

> Maybe an elegant solution would be that upon boot for example 
> "Balanced" is selected and when being in one of the predefined 
> profiles the button cycles them. But once Custom get selected via 
> sysfs the button starts sending a button press as the driver now 
> expects everything to be handled by userspace. Bonus points if 
> userspace can read out what the predefined profiles actually set to, 
> for example, use that as initialization for custom profiles.
Agreed, i will implement this behavior once my testers give me feedback.

Thanks,
Armin Wolf

> > > > Anyway, i attached the patch with the device descriptor 
> > > > infrastructure. The callback called during probe cannot modify
> > > > the feature bitmap anymore, but i suggest that you simply set the 
> > > > limit for cTGP to zero. The code responsible for
> > > > initializing cTGP support can then check if the cTGP limit is zero 
> > > > and return early.
> > >
> > > I wonder if we should directly put that into a formal quirk list. 
> > > Opinions?
> > >
> > > Best regards,
> > >
> > > Werner
> > The problem is that the quirk list will become RO before the driver 
> > can access the EC, so we have to use uniwill_data
> > for storing this information.
> >
> > Thanks,
> > Armin Wolf
> >
> > > > Thanks,
> > > > Armin Wolf
> > > >
> > > > > > Thanks,
> > > > > > Armin Wolf
> > > > > >
> > > > > > > > > > > The 2 things I can spontaneously think of would be a sysfs 
> > > > > > > > > > > toggle or 2 different UNIWILL_FEATURE_* defines.
> > > > > > > > > > TPH i would love to have an ordinary keycode allocated for 
> > > > > > > > > > that if the above does not work for you. There already
> > > > > > > > > > exists KEY_PERFORMANCE, so adding something like 
> > > > > > > > > > KEY_PERFORMANCE_CYCLE should be possible.
> > > > > > > > >
> > > > > > > > > New keycodes won't work on X11, I don't know the reason, but 
> > > > > > > > > X11 only supports a max of 248 keycodes
> > > > > > > > >
> > > > > > > > > That's why for example touchpad toggle is bound to F21 e.g. 
> > > > > > > > > here 
> > > > > > > > > https://elixir.bootlin.com/linux/v6.17.8/source/drivers/platform/x86/lg-laptop.c#L106 
> > > > > > > > > .
> > > > > > > > Oh no. In this case using F14 is fine.
> > > > > > > >
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Armin Wolf
> > > > > > > >
> > > > > > > > > > > > >         /* Reported when the user wants to adjust the 
> > > > > > > > > > > > > brightness of the keyboard */
> > > > > > > > > > > > >       { KE_KEY, UNIWILL_OSD_KBDILLUMDOWN, { 
> > > > > > > > > > > > > KEY_KBDILLUMDOWN }},
> > > > > > > > > > > > > @@ -382,11 +384,19 @@ static const struct key_entry 
> > > > > > > > > > > > > uniwill_keymap[] = {
> > > > > > > > > > > > >       /* Reported when the user wants to toggle the 
> > > > > > > > > > > > > microphone mute status */
> > > > > > > > > > > > >       { KE_KEY, UNIWILL_OSD_MIC_MUTE, { KEY_MICMUTE }},
> > > > > > > > > > > > >   +    /* Reported when the user wants to toggle the mute 
> > > > > > > > > > > > > status */
> > > > > > > > > > > > > +    { KE_IGNORE,    UNIWILL_OSD_MUTE, { KEY_MUTE }},
> > > > > > > > > > > >
> > > > > > > > > > > > Why is this event being ignored?
> > > > > > > > > > > Because the UNIWILL_OSD_MUTE event is sent in addition to 
> > > > > > > > > > > the mute key event, so not ignoring it here would result in 
> > > > > > > > > > > a double trigger.
> > > > > > > > > >
> > > > > > > > > > I understand.
> > > > > > > > > >
> > > > > > > > > > > > > +
> > > > > > > > > > > > >       /* Reported when the user locks/unlocks the Fn key */
> > > > > > > > > > > > >       { KE_IGNORE,    UNIWILL_OSD_FN_LOCK, { KEY_FN_ESC }},
> > > > > > > > > > > > >         /* Reported when the user wants to toggle the 
> > > > > > > > > > > > > brightness of the keyboard */
> > > > > > > > > > > > >       { KE_KEY, UNIWILL_OSD_KBDILLUMTOGGLE, { 
> > > > > > > > > > > > > KEY_KBDILLUMTOGGLE }},
> > > > > > > > > > > > > +    { KE_KEY, UNIWILL_OSD_KB_LED_LEVEL0, { 
> > > > > > > > > > > > > KEY_KBDILLUMTOGGLE }},
> > > > > > > > > > > > > +    { KE_KEY, UNIWILL_OSD_KB_LED_LEVEL1, { 
> > > > > > > > > > > > > KEY_KBDILLUMTOGGLE }},
> > > > > > > > > > > > > +    { KE_KEY, UNIWILL_OSD_KB_LED_LEVEL2, { 
> > > > > > > > > > > > > KEY_KBDILLUMTOGGLE }},
> > > > > > > > > > > > > +    { KE_KEY, UNIWILL_OSD_KB_LED_LEVEL3, { 
> > > > > > > > > > > > > KEY_KBDILLUMTOGGLE }},
> > > > > > > > > > > > > +    { KE_KEY, UNIWILL_OSD_KB_LED_LEVEL4, { 
> > > > > > > > > > > > > KEY_KBDILLUMTOGGLE }},
> > > > > > > > > > > > >         /* FIXME: find out the exact meaning of those 
> > > > > > > > > > > > > events */
> > > > > > > > > > > > >       { KE_IGNORE, UNIWILL_OSD_BAT_CHARGE_FULL_24_H, { 
> > > > > > > > > > > > > KEY_UNKNOWN }},
> > > > > > > > > > > > > @@ -395,6 +405,9 @@ static const struct key_entry 
> > > > > > > > > > > > > uniwill_keymap[] = {
> > > > > > > > > > > > >       /* Reported when the user wants to toggle the 
> > > > > > > > > > > > > benchmark mode status */
> > > > > > > > > > > > >       { KE_IGNORE, UNIWILL_OSD_BENCHMARK_MODE_TOGGLE, { 
> > > > > > > > > > > > > KEY_UNKNOWN }},
> > > > > > > > > > > > >   +    /* Reported when the user wants to toggle the 
> > > > > > > > > > > > > webcam */
> > > > > > > > > > > > > +    { KE_IGNORE, UNIWILL_OSD_WEBCAM_TOGGLE, { KEY_UNKNOWN 
> > > > > > > > > > > > > }},
> > > > > > > > > > > >
> > > > > > > > > > > > Same as above.
> > > > > > > > > > >
> > > > > > > > > > > Same as above ;)
> > > > > > > > > > >
> > > > > > > > > > > At least iirc, would have to double check
> > > > > > > > > > Ok.
> > > > > > > > > >
> > > > > > > > > > Thanks,
> > > > > > > > > > Armin Wolf
> > > > > > > > > >
> > > > > > > > > > > > > +
> > > > > > > > > > > > >       { KE_END }
> > > > > > > > > > > > >   };
> > > > > > > > > > > > >   @@ -1247,6 +1260,10 @@ static int 
> > > > > > > > > > > > > uniwill_notifier_call(struct notifier_block *nb, unsigned 
> > > > > > > > > > > > > long action
> > > > > > > > > > > > >           }
> > > > > > > > > > > > > mutex_unlock(&data->battery_lock);
> > > > > > > > > > > > >   +        return NOTIFY_OK;
> > > > > > > > > > > > > +    case UNIWILL_OSD_DC_ADAPTER_CHANGED:
> > > > > > > > > > > > > +        // noop for the time being
> > > > > > > > > > > >
> > > > > > > > > > > > Wrong comment style, please use /* */.
> > > > > > > > > > > ack
> > > > > > > > > > > > Thanks,
> > > > > > > > > > > > Armin Wolf
> > > > > > > > > > > >
> > > > > > > > > > > > > +
> > > > > > > > > > > > >           return NOTIFY_OK;
> > > > > > > > > > > > >       default:
> > > > > > > > > > > > > mutex_lock(&data->input_lock);
> > > > > > > > > > > > > diff --git a/drivers/platform/x86/uniwill/uniwill-wmi.h 
> > > > > > > > > > > > > b/drivers/platform/x86/uniwill/uniwill-wmi.h
> > > > > > > > > > > > > index 2bf69f2d80381..48783b2e9ffb9 100644
> > > > > > > > > > > > > --- a/drivers/platform/x86/uniwill/uniwill-wmi.h
> > > > > > > > > > > > > +++ b/drivers/platform/x86/uniwill/uniwill-wmi.h
> > > > > > > > > > > > > @@ -113,6 +113,8 @@
> > > > > > > > > > > > >     #define UNIWILL_OSD_BENCHMARK_MODE_TOGGLE 0xC0
> > > > > > > > > > > > >   +#define UNIWILL_OSD_WEBCAM_TOGGLE 0xCF
> > > > > > > > > > > > > +
> > > > > > > > > > > > >   #define UNIWILL_OSD_KBD_BACKLIGHT_CHANGED 0xF0
> > > > > > > > > > > > >     struct device;