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: 10030 bytes --]

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.

-- 
 i.