Re: [PATCH v15 0/6] Merge redmi-wmi into bitland-mifs-wmi

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 12023 bytes --]

On Wed, 11 Nov 2026, Mingyou Chen wrote:

> 
> On 11/11/26 21:35, Ilpo Järvinen wrote:
> > On Wed, 11 Nov 2026, Mingyou Chen wrote:
> > 
> > > On 11/9/26 21:09, Ilpo Järvinen wrote:
> > > > On Wed, 28 Oct 2026, Mingyou Chen wrote:
> > > > 
> > > > > > And I've got a question, from sashiko's comment in my v12
> > > > > > 
> > > > > > https://sashiko.dev/#/patchset/20261012133648.132363-1-qby140326%40gmail.com
> > > > > > 
> > > > > > > Will this result in the loss of KEY_PERFORMANCE events on
> > > > > > > event-only
> > > > > > > legacy
> > > > > > > devices?
> > > > > > > Looking at bitland_mifs_wmi_probe() in
> > > > > > > drivers/platform/x86/bitland-mifs-wmi.c,
> > > > > > > if dev_type == BITLAND_WMI_EVENT, it returns early after
> > > > > > > registering
> > > > > > > the input
> > > > > > > device:
> > > > > > > 	if (dev_type == BITLAND_WMI_EVENT) {
> > > > > > > 		/* Register input device for hotkeys */
> > > > > > > 		drv_data->input_dev =
> > > > > > > devm_input_allocate_device(&wdev->dev);
> > > > > > > 		...
> > > > > > > 		return input_register_device(drv_data->input_dev);
> > > > > > > 	}
> > > > > > > When the performance key is pressed, bitland_mifs_wmi_notify()
> > > > > > > intercepts it,
> > > > > > > invokes the bitland_notifier_list, and returns without reporting
> > > > > > > an
> > > > > > > input event:
> > > > > > > 	case WMI_EVENT_PERFORMANCE_PLAN:
> > > > > > > 	case WMI_EVENT_PERFORMANCE_PLAN_ALT:
> > > > > > > 		blocking_notifier_call_chain(&bitland_notifier_list,
> > > > > > > 					     BITLAND_NOTIFY_PLATFORM_PROFILE,
> > > > > > > 					     NULL);
> > > > > > > 		return;
> > > > > > > Because the platform profile and notifier were not registered
> > > > > > > during
> > > > > > > probe
> > > > > > > for event-only devices, does this mean the events are silently
> > > > > > > dropped
> > > > > > > instead
> > > > > > > of emitting KEY_PERFORMANCE like the old driver did?
> > > > > > I ackknowledged that KEY_PERFORMANCE (WMI_EVENT_PERFORMANCE_PLAN_ALT
> > > > > > in
> > > > > > bitland-mifs-wmi) looks like an event in this LKML thread
> > > > > > 
> > > > > > > Since the event reports a change that has already happened,
> > > > > > > platform_profile_notify() is the natural way to pass it on, as
> > > > > > > suggested in my v6 reply: power-profiles-daemon then re-reads the
> > > > > > > profile, which is harmless after the driver's own writes too. If
> > > > > > > KEY_PERFORMANCE should stay for on-screen displays (Ibragim asked
> > > > > > > to
> > > > > > > keep these events observable), it may be worth not reporting it
> > > > > > > for
> > > > > > > the
> > > > > > > event that echoes the driver's own write, or at least saying in a
> > > > > > > comment that it reports a mode change that already happened. On
> > > > > > > Ibragim's TM2209 the profile SET is reported not to work [3], so
> > > > > > > there
> > > > > > > 0x16 can only come from Fn+K and the echo does not exist; Ibragim,
> > > > > > > can
> > > > > > > you confirm?
> > > > > > https://lore.kernel.org/all/20261008152647.1510152-1-uselessfire@gmail.com/
> > > > > > 
> > > > > > So should I follow sashiko's advice?
> > > > Do you find Hans' earlier feedback on this not actionable? Or did you
> > > > miss
> > > > it.
> > You did not reply to this question (which is my only remaining concern
> > with this series)!?
> > 
> > 
> > > > Copying long walls of text from sashiko's findings for an older version
> > > > to
> > > > the cover letter might not be the best approach in general. Usually, we
> > > > want to get the issues addressed before sending the next version so that
> > > > discussion should have occurred in the thread for the previous version.
> > > > 
> > > > > > And some remaining comments from sashiko's review of my v13:
> > > > > > 
> > > > > > > > -	/* Keyboard backlight: Off / Auto / Low / High (new state in
> > > > > > > > byte 2) */
> > > > > > > > -	{KE_KEY, 0x00000501,	{KEY_KBDILLUMTOGGLE}},
> > > > > > > > -	{KE_KEY, 0x00800501,	{KEY_KBDILLUMTOGGLE}},
> > > > > > > > -	{KE_KEY, 0x00050501,	{KEY_KBDILLUMTOGGLE}},
> > > > > > > > -	{KE_KEY, 0x000a0501,	{KEY_KBDILLUMTOGGLE}},
> > > > > > > > -
> > > > > > > > -	/* Xiaomi G Command Center */
> > > > > > > > -	{KE_KEY, 0x00010a01,	{KEY_VENDOR}},
> > > > > > > > -
> > > > > > > > -	/* OEM preset power mode: 1=Balanced 2=Silent 3=Turbo 4=Full
> > > > > > > > speed */
> > > > > > > > -	{KE_KEY, 0x00011601,	{KEY_PERFORMANCE}},
> > > > > > > > -	{KE_KEY, 0x00021601,	{KEY_PERFORMANCE}},
> > > > > > > > -	{KE_KEY, 0x00031601,	{KEY_PERFORMANCE}},
> > > > > > > > -	{KE_KEY, 0x00041601,	{KEY_PERFORMANCE}},
> > > > > > > In drivers/platform/x86/redmi-wmi.c, redmi_wmi_notify() reported
> > > > > > > these
> > > > > > > keyboard backlight events (event_id 0x05) as KEY_KBDILLUMTOGGLE
> > > > > > > and
> > > > > > > OEM preset
> > > > > > > power mode events (event_id 0x16) as KEY_PERFORMANCE through
> > > > > > > sparse_keymap_report_entry().
> > > > > > > In drivers/platform/x86/bitland-mifs-wmi.c,
> > > > > > > bitland_mifs_wmi_keymap
> > > > > > > defines
> > > > > > > the four WMI_EVENT_KBD_BRIGHTNESS entries for KEY_KBDILLUMTOGGLE:
> > > > > > > 	{ KE_KEY, BI_HOTKEY_CODE(WMI_EVENT_KBD_BRIGHTNESS, 0, 0),
> > > > > > > {KEY_KBDILLUMTOGGLE} },
> > > > > > > 	{ KE_KEY, BI_HOTKEY_CODE(WMI_EVENT_KBD_BRIGHTNESS, 0x80, 0),
> > > > > > > {KEY_KBDILLUMTOGGLE} },
> > > > > > > 	{ KE_KEY, BI_HOTKEY_CODE(WMI_EVENT_KBD_BRIGHTNESS, 5, 0),
> > > > > > > {KEY_KBDILLUMTOGGLE} },
> > > > > > > 	{ KE_KEY, BI_HOTKEY_CODE(WMI_EVENT_KBD_BRIGHTNESS, 0x0a, 0),
> > > > > > > {KEY_KBDILLUMTOGGLE} },
> > > > > > > However, bitland_mifs_wmi_notify() returns early for both
> > > > > > > WMI_EVENT_KBD_BRIGHTNESS and WMI_EVENT_PERFORMANCE_PLAN_ALT before
> > > > > > > reaching
> > > > > > > bitland_mifs_wmi_report_key():
> > > > > > > 	switch (event->event_id) {
> > > > > > > 	case WMI_EVENT_KBD_BRIGHTNESS:
> > > > > > > 		brightness = event->value_low;
> > > > > > > 		blocking_notifier_call_chain(&bitland_notifier_list,
> > > > > > > 					     BITLAND_NOTIFY_KBD_BRIGHTNESS,
> > > > > > > 					     &brightness);
> > > > > > > 		return;
> > > > > > > 	case WMI_EVENT_PERFORMANCE_PLAN:
> > > > > > > 	case WMI_EVENT_PERFORMANCE_PLAN_ALT:
> > > > > > > 		blocking_notifier_call_chain(&bitland_notifier_list,
> > > > > > > 					     BITLAND_NOTIFY_PLATFORM_PROFILE,
> > > > > > > 					     NULL);
> > > > > > > 		return;
> > > > > > > 	...
> > > > > > > 	}
> > > > > > > 	payload = get_unaligned_le32(buffer->data);
> > > > > > > 	bitland_mifs_wmi_report_key(wdev, payload);
> > > > > > > Does this early return leave the WMI_EVENT_KBD_BRIGHTNESS entries
> > > > > > > in
> > > > > > > bitland_mifs_wmi_keymap unreachable and cause a regression on
> > > > > > > Redmibook
> > > > > > > laptops (especially models that only expose BITLAND_EVENT_GUID
> > > > > > > without
> > > > > > > the
> > > > > > > BITLAND_MIFS_GUID control device) when pressing the keyboard
> > > > > > > backlight
> > > > > > > or
> > > > > > > power mode hotkeys?
> > > > > > > [ ... ]
> > > > > > Don't actually agree with sashiko, if we report the key to the
> > > > > > userspace, when WMs handle the KEY_KBDILLUMTOGGLE key, the keyboard
> > > > > > light will be triggered twice if the firmware already respond to the
> > > > > > event,
> > > > > > which is not expected.
> > > > > > 
> > > > > > So the question is, does the firmware respond the KBD_BRIGHTNESS
> > > > > > event?
> > > > > > And should we report this key to userspace?
> > > > > >From Chris' comment:
> > > > > 
> > > > > > - Fn+Space (keyboard backlight) sends no WMI event at all, neither
> > > > > > 0x05
> > > > > >     nor anything else, and does not change the backlight under
> > > > > > Linux. So
> > > > > >     this model cannot tell whether the firmware handles
> > > > > > KBD_BRIGHTNESS.
> > > > > > 
> > > > > > - Fn+K sends event 0x28 with value_low 0x01 on every press (01 28 01
> > > > > > 00),
> > > > > >     not 0x16. The firmware does not change the mode by itself: the
> > > > > >     SystemPerMode value (EC 0x68) stays the same across the presses.
> > > > > > If
> > > > > >     0x28 is the performance mode key on this model, it is a request
> > > > > > rather
> > > > > >     than a notification, so platform_profile_cycle() would fit. 0x28
> > > > > > is
> > > > > >     currently logged as "Unknown WMI event".
> > > > > > 
> > > > > > - On Windows on the same machine, with Xiaomi PC Manager not running
> > > > > >     (it fails to start on this install), neither Fn+K nor Fn+Space
> > > > > > does
> > > > > >     anything either. So Fn+K looks like a request that Xiaomi's
> > > > > > software
> > > > > >     normally acts on, not a mode change made by the firmware.
> > > > > I think different models have different behaviors on this.
> > > > > Should we add a quirk item?
> > > > For different behaviors, it may well be possible we need a quirk.
> > > This is included in Chris' patch series.
> > > 
> > > https://lore.kernel.org/r/20261106114300.936782-5-chris@miget.com
> > I know, so it shouldn't be a concern in the context of _this series_.
> > 
> > We'll be applying Chris' series after this one (and relatively soon after
> > it too, I think).
> > 
> > > > But it would be nice to get this refactoring out of the way relatively
> > > > soon as it has been blocking lots of other bitland patches and I'm a bit
> > > > worried the scope of this work is expanding beyond what an initial
> > > > refactor change would need.
> > > > 
> > > > If Chris' device lacks support without his series, this refactoring
> > > > shouldn't have to care about that yet as long as it doesn't
> > > > intentionally
> > > > make decisions we need to change right away. New quirks on top of this
> > > > refactoring to support a previously unsupported model is not a problem
> > > > but
> > > > too limiting thermal profile mapping would have been problematic as we'd
> > > > have immediately had to rework the mapper.
> > > I think it's enough for the known cases except this [1].
> > > 
> > > [1]
> > > https://lore.kernel.org/all/CAFbNUuLmaoq22pQPzrUidJ+RC2xf2u5Ovxzy5d0LQZ7owH52OA@mail.gmail.com/
> > > 
> > > I'm confused with this bug report, everything looks correctly.
> > My point is, that we should focus on getting this refactor series applied
> > first.
> > 
> > This again, while nice you're trying to help the report, looks future work
> > from the perspective of this refactor series as you again had to add
> > another profile table. That is, it is not supported until more work is
> > done, which will only happen after this series in terms of applied
> > commits.
> > 
> > Also, we don't need to make the rework series perfect, it will take
> > forever that way. We might still need to refactor something but I think
> > the major problem with the profiles is already dealt with and we're good
> > to go in that front.
> > 
> > So the only remaining concern I have is the one I reflagged above. Please
> > share your thoughts on that.
> 
> Hi,
> 
> Sorry for missing your question (I only looked at the bottom, sorry about
> that), here's my opinion:
> 
> There are two behaviors on Fn+K as far as I know:
> 
> - Hardware report 0x28 and leave the profile unchanged (this is covered in
> Chris' series)
> 
> - Hardware change the profile and report 0x16 [1], the driver should refresh
> the platform profile state, and don't report key scancode to userspace, I've
> already done this in my series (in the WMI_EVENT_PERFORMANCE_PLAN_ALT case).

Okay. Sadly, v15 fails to apply in too many places cleanly on top of 
review-ilpo-next so I don't have confidence in my ability to reproduce the 
same result as what you sent.

Please respin v16.

-- 
 i.