Message-ID: <2aa477f3-526e-4b06-826a-83f95633c040@gmx.de>
Date: Mon, 28 Jul 2025 23:47:57 +0200
From: Armin Wolf <W_Armin@....de>
To: Gladyshev Ilya <foxido@...ido.dev>
Cc: Hans de Goede <hansg@...nel.org>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org,
 Nikita Krasnov <nikita.nikita.krasnov@...il.com>,
 Dmitry Torokhov <dmitry.torokhov@...il.com>,
 "open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)..."
 <linux-input@...r.kernel.org>
Subject: Re: [PATCH 1/1] Add WMI driver for Redmibook keyboard.

Am 28.07.25 um 00:34 schrieb Gladyshev Ilya:

> This driver implements support for various Fn keys (like Cut) and Xiaomi
> specific AI button.

Interesting, i was just talking with another person about implementing a WMI event
driver for the exact same WMI event device. I CCed the person involved in the discussion
so that he can test this driver on his device as well.

All in all the driver looks promising, but there are still things that need to be improved
before we can include this driver in the mainline kernel. For details see below.

Also please CC the linux input mailing list in the future so that they can give feedback as well.

> Signed-off-by: Gladyshev Ilya <foxido@...ido.dev>
> ---
>   MAINTAINERS                      |   6 ++
>   drivers/platform/x86/Kconfig     |  10 ++
>   drivers/platform/x86/Makefile    |   1 +
>   drivers/platform/x86/redmi-wmi.c | 164 +++++++++++++++++++++++++++++++
>   4 files changed, 181 insertions(+)
>   create mode 100644 drivers/platform/x86/redmi-wmi.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 10850512c118..b3956f3d2eb8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20965,6 +20965,12 @@ S:	Maintained
>   T:	git https://github.com/pkshih/rtw.git
>   F:	drivers/net/wireless/realtek/rtw89/
>   
> +REDMIBOOK WMI DRIVERS
> +M:	Gladyshev Ilya <foxido@...ido.dev>
> +L:	platform-driver-x86@...r.kernel.org
> +S:	Maintained
> +F:	drivers/platform/x86/redmi-wmi.c
> +
>   REDPINE WIRELESS DRIVER
>   L:	linux-wireless@...r.kernel.org
>   S:	Orphan
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index e5cbd58a99f3..b8d426e6b5a3 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -109,6 +109,16 @@ config XIAOMI_WMI
>   	  To compile this driver as a module, choose M here: the module will
>   	  be called xiaomi-wmi.
>   
> +config REDMI_WMI
> +	tristate "Redmibook WMI key driver"
> +	depends on ACPI_WMI
> +	depends on INPUT
> +	help
> +	  Say Y here if you want to support WMI-based keys on Redmibooks.

"Say Y here if you want support for WMI-based hotkey events on Xiaomi Redmi devices."

> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called redmi-wmi.
> +
>   config GIGABYTE_WMI
>   	tristate "Gigabyte WMI temperature driver"
>   	depends on ACPI_WMI
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index abbc2644ff6d..56903d7408cd 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_HUAWEI_WMI)		+= huawei-wmi.o
>   obj-$(CONFIG_MXM_WMI)			+= mxm-wmi.o
>   obj-$(CONFIG_NVIDIA_WMI_EC_BACKLIGHT)	+= nvidia-wmi-ec-backlight.o
>   obj-$(CONFIG_XIAOMI_WMI)		+= xiaomi-wmi.o
> +obj-$(CONFIG_REDMI_WMI)			+= redmi-wmi.o
>   obj-$(CONFIG_GIGABYTE_WMI)		+= gigabyte-wmi.o
>   
>   # Acer
> diff --git a/drivers/platform/x86/redmi-wmi.c b/drivers/platform/x86/redmi-wmi.c
> new file mode 100644
> index 000000000000..0bb6ea7b1081
> --- /dev/null
> +++ b/drivers/platform/x86/redmi-wmi.c
> @@ -0,0 +1,164 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* WMI driver for Xiaomi Redmibooks */
> +
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/input.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/wmi.h>
> +
> +#include <uapi/linux/input-event-codes.h>
> +
> +#define WMI_REDMIBOOK_KEYBOARD_EVENT_GUID "46c93e13-ee9b-4262-8488-563bca757fef"
> +
> +/* Supported WMI keys ... */
> +#define ACPI_CUT_PAYLOAD		0x00000201
> +#define ACPI_ALL_APPS_PAYLOAD		0x00000301
> +#define ACPI_SETUP_PAYLOAD		0x00001b01
> +#define ACPI_CST_KEY_PRESS_PAYLOAD	0x00011801
> +#define ACPI_CST_KEY_RELEASE_PAYLOAD	0x00011901
> +
> +/* ... and their mappings */
> +#define WMI_CUT_KEY		KEY_PROG1
> +#define WMI_ALL_APPS_KEY	KEY_ALL_APPLICATIONS
> +#define WMI_SETUP_KEY		KEY_SETUP
> +#define WMI_CST_KEY		KEY_ASSISTANT
> +

Why not using sparse-keymap for this?

> +/* Keyboard backlight key (not supported yet) */
> +#define BACKLIGHT_LEVEL_0_PAYLOAD	0x00000501
> +#define BACKLIGHT_LEVEL_1_PAYLOAD	0x00800501
> +#define BACKLIGHT_LEVEL_2_PAYLOAD	0x00050501
> +#define BACKLIGHT_LEVEL_3_PAYLOAD	0x000a0501
> +
> +struct redmi_wmi {
> +	struct input_dev *input_dev;
> +	/* Protects the key event sequence */
> +	struct mutex key_lock;
> +};
> +
> +static void redmi_mutex_destroy(void *data)
> +{
> +	struct mutex *lock = data;
> +
> +	mutex_destroy(lock);
> +}
> +
> +static int redmi_wmi_probe(struct wmi_device *wdev, const void *context)
> +{
> +	struct redmi_wmi *data;
> +	int ret;
> +
> +	/* Init dev */
> +	data = devm_kzalloc(&wdev->dev, sizeof(struct redmi_wmi), GFP_KERNEL);

sizeof(struct redmi_wmi) -> sizeof(*data)

> +	if (!data)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&wdev->dev, data);
> +
> +	/* Init mutex & setup destroy at exit */
> +	mutex_init(&data->key_lock);
> +	ret = devm_add_action_or_reset(&wdev->dev, redmi_mutex_destroy, &data->key_lock);
> +	if (ret < 0)
> +		return ret;

Please use devm_mutex_init() instead. You can also drop the comment then.

> +
> +	/* Setup input device */
> +	data->input_dev = devm_input_allocate_device(&wdev->dev);
> +	if (!data->input_dev)
> +		return -ENOMEM;
> +	data->input_dev->name = "Redmibook WMI keys";
> +	data->input_dev->phys = "wmi/input0";
> +
> +	set_bit(EV_KEY, data->input_dev->evbit);
> +
> +	/* "Cut" key*/
> +	set_bit(WMI_CUT_KEY, data->input_dev->keybit);
> +	/* "All apps" key*/
> +	set_bit(WMI_ALL_APPS_KEY, data->input_dev->keybit);
> +	/* "Settings" key */
> +	set_bit(WMI_SETUP_KEY, data->input_dev->keybit);
> +	/* Custom (AI?) key */
> +	set_bit(WMI_CST_KEY, data->input_dev->keybit);

Please use sparse-keymap for setting up all of this.

> +
> +	return input_register_device(data->input_dev);
> +}
> +
> +static void press_and_release_key(struct input_dev *dev, unsigned int code)
> +{
> +	input_report_key(dev, code, 1);
> +	input_sync(dev);
> +	input_report_key(dev, code, 0);
> +	input_sync(dev);
> +}

Using sparse-keymap would allow you to drop this function and instead rely on the autorelease
functionality of sparse-keymap instead.

> +
> +static void redmi_wmi_notify(struct wmi_device *wdev, union acpi_object *obj)
> +{
> +	struct redmi_wmi *data = dev_get_drvdata(&wdev->dev);
> +
> +	if (obj->type != ACPI_TYPE_BUFFER) {
> +		dev_err(&wdev->dev, "Bad response type %u\n", obj->type);
> +		return;
> +	}
> +
> +	if (obj->buffer.length < 4) {
> +		dev_err(&wdev->dev, "Invalid buffer length %u\n", obj->buffer.length);
> +		return;
> +	}

The MOF description of the WMI event looks like this:

class WMIEvent : __ExtrinsicEvent {
};

[WMI, Dynamic, Provider("WmiProv"), Locale("MS\\0x40A"), Description("Root/WMI/HID_EVENT20"), guid("{46c93e13-ee9b-4262-8488-563bca757fef}")]
class HID_EVENT20 : WmiEvent {
   [key, read] string InstanceName;
   [read] boolean Active;
   [WmiDataId(1), read, write, Description("Package Data")] uint8 EventDetail[32];
};

As you can see the buffer should contain at least 32 bytes. Please check this even when you are only using the
first 4 bytes as some WMI events might return fewer than 32 bytes of data in order to signal a firmware error.

> +
> +	/* For linearizability */
> +	guard(mutex)(&data->key_lock);
> +
> +	u32 payload = ((u32 *)obj->buffer.pointer)[0];

Please use get_unaligned_le32() from linux/unaligned.h as the buffer might not
be properly aligned for 32 bit integers.

> +
> +	switch (payload) {
> +	case ACPI_CUT_PAYLOAD:
> +		press_and_release_key(data->input_dev, WMI_CUT_KEY);
> +		break;
> +	case ACPI_ALL_APPS_PAYLOAD:
> +		press_and_release_key(data->input_dev, WMI_ALL_APPS_KEY);
> +		break;
> +	case ACPI_SETUP_PAYLOAD:
> +		press_and_release_key(data->input_dev, WMI_SETUP_KEY);
> +		break;
> +	case ACPI_CST_KEY_PRESS_PAYLOAD:
> +		input_report_key(data->input_dev, WMI_CST_KEY, 1);
> +		input_sync(data->input_dev);
> +		break;
> +	case ACPI_CST_KEY_RELEASE_PAYLOAD:
> +		input_report_key(data->input_dev, WMI_CST_KEY, 0);
> +		input_sync(data->input_dev);
> +		break;
> +	case BACKLIGHT_LEVEL_0_PAYLOAD:
> +	case BACKLIGHT_LEVEL_1_PAYLOAD:
> +	case BACKLIGHT_LEVEL_2_PAYLOAD:
> +	case BACKLIGHT_LEVEL_3_PAYLOAD:
> +		pr_debug("keyboard backlight WMI event, no action");
> +		break;
> +	default:
> +		pr_debug("unsupported Redmibook WMI event with 4byte payload %u", payload);

Please use dev_dbg() here. Also using sparse-keymap would allow you to skip all of this and
instead let the input subsystem do the matching.

> +		break;
> +	}
> +}
> +
> +static const struct wmi_device_id redmi_wmi_id_table[] = {
> +	{ .guid_string = WMI_REDMIBOOK_KEYBOARD_EVENT_GUID },
> +	/* Terminating entry */

Pointless commit, please remove.

Thanks,
Armin Wolf

> +	{ }
> +};
> +
> +static struct wmi_driver redmi_wmi_driver = {
> +	.driver = {
> +		.name = "redmi-wmi",
> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS
> +	},
> +	.id_table = redmi_wmi_id_table,
> +	.probe = redmi_wmi_probe,
> +	.notify = redmi_wmi_notify,
> +	.no_singleton = true,
> +};
> +module_wmi_driver(redmi_wmi_driver);
> +
> +MODULE_DEVICE_TABLE(wmi, redmi_wmi_id_table);
> +MODULE_AUTHOR("Gladyshev Ilya <foxido@...ido.dev>");
> +MODULE_DESCRIPTION("Redmibook WMI driver");
> +MODULE_LICENSE("GPL");

Powered by blists - more mailing lists