[PATCH] Add IdeaPad WMI Fn Keys driver

[PATCH] Add IdeaPad WMI Fn Keys driver

[Philipp Jungkamp]

Create an input device for WMI events corresponding to some special
keys on the 'Lenovo Yoga' line.

This include the 3 keys to the right on the 'Lenovo Yoga9 14IAP7' and
additionally the 'Lenovo Support' and 'Lenovo Favorites' (star with 'S'
inside) in the fn key row as well as the event emitted on 'Fn+R' which
toggles between 60Hz and 90Hz display refresh rate on windows.

Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
---
I found this patch by poking in the DSDT. I have not submitted any
notable patches yet and hope you can help me improve in case I make
unfortunate choices during submission.

Thank you for your time!
Philipp Jungkamp

 drivers/platform/x86/Kconfig       |  13 +++
 drivers/platform/x86/Makefile      |   1 +
 drivers/platform/x86/ideapad-wmi.c | 153 +++++++++++++++++++++++++++++
 3 files changed, 167 insertions(+)
 create mode 100644 drivers/platform/x86/ideapad-wmi.c

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index f2f98e942cf2..e7c5148e5cb4 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -140,6 +140,19 @@ config YOGABOOK_WMI
 	  To compile this driver as a module, choose M here: the module will
 	  be called lenovo-yogabook-wmi.

+config IDEAPAD_WMI
+	tristate "Lenovo IdeaPad WMI Fn Keys"
+	depends on ACPI_WMI
+	depends on INPUT
+	select INPUT_SPARSEKMAP
+	help
+	  Say Y here if you want to receive key presses from some lenovo
+	  specific keys. (Star Key, Support Key, Virtual Background,
+	  Dark Mode Toggle, ...)
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called ideapad-wmi.
+
 config ACERHDF
 	tristate "Acer Aspire One temperature and fan driver"
 	depends on ACPI && THERMAL
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 5a428caa654a..d8bec884d6bc 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_PEAQ_WMI)			+= peaq-wmi.o
 obj-$(CONFIG_XIAOMI_WMI)		+= xiaomi-wmi.o
 obj-$(CONFIG_GIGABYTE_WMI)		+= gigabyte-wmi.o
 obj-$(CONFIG_YOGABOOK_WMI)		+= lenovo-yogabook-wmi.o
+obj-$(CONFIG_IDEAPAD_WMI)		+= ideapad-wmi.o

 # Acer
 obj-$(CONFIG_ACERHDF)		+= acerhdf.o
diff --git a/drivers/platform/x86/ideapad-wmi.c b/drivers/platform/x86/ideapad-wmi.c
new file mode 100644
index 000000000000..38f7b3d0c171
--- /dev/null
+++ b/drivers/platform/x86/ideapad-wmi.c
@@ -0,0 +1,153 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * ideapad-wmi.c - Ideapad WMI fn keys driver
+ *
+ * Copyright (C) 2022 Philipp Jungkamp <p.jungkamp@gmx.net>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/acpi.h>
+#include <linux/input.h>
+#include <linux/input/sparse-keymap.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/wmi.h>
+
+#define IDEAPAD_FN_KEY_EVENT_GUID	"8FC0DE0C-B4E4-43FD-B0F3-8871711C1294"
+
+struct ideapad_wmi_private {
+	struct wmi_device *wmi_device;
+	struct input_dev *input_dev;
+};
+
+static const struct key_entry ideapad_wmi_fn_key_keymap[] = {
+	/* FnLock (handled by the firmware) */
+	{ KE_IGNORE,	0x02 },
+	/* Customizable Lenovo Hotkey ("star" with 'S' inside) */
+	{ KE_KEY,	0x01, { KEY_FAVORITES } },
+	/* Dark mode toggle */
+	{ KE_KEY,	0x13, { KEY_PROG1 } },
+	/* Sound profile switch */
+	{ KE_KEY,	0x12, { KEY_PROG2 } },
+	/* Lenovo Virtual Background application */
+	{ KE_KEY,	0x28, { KEY_PROG3 } },
+	/* Lenovo Support */
+	{ KE_KEY,	0x27, { KEY_HELP } },
+	/* Refresh Rate Toggle */
+	{ KE_KEY,	0x0a, { KEY_DISPLAYTOGGLE } },
+	{ KE_END },
+};
+
+static int ideapad_wmi_input_init(struct ideapad_wmi_private *priv)
+{
+	struct input_dev *input_dev;
+	int err;
+
+	input_dev = input_allocate_device();
+	if (!input_dev) {
+		return -ENOMEM;
+	}
+
+	input_dev->name = "Ideapad WMI Fn Keys";
+	input_dev->phys = IDEAPAD_FN_KEY_EVENT_GUID "/input0";
+	input_dev->id.bustype = BUS_HOST;
+	input_dev->dev.parent = &priv->wmi_device->dev;
+
+	err = sparse_keymap_setup(input_dev, ideapad_wmi_fn_key_keymap, NULL);
+	if (err) {
+		dev_err(&priv->wmi_device->dev,
+			"Could not set up input device keymap: %d\n", err);
+		goto err_free_dev;
+	}
+
+	err = input_register_device(input_dev);
+	if (err) {
+		dev_err(&priv->wmi_device->dev,
+			"Could not register input device: %d\n", err);
+		goto err_free_dev;
+	}
+
+	priv->input_dev = input_dev;
+	return 0;
+
+err_free_dev:
+	input_free_device(input_dev);
+	return err;
+}
+
+static void ideapad_wmi_input_exit(struct ideapad_wmi_private *priv)
+{
+	input_unregister_device(priv->input_dev);
+	priv->input_dev = NULL;
+}
+
+static void ideapad_wmi_input_report(struct ideapad_wmi_private *priv,
+				     unsigned int scancode)
+{
+	sparse_keymap_report_event(priv->input_dev, scancode, 1, true);
+}
+
+static int ideapad_wmi_probe(struct wmi_device *wdev, const void *ctx)
+{
+	struct ideapad_wmi_private *priv;
+	int err;
+
+	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	dev_set_drvdata(&wdev->dev, priv);
+
+	priv->wmi_device = wdev;
+
+	err = ideapad_wmi_input_init(priv);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+static void ideapad_wmi_remove(struct wmi_device *wdev)
+{
+	struct ideapad_wmi_private *priv = dev_get_drvdata(&wdev->dev);
+
+	ideapad_wmi_input_exit(priv);
+}
+
+static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
+{
+	struct ideapad_wmi_private *priv = dev_get_drvdata(&wdev->dev);
+
+	if(data->type != ACPI_TYPE_INTEGER) {
+		dev_warn(&priv->wmi_device->dev,
+			"WMI event data is not an integer\n");
+		return;
+	}
+
+	ideapad_wmi_input_report(priv, data->integer.value);
+}
+
+static const struct wmi_device_id ideapad_wmi_id_table[] = {
+	{	/* Special Keys on the Yoga 9 14IAP7 */
+		.guid_string = IDEAPAD_FN_KEY_EVENT_GUID
+	},
+	{ }
+};
+
+static struct wmi_driver ideapad_wmi_driver = {
+	.driver = {
+		.name = "ideapad-wmi",
+	},
+	.id_table = ideapad_wmi_id_table,
+	.probe = ideapad_wmi_probe,
+	.remove = ideapad_wmi_remove,
+	.notify = ideapad_wmi_notify,
+};
+
+module_wmi_driver(ideapad_wmi_driver);
+
+MODULE_DEVICE_TABLE(wmi, ideapad_wmi_id_table);
+MODULE_AUTHOR("Philipp Jungkamp <p.jungkamp@gmx.net>");
+MODULE_DESCRIPTION("Ideapad WMI fn keys driver");
+MODULE_LICENSE("GPL");
--
2.37.3

Re: [PATCH] Add IdeaPad WMI Fn Keys driver

[Hans de Goede]

Hi Philipp,

On 9/11/22 18:04, Philipp Jungkamp wrote:
> Create an input device for WMI events corresponding to some special
> keys on the 'Lenovo Yoga' line.
> 
> This include the 3 keys to the right on the 'Lenovo Yoga9 14IAP7' and
> additionally the 'Lenovo Support' and 'Lenovo Favorites' (star with 'S'
> inside) in the fn key row as well as the event emitted on 'Fn+R' which
> toggles between 60Hz and 90Hz display refresh rate on windows.
> 
> Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
> ---
> I found this patch by poking in the DSDT. I have not submitted any
> notable patches yet and hope you can help me improve in case I make
> unfortunate choices during submission.

No worries at a first glance (I have not looked at this in any
detail yet) this looks pretty good for a first submission.

And thank you for contributing to the Linux kernel!


> Philipp Jungkamp
> 
>  drivers/platform/x86/Kconfig       |  13 +++
>  drivers/platform/x86/Makefile      |   1 +
>  drivers/platform/x86/ideapad-wmi.c | 153 +++++++++++++++++++++++++++++
>  3 files changed, 167 insertions(+)
>  create mode 100644 drivers/platform/x86/ideapad-wmi.c
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index f2f98e942cf2..e7c5148e5cb4 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -140,6 +140,19 @@ config YOGABOOK_WMI
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called lenovo-yogabook-wmi.
> 
> +config IDEAPAD_WMI
> +	tristate "Lenovo IdeaPad WMI Fn Keys"
> +	depends on ACPI_WMI
> +	depends on INPUT
> +	select INPUT_SPARSEKMAP
> +	help
> +	  Say Y here if you want to receive key presses from some lenovo
> +	  specific keys. (Star Key, Support Key, Virtual Background,
> +	  Dark Mode Toggle, ...)
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called ideapad-wmi.
> +
>  config ACERHDF
>  	tristate "Acer Aspire One temperature and fan driver"
>  	depends on ACPI && THERMAL
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 5a428caa654a..d8bec884d6bc 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_PEAQ_WMI)			+= peaq-wmi.o
>  obj-$(CONFIG_XIAOMI_WMI)		+= xiaomi-wmi.o
>  obj-$(CONFIG_GIGABYTE_WMI)		+= gigabyte-wmi.o
>  obj-$(CONFIG_YOGABOOK_WMI)		+= lenovo-yogabook-wmi.o
> +obj-$(CONFIG_IDEAPAD_WMI)		+= ideapad-wmi.o
> 
>  # Acer
>  obj-$(CONFIG_ACERHDF)		+= acerhdf.o
> diff --git a/drivers/platform/x86/ideapad-wmi.c b/drivers/platform/x86/ideapad-wmi.c
> new file mode 100644
> index 000000000000..38f7b3d0c171
> --- /dev/null
> +++ b/drivers/platform/x86/ideapad-wmi.c
> @@ -0,0 +1,153 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * ideapad-wmi.c - Ideapad WMI fn keys driver
> + *
> + * Copyright (C) 2022 Philipp Jungkamp <p.jungkamp@gmx.net>
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/input.h>
> +#include <linux/input/sparse-keymap.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/wmi.h>
> +
> +#define IDEAPAD_FN_KEY_EVENT_GUID	"8FC0DE0C-B4E4-43FD-B0F3-8871711C1294"

At a first hunch (basically huh, don't we have a driver for that
already?) I grepped through the kernel sources and found:

drivers/platform/x86/ideapad-laptop.c

can you see if you can make things work with that driver?

Regards,

Hans



> +
> +struct ideapad_wmi_private {
> +	struct wmi_device *wmi_device;
> +	struct input_dev *input_dev;
> +};
> +
> +static const struct key_entry ideapad_wmi_fn_key_keymap[] = {
> +	/* FnLock (handled by the firmware) */
> +	{ KE_IGNORE,	0x02 },
> +	/* Customizable Lenovo Hotkey ("star" with 'S' inside) */
> +	{ KE_KEY,	0x01, { KEY_FAVORITES } },
> +	/* Dark mode toggle */
> +	{ KE_KEY,	0x13, { KEY_PROG1 } },
> +	/* Sound profile switch */
> +	{ KE_KEY,	0x12, { KEY_PROG2 } },
> +	/* Lenovo Virtual Background application */
> +	{ KE_KEY,	0x28, { KEY_PROG3 } },
> +	/* Lenovo Support */
> +	{ KE_KEY,	0x27, { KEY_HELP } },
> +	/* Refresh Rate Toggle */
> +	{ KE_KEY,	0x0a, { KEY_DISPLAYTOGGLE } },
> +	{ KE_END },
> +};
> +
> +static int ideapad_wmi_input_init(struct ideapad_wmi_private *priv)
> +{
> +	struct input_dev *input_dev;
> +	int err;
> +
> +	input_dev = input_allocate_device();
> +	if (!input_dev) {
> +		return -ENOMEM;
> +	}
> +
> +	input_dev->name = "Ideapad WMI Fn Keys";
> +	input_dev->phys = IDEAPAD_FN_KEY_EVENT_GUID "/input0";
> +	input_dev->id.bustype = BUS_HOST;
> +	input_dev->dev.parent = &priv->wmi_device->dev;
> +
> +	err = sparse_keymap_setup(input_dev, ideapad_wmi_fn_key_keymap, NULL);
> +	if (err) {
> +		dev_err(&priv->wmi_device->dev,
> +			"Could not set up input device keymap: %d\n", err);
> +		goto err_free_dev;
> +	}
> +
> +	err = input_register_device(input_dev);
> +	if (err) {
> +		dev_err(&priv->wmi_device->dev,
> +			"Could not register input device: %d\n", err);
> +		goto err_free_dev;
> +	}
> +
> +	priv->input_dev = input_dev;
> +	return 0;
> +
> +err_free_dev:
> +	input_free_device(input_dev);
> +	return err;
> +}
> +
> +static void ideapad_wmi_input_exit(struct ideapad_wmi_private *priv)
> +{
> +	input_unregister_device(priv->input_dev);
> +	priv->input_dev = NULL;
> +}
> +
> +static void ideapad_wmi_input_report(struct ideapad_wmi_private *priv,
> +				     unsigned int scancode)
> +{
> +	sparse_keymap_report_event(priv->input_dev, scancode, 1, true);
> +}
> +
> +static int ideapad_wmi_probe(struct wmi_device *wdev, const void *ctx)
> +{
> +	struct ideapad_wmi_private *priv;
> +	int err;
> +
> +	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&wdev->dev, priv);
> +
> +	priv->wmi_device = wdev;
> +
> +	err = ideapad_wmi_input_init(priv);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static void ideapad_wmi_remove(struct wmi_device *wdev)
> +{
> +	struct ideapad_wmi_private *priv = dev_get_drvdata(&wdev->dev);
> +
> +	ideapad_wmi_input_exit(priv);
> +}
> +
> +static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
> +{
> +	struct ideapad_wmi_private *priv = dev_get_drvdata(&wdev->dev);
> +
> +	if(data->type != ACPI_TYPE_INTEGER) {
> +		dev_warn(&priv->wmi_device->dev,
> +			"WMI event data is not an integer\n");
> +		return;
> +	}
> +
> +	ideapad_wmi_input_report(priv, data->integer.value);
> +}
> +
> +static const struct wmi_device_id ideapad_wmi_id_table[] = {
> +	{	/* Special Keys on the Yoga 9 14IAP7 */
> +		.guid_string = IDEAPAD_FN_KEY_EVENT_GUID
> +	},
> +	{ }
> +};
> +
> +static struct wmi_driver ideapad_wmi_driver = {
> +	.driver = {
> +		.name = "ideapad-wmi",
> +	},
> +	.id_table = ideapad_wmi_id_table,
> +	.probe = ideapad_wmi_probe,
> +	.remove = ideapad_wmi_remove,
> +	.notify = ideapad_wmi_notify,
> +};
> +
> +module_wmi_driver(ideapad_wmi_driver);
> +
> +MODULE_DEVICE_TABLE(wmi, ideapad_wmi_id_table);
> +MODULE_AUTHOR("Philipp Jungkamp <p.jungkamp@gmx.net>");
> +MODULE_DESCRIPTION("Ideapad WMI fn keys driver");
> +MODULE_LICENSE("GPL");
> --
> 2.37.3
> 

Re: [PATCH] Add IdeaPad WMI Fn Keys driver

[Hans de Goede]

Hi again,

On 9/11/22 17:18, Hans de Goede wrote:
> Hi Philipp,
> 
> On 9/11/22 18:04, Philipp Jungkamp wrote:
>> Create an input device for WMI events corresponding to some special
>> keys on the 'Lenovo Yoga' line.
>>
>> This include the 3 keys to the right on the 'Lenovo Yoga9 14IAP7' and
>> additionally the 'Lenovo Support' and 'Lenovo Favorites' (star with 'S'
>> inside) in the fn key row as well as the event emitted on 'Fn+R' which
>> toggles between 60Hz and 90Hz display refresh rate on windows.
>>
>> Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
>> ---
>> I found this patch by poking in the DSDT. I have not submitted any
>> notable patches yet and hope you can help me improve in case I make
>> unfortunate choices during submission.
> 
> No worries at a first glance (I have not looked at this in any
> detail yet) this looks pretty good for a first submission.
> 
> And thank you for contributing to the Linux kernel!
> 
> 
>> Philipp Jungkamp
>>
>>  drivers/platform/x86/Kconfig       |  13 +++
>>  drivers/platform/x86/Makefile      |   1 +
>>  drivers/platform/x86/ideapad-wmi.c | 153 +++++++++++++++++++++++++++++
>>  3 files changed, 167 insertions(+)
>>  create mode 100644 drivers/platform/x86/ideapad-wmi.c
>>
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index f2f98e942cf2..e7c5148e5cb4 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -140,6 +140,19 @@ config YOGABOOK_WMI
>>  	  To compile this driver as a module, choose M here: the module will
>>  	  be called lenovo-yogabook-wmi.
>>
>> +config IDEAPAD_WMI
>> +	tristate "Lenovo IdeaPad WMI Fn Keys"
>> +	depends on ACPI_WMI
>> +	depends on INPUT
>> +	select INPUT_SPARSEKMAP
>> +	help
>> +	  Say Y here if you want to receive key presses from some lenovo
>> +	  specific keys. (Star Key, Support Key, Virtual Background,
>> +	  Dark Mode Toggle, ...)
>> +
>> +	  To compile this driver as a module, choose M here: the module will
>> +	  be called ideapad-wmi.
>> +
>>  config ACERHDF
>>  	tristate "Acer Aspire One temperature and fan driver"
>>  	depends on ACPI && THERMAL
>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
>> index 5a428caa654a..d8bec884d6bc 100644
>> --- a/drivers/platform/x86/Makefile
>> +++ b/drivers/platform/x86/Makefile
>> @@ -16,6 +16,7 @@ obj-$(CONFIG_PEAQ_WMI)			+= peaq-wmi.o
>>  obj-$(CONFIG_XIAOMI_WMI)		+= xiaomi-wmi.o
>>  obj-$(CONFIG_GIGABYTE_WMI)		+= gigabyte-wmi.o
>>  obj-$(CONFIG_YOGABOOK_WMI)		+= lenovo-yogabook-wmi.o
>> +obj-$(CONFIG_IDEAPAD_WMI)		+= ideapad-wmi.o
>>
>>  # Acer
>>  obj-$(CONFIG_ACERHDF)		+= acerhdf.o
>> diff --git a/drivers/platform/x86/ideapad-wmi.c b/drivers/platform/x86/ideapad-wmi.c
>> new file mode 100644
>> index 000000000000..38f7b3d0c171
>> --- /dev/null
>> +++ b/drivers/platform/x86/ideapad-wmi.c
>> @@ -0,0 +1,153 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * ideapad-wmi.c - Ideapad WMI fn keys driver
>> + *
>> + * Copyright (C) 2022 Philipp Jungkamp <p.jungkamp@gmx.net>
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/input.h>
>> +#include <linux/input/sparse-keymap.h>
>> +#include <linux/list.h>
>> +#include <linux/module.h>
>> +#include <linux/wmi.h>
>> +
>> +#define IDEAPAD_FN_KEY_EVENT_GUID	"8FC0DE0C-B4E4-43FD-B0F3-8871711C1294"
> 
> At a first hunch (basically huh, don't we have a driver for that
> already?) I grepped through the kernel sources and found:
> 
> drivers/platform/x86/ideapad-laptop.c
> 
> can you see if you can make things work with that driver?

So I have taken a quick look at this and it seems to me that this
really should be able to work with the existing ideapad-laptop.c code ?

For starters you could add a debug printk / dev_info to this block,

#if IS_ENABLED(CONFIG_ACPI_WMI)
        for (i = 0; i < ARRAY_SIZE(ideapad_wmi_fnesc_events); i++) {
                status = wmi_install_notify_handler(ideapad_wmi_fnesc_events[i],
                                                    ideapad_wmi_notify, priv);
                if (ACPI_SUCCESS(status)) {
                        priv->fnesc_guid = ideapad_wmi_fnesc_events[i];
                        break;
                }
        }

        if (ACPI_FAILURE(status) && status != AE_NOT_EXIST) {
                err = -EIO;
                goto notification_failed_wmi;
        }
#endif

checking which event GUID ideapad-laptop binds to on your laptop. Assuming that
it does bind to the GUID this driver is binding to too, then it would be
a matter of extending the existing ideapad_wmi_notify() to do the same
as your notify function in this stand-alone driver. Note you can get the
the equivalend of the union acpi_object *data argument in your wmi handler
by calling wmi_get_event_data().

Regards,

Hans



>> +
>> +struct ideapad_wmi_private {
>> +	struct wmi_device *wmi_device;
>> +	struct input_dev *input_dev;
>> +};
>> +
>> +static const struct key_entry ideapad_wmi_fn_key_keymap[] = {
>> +	/* FnLock (handled by the firmware) */
>> +	{ KE_IGNORE,	0x02 },
>> +	/* Customizable Lenovo Hotkey ("star" with 'S' inside) */
>> +	{ KE_KEY,	0x01, { KEY_FAVORITES } },
>> +	/* Dark mode toggle */
>> +	{ KE_KEY,	0x13, { KEY_PROG1 } },
>> +	/* Sound profile switch */
>> +	{ KE_KEY,	0x12, { KEY_PROG2 } },
>> +	/* Lenovo Virtual Background application */
>> +	{ KE_KEY,	0x28, { KEY_PROG3 } },
>> +	/* Lenovo Support */
>> +	{ KE_KEY,	0x27, { KEY_HELP } },
>> +	/* Refresh Rate Toggle */
>> +	{ KE_KEY,	0x0a, { KEY_DISPLAYTOGGLE } },
>> +	{ KE_END },
>> +};
>> +
>> +static int ideapad_wmi_input_init(struct ideapad_wmi_private *priv)
>> +{
>> +	struct input_dev *input_dev;
>> +	int err;
>> +
>> +	input_dev = input_allocate_device();
>> +	if (!input_dev) {
>> +		return -ENOMEM;
>> +	}
>> +
>> +	input_dev->name = "Ideapad WMI Fn Keys";
>> +	input_dev->phys = IDEAPAD_FN_KEY_EVENT_GUID "/input0";
>> +	input_dev->id.bustype = BUS_HOST;
>> +	input_dev->dev.parent = &priv->wmi_device->dev;
>> +
>> +	err = sparse_keymap_setup(input_dev, ideapad_wmi_fn_key_keymap, NULL);
>> +	if (err) {
>> +		dev_err(&priv->wmi_device->dev,
>> +			"Could not set up input device keymap: %d\n", err);
>> +		goto err_free_dev;
>> +	}
>> +
>> +	err = input_register_device(input_dev);
>> +	if (err) {
>> +		dev_err(&priv->wmi_device->dev,
>> +			"Could not register input device: %d\n", err);
>> +		goto err_free_dev;
>> +	}
>> +
>> +	priv->input_dev = input_dev;
>> +	return 0;
>> +
>> +err_free_dev:
>> +	input_free_device(input_dev);
>> +	return err;
>> +}
>> +
>> +static void ideapad_wmi_input_exit(struct ideapad_wmi_private *priv)
>> +{
>> +	input_unregister_device(priv->input_dev);
>> +	priv->input_dev = NULL;
>> +}
>> +
>> +static void ideapad_wmi_input_report(struct ideapad_wmi_private *priv,
>> +				     unsigned int scancode)
>> +{
>> +	sparse_keymap_report_event(priv->input_dev, scancode, 1, true);
>> +}
>> +
>> +static int ideapad_wmi_probe(struct wmi_device *wdev, const void *ctx)
>> +{
>> +	struct ideapad_wmi_private *priv;
>> +	int err;
>> +
>> +	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	dev_set_drvdata(&wdev->dev, priv);
>> +
>> +	priv->wmi_device = wdev;
>> +
>> +	err = ideapad_wmi_input_init(priv);
>> +	if (err)
>> +		return err;
>> +
>> +	return 0;
>> +}
>> +
>> +static void ideapad_wmi_remove(struct wmi_device *wdev)
>> +{
>> +	struct ideapad_wmi_private *priv = dev_get_drvdata(&wdev->dev);
>> +
>> +	ideapad_wmi_input_exit(priv);
>> +}
>> +
>> +static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
>> +{
>> +	struct ideapad_wmi_private *priv = dev_get_drvdata(&wdev->dev);
>> +
>> +	if(data->type != ACPI_TYPE_INTEGER) {
>> +		dev_warn(&priv->wmi_device->dev,
>> +			"WMI event data is not an integer\n");
>> +		return;
>> +	}
>> +
>> +	ideapad_wmi_input_report(priv, data->integer.value);
>> +}
>> +
>> +static const struct wmi_device_id ideapad_wmi_id_table[] = {
>> +	{	/* Special Keys on the Yoga 9 14IAP7 */
>> +		.guid_string = IDEAPAD_FN_KEY_EVENT_GUID
>> +	},
>> +	{ }
>> +};
>> +
>> +static struct wmi_driver ideapad_wmi_driver = {
>> +	.driver = {
>> +		.name = "ideapad-wmi",
>> +	},
>> +	.id_table = ideapad_wmi_id_table,
>> +	.probe = ideapad_wmi_probe,
>> +	.remove = ideapad_wmi_remove,
>> +	.notify = ideapad_wmi_notify,
>> +};
>> +
>> +module_wmi_driver(ideapad_wmi_driver);
>> +
>> +MODULE_DEVICE_TABLE(wmi, ideapad_wmi_id_table);
>> +MODULE_AUTHOR("Philipp Jungkamp <p.jungkamp@gmx.net>");
>> +MODULE_DESCRIPTION("Ideapad WMI fn keys driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.37.3
>>

Re: [PATCH] Add IdeaPad WMI Fn Keys driver

[Philipp Jungkamp]

On Mon, 2022-09-19 at 08:54 +0100, Hans de Goede wrote:
> Hi again,
> 
> On 9/11/22 17:18, Hans de Goede wrote:
> > Hi Philipp,
> > 
> > On 9/11/22 18:04, Philipp Jungkamp wrote:
> > > Create an input device for WMI events corresponding to some
> > > special
> > > keys on the 'Lenovo Yoga' line.
> > > 
> > > This include the 3 keys to the right on the 'Lenovo Yoga9 14IAP7'
> > > and
> > > additionally the 'Lenovo Support' and 'Lenovo Favorites' (star
> > > with 'S'
> > > inside) in the fn key row as well as the event emitted on 'Fn+R'
> > > which
> > > toggles between 60Hz and 90Hz display refresh rate on windows.
> > > 
> > > Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
> > > ---
> > > I found this patch by poking in the DSDT. I have not submitted
> > > any
> > > notable patches yet and hope you can help me improve in case I
> > > make
> > > unfortunate choices during submission.
> > 
> > No worries at a first glance (I have not looked at this in any
> > detail yet) this looks pretty good for a first submission.
> > 
> > And thank you for contributing to the Linux kernel!
> > 
> > 
> > > Philipp Jungkamp
> > > 
> > >  drivers/platform/x86/Kconfig       |  13 +++
> > >  drivers/platform/x86/Makefile      |   1 +
> > >  drivers/platform/x86/ideapad-wmi.c | 153
> > > +++++++++++++++++++++++++++++
> > >  3 files changed, 167 insertions(+)
> > >  create mode 100644 drivers/platform/x86/ideapad-wmi.c
> > > 
> > > diff --git a/drivers/platform/x86/Kconfig
> > > b/drivers/platform/x86/Kconfig
> > > index f2f98e942cf2..e7c5148e5cb4 100644
> > > --- a/drivers/platform/x86/Kconfig
> > > +++ b/drivers/platform/x86/Kconfig
> > > @@ -140,6 +140,19 @@ config YOGABOOK_WMI
> > >           To compile this driver as a module, choose M here: the
> > > module will
> > >           be called lenovo-yogabook-wmi.
> > > 
> > > +config IDEAPAD_WMI
> > > +       tristate "Lenovo IdeaPad WMI Fn Keys"
> > > +       depends on ACPI_WMI
> > > +       depends on INPUT
> > > +       select INPUT_SPARSEKMAP
> > > +       help
> > > +         Say Y here if you want to receive key presses from some
> > > lenovo
> > > +         specific keys. (Star Key, Support Key, Virtual
> > > Background,
> > > +         Dark Mode Toggle, ...)
> > > +
> > > +         To compile this driver as a module, choose M here: the
> > > module will
> > > +         be called ideapad-wmi.
> > > +
> > >  config ACERHDF
> > >         tristate "Acer Aspire One temperature and fan driver"
> > >         depends on ACPI && THERMAL
> > > diff --git a/drivers/platform/x86/Makefile
> > > b/drivers/platform/x86/Makefile
> > > index 5a428caa654a..d8bec884d6bc 100644
> > > --- a/drivers/platform/x86/Makefile
> > > +++ b/drivers/platform/x86/Makefile
> > > @@ -16,6 +16,7 @@ obj-
> > > $(CONFIG_PEAQ_WMI)                        += peaq-wmi.o
> > >  obj-$(CONFIG_XIAOMI_WMI)               += xiaomi-wmi.o
> > >  obj-$(CONFIG_GIGABYTE_WMI)             += gigabyte-wmi.o
> > >  obj-$(CONFIG_YOGABOOK_WMI)             += lenovo-yogabook-wmi.o
> > > +obj-$(CONFIG_IDEAPAD_WMI)              += ideapad-wmi.o
> > > 
> > >  # Acer
> > >  obj-$(CONFIG_ACERHDF)          += acerhdf.o
> > > diff --git a/drivers/platform/x86/ideapad-wmi.c
> > > b/drivers/platform/x86/ideapad-wmi.c
> > > new file mode 100644
> > > index 000000000000..38f7b3d0c171
> > > --- /dev/null
> > > +++ b/drivers/platform/x86/ideapad-wmi.c
> > > @@ -0,0 +1,153 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * ideapad-wmi.c - Ideapad WMI fn keys driver
> > > + *
> > > + * Copyright (C) 2022 Philipp Jungkamp <p.jungkamp@gmx.net>
> > > + */
> > > +
> > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > +
> > > +#include <linux/acpi.h>
> > > +#include <linux/input.h>
> > > +#include <linux/input/sparse-keymap.h>
> > > +#include <linux/list.h>
> > > +#include <linux/module.h>
> > > +#include <linux/wmi.h>
> > > +
> > > +#define IDEAPAD_FN_KEY_EVENT_GUID      "8FC0DE0C-B4E4-43FD-B0F3-
> > > 8871711C1294"
> > 
> > At a first hunch (basically huh, don't we have a driver for that
> > already?) I grepped through the kernel sources and found:
> > 
> > drivers/platform/x86/ideapad-laptop.c
> > 
> > can you see if you can make things work with that driver?
> 
> So I have taken a quick look at this and it seems to me that this
> really should be able to work with the existing ideapad-laptop.c code
> ?
> 
> For starters you could add a debug printk / dev_info to this block,
> 
> #if IS_ENABLED(CONFIG_ACPI_WMI)
>         for (i = 0; i < ARRAY_SIZE(ideapad_wmi_fnesc_events); i++) {
>                 status =
> wmi_install_notify_handler(ideapad_wmi_fnesc_events[i],
>                                                    
> ideapad_wmi_notify, priv);
>                 if (ACPI_SUCCESS(status)) {
>                         priv->fnesc_guid =
> ideapad_wmi_fnesc_events[i];
>                         break;
>                 }
>         }
> 
>         if (ACPI_FAILURE(status) && status != AE_NOT_EXIST) {
>                 err = -EIO;
>                 goto notification_failed_wmi;
>         }
> #endif
> 
> checking which event GUID ideapad-laptop binds to on your laptop.
> Assuming that
> it does bind to the GUID this driver is binding to too, then it would
> be
> a matter of extending the existing ideapad_wmi_notify() to do the
> same
> as your notify function in this stand-alone driver. Note you can get
> the
> the equivalend of the union acpi_object *data argument in your wmi
> handler
> by calling wmi_get_event_data().
> 
> Regards,
> 
> Hans
> 

Hello Hans,

I did actually start by doing that.
The problem lies with the wmi_get_event_data() function not working
for my ACPI.

wmi_get_event_data() takes the event notify_id as input and is supposed
to give the data associated with the event back. This occures by
calling _WED on the first WMI block that contains said notify_id.

drivers/platform/x86/wmi.c:657:
	list_for_each_entry(wblock, &wmi_block_list, list) {
		struct guid_block *gblock = &wblock->gblock;

		if ((gblock->flags & ACPI_WMI_EVENT) && gblock-
>notify_id == event)
			return get_event_data(wblock, out);
	}

The ACPI of the Lenovo Yoga 9 14IAP7 contains multiple WMI event
blocks hat contain the same notify_id 0xD0.

Here are two of the four WMI objects found in the DSDT:

in _WDG of block WMIY:
	06129D99-6083-4164-81AD-F092F9D773A6:
		notify_id: 0xD0
		instance_count: 1
		flags: 0x8 ACPI_WMI_EVENT

in _WDG of block WMIU:
	8FC0DE0C-B4E4-43FD-B0F3-8871711C1294:
		notify_id: 0xD0
		instance_count: 1
		flags: 0x8 ACPI_WMI_EVENT

These event block belong to different WMI devices and report
unrelated values from different _WED handlers. WMIY for example
triggers its event on "mode changes", e.g. laptop/tablet/tent based
on the accelometers/hinge.
WMIU is the WMI block with the event which reports the special keys
I'm interested in.

WMIY comes before WMIU in the wmi_block_list.
Calling wmi_get_event_data() in ideapad_wmi_notify() calls the wrong
_WED (the one of WMIY) and thus returns the wrong event data.

I noticed that the wmi_driver interface does not incur the problem
with the event because it binds to a wmi_block and calls the _WED
directly without searching through other WMI devices.

I thought of changing the signature of wmi_get_event_data() to include
the GUID of the correct WMI block, but chose wmi_driver instead.
Would you consider adding a wmi_get_event_data_for_guid() function to
the wmi module and using that in the ideapad_wmi_notify function to be
a better solution than the one in the patch presented here?

Regards,
Philipp

> 
> 
> > > +
> > > +struct ideapad_wmi_private {
> > > +       struct wmi_device *wmi_device;
> > > +       struct input_dev *input_dev;
> > > +};
> > > +
> > > +static const struct key_entry ideapad_wmi_fn_key_keymap[] = {
> > > +       /* FnLock (handled by the firmware) */
> > > +       { KE_IGNORE,    0x02 },
> > > +       /* Customizable Lenovo Hotkey ("star" with 'S' inside) */
> > > +       { KE_KEY,       0x01, { KEY_FAVORITES } },
> > > +       /* Dark mode toggle */
> > > +       { KE_KEY,       0x13, { KEY_PROG1 } },
> > > +       /* Sound profile switch */
> > > +       { KE_KEY,       0x12, { KEY_PROG2 } },
> > > +       /* Lenovo Virtual Background application */
> > > +       { KE_KEY,       0x28, { KEY_PROG3 } },
> > > +       /* Lenovo Support */
> > > +       { KE_KEY,       0x27, { KEY_HELP } },
> > > +       /* Refresh Rate Toggle */
> > > +       { KE_KEY,       0x0a, { KEY_DISPLAYTOGGLE } },
> > > +       { KE_END },
> > > +};
> > > +
> > > +static int ideapad_wmi_input_init(struct ideapad_wmi_private
> > > *priv)
> > > +{
> > > +       struct input_dev *input_dev;
> > > +       int err;
> > > +
> > > +       input_dev = input_allocate_device();
> > > +       if (!input_dev) {
> > > +               return -ENOMEM;
> > > +       }
> > > +
> > > +       input_dev->name = "Ideapad WMI Fn Keys";
> > > +       input_dev->phys = IDEAPAD_FN_KEY_EVENT_GUID "/input0";
> > > +       input_dev->id.bustype = BUS_HOST;
> > > +       input_dev->dev.parent = &priv->wmi_device->dev;
> > > +
> > > +       err = sparse_keymap_setup(input_dev,
> > > ideapad_wmi_fn_key_keymap, NULL);
> > > +       if (err) {
> > > +               dev_err(&priv->wmi_device->dev,
> > > +                       "Could not set up input device keymap:
> > > %d\n", err);
> > > +               goto err_free_dev;
> > > +       }
> > > +
> > > +       err = input_register_device(input_dev);
> > > +       if (err) {
> > > +               dev_err(&priv->wmi_device->dev,
> > > +                       "Could not register input device: %d\n",
> > > err);
> > > +               goto err_free_dev;
> > > +       }
> > > +
> > > +       priv->input_dev = input_dev;
> > > +       return 0;
> > > +
> > > +err_free_dev:
> > > +       input_free_device(input_dev);
> > > +       return err;
> > > +}
> > > +
> > > +static void ideapad_wmi_input_exit(struct ideapad_wmi_private
> > > *priv)
> > > +{
> > > +       input_unregister_device(priv->input_dev);
> > > +       priv->input_dev = NULL;
> > > +}
> > > +
> > > +static void ideapad_wmi_input_report(struct ideapad_wmi_private
> > > *priv,
> > > +                                    unsigned int scancode)
> > > +{
> > > +       sparse_keymap_report_event(priv->input_dev, scancode, 1,
> > > true);
> > > +}
> > > +
> > > +static int ideapad_wmi_probe(struct wmi_device *wdev, const void
> > > *ctx)
> > > +{
> > > +       struct ideapad_wmi_private *priv;
> > > +       int err;
> > > +
> > > +       priv = devm_kzalloc(&wdev->dev, sizeof(*priv),
> > > GFP_KERNEL);
> > > +       if (!priv)
> > > +               return -ENOMEM;
> > > +
> > > +       dev_set_drvdata(&wdev->dev, priv);
> > > +
> > > +       priv->wmi_device = wdev;
> > > +
> > > +       err = ideapad_wmi_input_init(priv);
> > > +       if (err)
> > > +               return err;
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static void ideapad_wmi_remove(struct wmi_device *wdev)
> > > +{
> > > +       struct ideapad_wmi_private *priv = dev_get_drvdata(&wdev-
> > > >dev);
> > > +
> > > +       ideapad_wmi_input_exit(priv);
> > > +}
> > > +
> > > +static void ideapad_wmi_notify(struct wmi_device *wdev, union
> > > acpi_object *data)
> > > +{
> > > +       struct ideapad_wmi_private *priv = dev_get_drvdata(&wdev-
> > > >dev);
> > > +
> > > +       if(data->type != ACPI_TYPE_INTEGER) {
> > > +               dev_warn(&priv->wmi_device->dev,
> > > +                       "WMI event data is not an integer\n");
> > > +               return;
> > > +       }
> > > +
> > > +       ideapad_wmi_input_report(priv, data->integer.value);
> > > +}
> > > +
> > > +static const struct wmi_device_id ideapad_wmi_id_table[] = {
> > > +       {       /* Special Keys on the Yoga 9 14IAP7 */
> > > +               .guid_string = IDEAPAD_FN_KEY_EVENT_GUID
> > > +       },
> > > +       { }
> > > +};
> > > +
> > > +static struct wmi_driver ideapad_wmi_driver = {
> > > +       .driver = {
> > > +               .name = "ideapad-wmi",
> > > +       },
> > > +       .id_table = ideapad_wmi_id_table,
> > > +       .probe = ideapad_wmi_probe,
> > > +       .remove = ideapad_wmi_remove,
> > > +       .notify = ideapad_wmi_notify,
> > > +};
> > > +
> > > +module_wmi_driver(ideapad_wmi_driver);
> > > +
> > > +MODULE_DEVICE_TABLE(wmi, ideapad_wmi_id_table);
> > > +MODULE_AUTHOR("Philipp Jungkamp <p.jungkamp@gmx.net>");
> > > +MODULE_DESCRIPTION("Ideapad WMI fn keys driver");
> > > +MODULE_LICENSE("GPL");
> > > --
> > > 2.37.3
> > > 
> 

Re: [PATCH] Add IdeaPad WMI Fn Keys driver

[Hans de Goede]

Hi Philipp,

On 11/10/22 21:02, Philipp Jungkamp wrote:
> On Mon, 2022-09-19 at 08:54 +0100, Hans de Goede wrote:
>> Hi again,
>>
>> On 9/11/22 17:18, Hans de Goede wrote:
>>> Hi Philipp,
>>>
>>> On 9/11/22 18:04, Philipp Jungkamp wrote:
>>>> Create an input device for WMI events corresponding to some
>>>> special
>>>> keys on the 'Lenovo Yoga' line.
>>>>
>>>> This include the 3 keys to the right on the 'Lenovo Yoga9 14IAP7'
>>>> and
>>>> additionally the 'Lenovo Support' and 'Lenovo Favorites' (star
>>>> with 'S'
>>>> inside) in the fn key row as well as the event emitted on 'Fn+R'
>>>> which
>>>> toggles between 60Hz and 90Hz display refresh rate on windows.
>>>>
>>>> Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
>>>> ---
>>>> I found this patch by poking in the DSDT. I have not submitted
>>>> any
>>>> notable patches yet and hope you can help me improve in case I
>>>> make
>>>> unfortunate choices during submission.
>>>
>>> No worries at a first glance (I have not looked at this in any
>>> detail yet) this looks pretty good for a first submission.
>>>
>>> And thank you for contributing to the Linux kernel!
>>>
>>>
>>>> Philipp Jungkamp
>>>>
>>>>  drivers/platform/x86/Kconfig       |  13 +++
>>>>  drivers/platform/x86/Makefile      |   1 +
>>>>  drivers/platform/x86/ideapad-wmi.c | 153
>>>> +++++++++++++++++++++++++++++
>>>>  3 files changed, 167 insertions(+)
>>>>  create mode 100644 drivers/platform/x86/ideapad-wmi.c
>>>>
>>>> diff --git a/drivers/platform/x86/Kconfig
>>>> b/drivers/platform/x86/Kconfig
>>>> index f2f98e942cf2..e7c5148e5cb4 100644
>>>> --- a/drivers/platform/x86/Kconfig
>>>> +++ b/drivers/platform/x86/Kconfig
>>>> @@ -140,6 +140,19 @@ config YOGABOOK_WMI
>>>>           To compile this driver as a module, choose M here: the
>>>> module will
>>>>           be called lenovo-yogabook-wmi.
>>>>
>>>> +config IDEAPAD_WMI
>>>> +       tristate "Lenovo IdeaPad WMI Fn Keys"
>>>> +       depends on ACPI_WMI
>>>> +       depends on INPUT
>>>> +       select INPUT_SPARSEKMAP
>>>> +       help
>>>> +         Say Y here if you want to receive key presses from some
>>>> lenovo
>>>> +         specific keys. (Star Key, Support Key, Virtual
>>>> Background,
>>>> +         Dark Mode Toggle, ...)
>>>> +
>>>> +         To compile this driver as a module, choose M here: the
>>>> module will
>>>> +         be called ideapad-wmi.
>>>> +
>>>>  config ACERHDF
>>>>         tristate "Acer Aspire One temperature and fan driver"
>>>>         depends on ACPI && THERMAL
>>>> diff --git a/drivers/platform/x86/Makefile
>>>> b/drivers/platform/x86/Makefile
>>>> index 5a428caa654a..d8bec884d6bc 100644
>>>> --- a/drivers/platform/x86/Makefile
>>>> +++ b/drivers/platform/x86/Makefile
>>>> @@ -16,6 +16,7 @@ obj-
>>>> $(CONFIG_PEAQ_WMI)                        += peaq-wmi.o
>>>>  obj-$(CONFIG_XIAOMI_WMI)               += xiaomi-wmi.o
>>>>  obj-$(CONFIG_GIGABYTE_WMI)             += gigabyte-wmi.o
>>>>  obj-$(CONFIG_YOGABOOK_WMI)             += lenovo-yogabook-wmi.o
>>>> +obj-$(CONFIG_IDEAPAD_WMI)              += ideapad-wmi.o
>>>>
>>>>  # Acer
>>>>  obj-$(CONFIG_ACERHDF)          += acerhdf.o
>>>> diff --git a/drivers/platform/x86/ideapad-wmi.c
>>>> b/drivers/platform/x86/ideapad-wmi.c
>>>> new file mode 100644
>>>> index 000000000000..38f7b3d0c171
>>>> --- /dev/null
>>>> +++ b/drivers/platform/x86/ideapad-wmi.c
>>>> @@ -0,0 +1,153 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>>> +/*
>>>> + * ideapad-wmi.c - Ideapad WMI fn keys driver
>>>> + *
>>>> + * Copyright (C) 2022 Philipp Jungkamp <p.jungkamp@gmx.net>
>>>> + */
>>>> +
>>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>>> +
>>>> +#include <linux/acpi.h>
>>>> +#include <linux/input.h>
>>>> +#include <linux/input/sparse-keymap.h>
>>>> +#include <linux/list.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/wmi.h>
>>>> +
>>>> +#define IDEAPAD_FN_KEY_EVENT_GUID      "8FC0DE0C-B4E4-43FD-B0F3-
>>>> 8871711C1294"
>>>
>>> At a first hunch (basically huh, don't we have a driver for that
>>> already?) I grepped through the kernel sources and found:
>>>
>>> drivers/platform/x86/ideapad-laptop.c
>>>
>>> can you see if you can make things work with that driver?
>>
>> So I have taken a quick look at this and it seems to me that this
>> really should be able to work with the existing ideapad-laptop.c code
>> ?
>>
>> For starters you could add a debug printk / dev_info to this block,
>>
>> #if IS_ENABLED(CONFIG_ACPI_WMI)
>>         for (i = 0; i < ARRAY_SIZE(ideapad_wmi_fnesc_events); i++) {
>>                 status =
>> wmi_install_notify_handler(ideapad_wmi_fnesc_events[i],
>>                                                    
>> ideapad_wmi_notify, priv);
>>                 if (ACPI_SUCCESS(status)) {
>>                         priv->fnesc_guid =
>> ideapad_wmi_fnesc_events[i];
>>                         break;
>>                 }
>>         }
>>
>>         if (ACPI_FAILURE(status) && status != AE_NOT_EXIST) {
>>                 err = -EIO;
>>                 goto notification_failed_wmi;
>>         }
>> #endif
>>
>> checking which event GUID ideapad-laptop binds to on your laptop.
>> Assuming that
>> it does bind to the GUID this driver is binding to too, then it would
>> be
>> a matter of extending the existing ideapad_wmi_notify() to do the
>> same
>> as your notify function in this stand-alone driver. Note you can get
>> the
>> the equivalend of the union acpi_object *data argument in your wmi
>> handler
>> by calling wmi_get_event_data().
>>
>> Regards,
>>
>> Hans
>>
> 
> Hello Hans,
> 
> I did actually start by doing that.
> The problem lies with the wmi_get_event_data() function not working
> for my ACPI.
> 
> wmi_get_event_data() takes the event notify_id as input and is supposed
> to give the data associated with the event back. This occures by
> calling _WED on the first WMI block that contains said notify_id.
> 
> drivers/platform/x86/wmi.c:657:
> 	list_for_each_entry(wblock, &wmi_block_list, list) {
> 		struct guid_block *gblock = &wblock->gblock;
> 
> 		if ((gblock->flags & ACPI_WMI_EVENT) && gblock-
>> notify_id == event)
> 			return get_event_data(wblock, out);
> 	}
> 
> The ACPI of the Lenovo Yoga 9 14IAP7 contains multiple WMI event
> blocks hat contain the same notify_id 0xD0.
> 
> Here are two of the four WMI objects found in the DSDT:
> 
> in _WDG of block WMIY:
> 	06129D99-6083-4164-81AD-F092F9D773A6:
> 		notify_id: 0xD0
> 		instance_count: 1
> 		flags: 0x8 ACPI_WMI_EVENT
> 
> in _WDG of block WMIU:
> 	8FC0DE0C-B4E4-43FD-B0F3-8871711C1294:
> 		notify_id: 0xD0
> 		instance_count: 1
> 		flags: 0x8 ACPI_WMI_EVENT
> 
> These event block belong to different WMI devices and report
> unrelated values from different _WED handlers. WMIY for example
> triggers its event on "mode changes", e.g. laptop/tablet/tent based
> on the accelometers/hinge.
> WMIU is the WMI block with the event which reports the special keys
> I'm interested in.
> 
> WMIY comes before WMIU in the wmi_block_list.
> Calling wmi_get_event_data() in ideapad_wmi_notify() calls the wrong
> _WED (the one of WMIY) and thus returns the wrong event data.
> 
> I noticed that the wmi_driver interface does not incur the problem
> with the event because it binds to a wmi_block and calls the _WED
> directly without searching through other WMI devices.

Right, I see in that case I guess that using the wmi_driver
interface does make sense.

> I thought of changing the signature of wmi_get_event_data() to include
> the GUID of the correct WMI block, but chose wmi_driver instead.
> Would you consider adding a wmi_get_event_data_for_guid() function to
> the wmi module and using that in the ideapad_wmi_notify function to be
> a better solution than the one in the patch presented here?

So the problem is that ideapad-laptop is using the old interface
and it does:

                status = wmi_install_notify_handler(ideapad_wmi_fnesc_events[i],
                                                    ideapad_wmi_notify, priv);

And then the code to call that notifier in wmi.c goes like this:

        if (test_bit(WMI_PROBED, &wblock->flags) && wblock->dev.dev.driver) {
                struct wmi_driver *driver = drv_to_wdrv(wblock->dev.dev.driver);
                struct acpi_buffer evdata = { ACPI_ALLOCATE_BUFFER, NULL };
                acpi_status status;

                if (!driver->no_notify_data) {
                        status = get_event_data(wblock, &evdata);
                        if (ACPI_FAILURE(status)) {
                                dev_warn(&wblock->dev.dev, "failed to get event data\n");
                                return;
                        }
                }

                if (driver->notify)
                        driver->notify(&wblock->dev, evdata.pointer);

                kfree(evdata.pointer);
        } else if (wblock->handler) {
                /* Legacy handler */
                wblock->handler(event, wblock->handler_data);
        }

So if we move ahead with your new style wmi-driver for GUID 
8FC0DE0C-B4E4-43FD-B0F3-8871711C1294.

Then we hit the "true" path of the if and the legacy handler
(registered by ideapad-laptop) will never trigger.

I guess you could argue that this is a feature since your
new-style driver "replaces" ideapad-laptop, but ending up with
2 drivers for ideapad laptops / for GUID
8FC0DE0C-B4E4-43FD-B0F3-8871711C1294 and them having them have
to coordinate to decide which one to load sounds less then ideal.

So I think that your idea to add a wmi_get_event_data_for_guid()
function to wmi.c is actually a good idea.

Can you give that a try please and see how that goes ?

Regards,

Hans

[PATCH v2 1/2] platform/x86: wmi: Disambiguate WMI event data on some ACPI tables

[Philipp Jungkamp]

The ACPI DSDT table includes multiple WMI blocks which emit events with
the same notify_id. The wmi_get_event_data() function chooses the
wmi_block with the _WED handler to call based on the notify_id. This
function may call the wrong _WED event handler based on the order the
WMI blocks are parsed.

This introduces wmi_get_event_data_with_guid() to diambiguate the _WED
call to get metadata for an event. The GUID here is the one of the
containing WMI block, not the one of the WMI event itself.

Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
---
Was separating this change into it's own commit correct?

 drivers/platform/x86/wmi.c | 30 ++++++++++++++++++++++++++++++
 include/linux/acpi.h       |  3 +++
 2 files changed, 33 insertions(+)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 223550a10d4d..56b666f4b40b 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -659,6 +659,36 @@ acpi_status wmi_get_event_data(u32 event, struct acpi_buffer *out)
 }
 EXPORT_SYMBOL_GPL(wmi_get_event_data);

+/**
+ * wmi_get_event_data_with_guid - Get WMI data associated with an event by guid
+ *
+ * Consider using this instead of wmi_get_event_data() when the notify_id
+ * of the WMI event may not be unique among all WMI blocks of a device.
+ *
+ * @guid: GUID of the WMI block for this event
+ * @event: Event to find
+ * @out: Buffer to hold event data. out->pointer should be freed with kfree()
+ *
+ * Returns extra data associated with an event in WMI.
+ */
+acpi_status wmi_get_event_data_with_guid(const char *guid, u32 event, struct acpi_buffer *out)
+{
+	struct wmi_block *wblock = NULL;
+	struct guid_block *gblock;
+	acpi_status status;
+
+	status = find_guid(guid, &wblock);
+	if (ACPI_FAILURE(status))
+		return AE_NOT_FOUND;
+
+	gblock = &wblock->gblock;
+	if ((gblock->flags & ACPI_WMI_EVENT) && gblock->notify_id == event)
+		return get_event_data(wblock, out);
+
+	return AE_NOT_FOUND;
+}
+EXPORT_SYMBOL_GPL(wmi_get_event_data_with_guid);
+
 /**
  * wmi_has_guid - Check if a GUID is available
  * @guid_string: 36 char string of the form fa50ff2b-f2e8-45de-83fa-65417f2f49ba
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 3015235d65e3..51ac4d6bcae1 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -423,6 +423,9 @@ extern acpi_status wmi_set_block(const char *guid, u8 instance,
 extern acpi_status wmi_install_notify_handler(const char *guid,
 					wmi_notify_handler handler, void *data);
 extern acpi_status wmi_remove_notify_handler(const char *guid);
+extern acpi_status wmi_get_event_data_with_guid(const char *guid,
+						u32 event,
+						struct acpi_buffer *out);
 extern acpi_status wmi_get_event_data(u32 event, struct acpi_buffer *out);
 extern bool wmi_has_guid(const char *guid);
 extern char *wmi_get_acpi_device_uid(const char *guid);
--
2.38.1

[PATCH v2 2/2] platform/x86: ideapad-laptop: support for more special keys in WMI

[Philipp Jungkamp]

The event data of the WMI event 0xD0, which is assumed to be the
fn_lock, is used to indicate several special keys on newer Yoga 7/9
laptops.

Add support for these keys using the wmi_get_event_data_with_guid()
function.

Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
---
I can't really judge how this would affect other devices which depend on the
event to toggle their fn-lock light unconditionally.

One could check whether the event data on those devices makes it clear when
exactly the eval_hals()/exec_sals() path is needed. This should be observable
through the dev_dbg() put before the ideapad_input_report().

 drivers/platform/x86/ideapad-laptop.c | 46 +++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 33b3dfdd1b08..2ade423c7813 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -1074,6 +1074,7 @@ static void ideapad_sysfs_exit(struct ideapad_private *priv)
 /*
  * input device
  */
+#define IDEAPAD_WMI_KEY 0x100
 static const struct key_entry ideapad_keymap[] = {
 	{ KE_KEY,   6, { KEY_SWITCHVIDEOMODE } },
 	{ KE_KEY,   7, { KEY_CAMERA } },
@@ -1087,6 +1088,26 @@ static const struct key_entry ideapad_keymap[] = {
 	{ KE_KEY,  66, { KEY_TOUCHPAD_OFF } },
 	{ KE_KEY,  67, { KEY_TOUCHPAD_ON } },
 	{ KE_KEY, 128, { KEY_ESC } },
+
+	/*
+	 * WMI keys
+	 */
+
+	/* FnLock (handled by the firmware) */
+	{ KE_IGNORE,	0x02 | IDEAPAD_WMI_KEY },
+	/* Customizable Lenovo Hotkey ("star" with 'S' inside) */
+	{ KE_KEY,	0x01 | IDEAPAD_WMI_KEY, { KEY_FAVORITES } },
+	/* Dark mode toggle */
+	{ KE_KEY,	0x13 | IDEAPAD_WMI_KEY, { KEY_PROG1 } },
+	/* Sound profile switch */
+	{ KE_KEY,	0x12 | IDEAPAD_WMI_KEY, { KEY_PROG2 } },
+	/* Lenovo Virtual Background application */
+	{ KE_KEY,	0x28 | IDEAPAD_WMI_KEY, { KEY_PROG3 } },
+	/* Lenovo Support */
+	{ KE_KEY,	0x27 | IDEAPAD_WMI_KEY, { KEY_HELP } },
+	/* Refresh Rate Toggle */
+	{ KE_KEY,	0x0a | IDEAPAD_WMI_KEY, { KEY_DISPLAYTOGGLE } },
+
 	{ KE_END },
 };

@@ -1496,6 +1517,10 @@ static void ideapad_wmi_notify(u32 value, void *context)
 	struct ideapad_private *priv = context;
 	unsigned long result;

+	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *data;
+	acpi_status status;
+
 	switch (value) {
 	case 128:
 		ideapad_input_report(priv, value);
@@ -1506,6 +1531,27 @@ static void ideapad_wmi_notify(u32 value, void *context)

 			exec_sals(priv->adev->handle, state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
 		}
+
+		status = wmi_get_event_data_with_guid(priv->fnesc_guid, value, &response);
+		if (ACPI_FAILURE(status)) {
+			dev_warn(&priv->platform_device->dev,
+				"Bad WMI event data 0x%x\n", status);
+			break;
+		}
+
+		data = (union acpi_object *) response.pointer;
+		if (data->type != ACPI_TYPE_INTEGER) {
+			dev_warn(&priv->platform_device->dev,
+				"WMI event data is not an integer\n");
+		} else {
+			dev_dbg(&priv->platform_device->dev,
+				"WMI event: notify_id=%d, data=%d\n",
+				value,
+				data->integer.value);
+			ideapad_input_report(priv, data->integer.value | IDEAPAD_WMI_KEY);
+		}
+
+		kfree(response.pointer);
 		break;
 	default:
 		dev_info(&priv->platform_device->dev,
--
2.38.1

Re: [PATCH v2 1/2] platform/x86: wmi: Disambiguate WMI event data on some ACPI tables

[Armin Wolf]

Am 13.11.22 um 13:12 schrieb Philipp Jungkamp:

> The ACPI DSDT table includes multiple WMI blocks which emit events with
> the same notify_id. The wmi_get_event_data() function chooses the
> wmi_block with the _WED handler to call based on the notify_id. This
> function may call the wrong _WED event handler based on the order the
> WMI blocks are parsed.
>
> This introduces wmi_get_event_data_with_guid() to diambiguate the _WED
> call to get metadata for an event. The GUID here is the one of the
> containing WMI block, not the one of the WMI event itself.

Hello,

maybe it would be better to instead rewrite the driver to utilize the WMI bus infrastructure?
Because a GUID is not guaranteed to be unique inside a system, there would still be a chance
to call the wrong _WED handler.
AFAIK only utilizing the WMI bus infrastructure would fully disambiguate the WMI event data.

Armin Wolf

> Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
> ---
> Was separating this change into it's own commit correct?
>
>   drivers/platform/x86/wmi.c | 30 ++++++++++++++++++++++++++++++
>   include/linux/acpi.h       |  3 +++
>   2 files changed, 33 insertions(+)
>
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index 223550a10d4d..56b666f4b40b 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -659,6 +659,36 @@ acpi_status wmi_get_event_data(u32 event, struct acpi_buffer *out)
>   }
>   EXPORT_SYMBOL_GPL(wmi_get_event_data);
>
> +/**
> + * wmi_get_event_data_with_guid - Get WMI data associated with an event by guid
> + *
> + * Consider using this instead of wmi_get_event_data() when the notify_id
> + * of the WMI event may not be unique among all WMI blocks of a device.
> + *
> + * @guid: GUID of the WMI block for this event
> + * @event: Event to find
> + * @out: Buffer to hold event data. out->pointer should be freed with kfree()
> + *
> + * Returns extra data associated with an event in WMI.
> + */
> +acpi_status wmi_get_event_data_with_guid(const char *guid, u32 event, struct acpi_buffer *out)
> +{
> +	struct wmi_block *wblock = NULL;
> +	struct guid_block *gblock;
> +	acpi_status status;
> +
> +	status = find_guid(guid, &wblock);
> +	if (ACPI_FAILURE(status))
> +		return AE_NOT_FOUND;
> +
> +	gblock = &wblock->gblock;
> +	if ((gblock->flags & ACPI_WMI_EVENT) && gblock->notify_id == event)
> +		return get_event_data(wblock, out);
> +
> +	return AE_NOT_FOUND;
> +}
> +EXPORT_SYMBOL_GPL(wmi_get_event_data_with_guid);
> +
>   /**
>    * wmi_has_guid - Check if a GUID is available
>    * @guid_string: 36 char string of the form fa50ff2b-f2e8-45de-83fa-65417f2f49ba
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 3015235d65e3..51ac4d6bcae1 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -423,6 +423,9 @@ extern acpi_status wmi_set_block(const char *guid, u8 instance,
>   extern acpi_status wmi_install_notify_handler(const char *guid,
>   					wmi_notify_handler handler, void *data);
>   extern acpi_status wmi_remove_notify_handler(const char *guid);
> +extern acpi_status wmi_get_event_data_with_guid(const char *guid,
> +						u32 event,
> +						struct acpi_buffer *out);
>   extern acpi_status wmi_get_event_data(u32 event, struct acpi_buffer *out);
>   extern bool wmi_has_guid(const char *guid);
>   extern char *wmi_get_acpi_device_uid(const char *guid);
> --
> 2.38.1
>

Re: [PATCH v2 1/2] platform/x86: wmi: Disambiguate WMI event data on some ACPI tables

[Philipp Jungkamp]

On Sun, 2022-11-13 at 21:30 +0100, Armin Wolf wrote:
> Am 13.11.22 um 13:12 schrieb Philipp Jungkamp:
> 
> > The ACPI DSDT table includes multiple WMI blocks which emit events
> > with
> > the same notify_id. The wmi_get_event_data() function chooses the
> > wmi_block with the _WED handler to call based on the notify_id.
> > This
> > function may call the wrong _WED event handler based on the order
> > the
> > WMI blocks are parsed.
> > 
> > This introduces wmi_get_event_data_with_guid() to diambiguate the
> > _WED
> > call to get metadata for an event. The GUID here is the one of the
> > containing WMI block, not the one of the WMI event itself.
> 
> Hello,
> 
> maybe it would be better to instead rewrite the driver to utilize the
> WMI bus infrastructure?
> Because a GUID is not guaranteed to be unique inside a system, there
> would still be a chance
> to call the wrong _WED handler.
> AFAIK only utilizing the WMI bus infrastructure would fully
> disambiguate the WMI event data.
> 
> Armin Wolf
> 
Hello,

I thought the same, so I implemented the bare minimum to support the
GUID on my hardware using the WMI bus in the v1 patch.

But I also agree with Hans in that it should probably reside in the
ideapad-laptop driver, so there aren't two modules for the same
hardware.

Therefore I'd need to change the WMI handling part of ideapad-laptop in
a way that does not break other hardware.

I don't know how to write a driver which connects on two buses.
The driver is currently for a platform device with the corresponding
probe and remove hooks. How would I coordinate the private data
allocation/deletion between the  platform-probe/-remove and wmi-probe/-
remove in a way that is established by other drivers.

Do you have a recommendation on how to setup a module to register on
two buses or another driver that is good to learn from?

Greetings,
Philipp Jungkamp

> > Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
> > ---
> > Was separating this change into it's own commit correct?
> > 
> >   drivers/platform/x86/wmi.c | 30 ++++++++++++++++++++++++++++++
> >   include/linux/acpi.h       |  3 +++
> >   2 files changed, 33 insertions(+)
> > 
> > diff --git a/drivers/platform/x86/wmi.c
> > b/drivers/platform/x86/wmi.c
> > index 223550a10d4d..56b666f4b40b 100644
> > --- a/drivers/platform/x86/wmi.c
> > +++ b/drivers/platform/x86/wmi.c
> > @@ -659,6 +659,36 @@ acpi_status wmi_get_event_data(u32 event,
> > struct acpi_buffer *out)
> >   }
> >   EXPORT_SYMBOL_GPL(wmi_get_event_data);
> > 
> > +/**
> > + * wmi_get_event_data_with_guid - Get WMI data associated with an
> > event by guid
> > + *
> > + * Consider using this instead of wmi_get_event_data() when the
> > notify_id
> > + * of the WMI event may not be unique among all WMI blocks of a
> > device.
> > + *
> > + * @guid: GUID of the WMI block for this event
> > + * @event: Event to find
> > + * @out: Buffer to hold event data. out->pointer should be freed
> > with kfree()
> > + *
> > + * Returns extra data associated with an event in WMI.
> > + */
> > +acpi_status wmi_get_event_data_with_guid(const char *guid, u32
> > event, struct acpi_buffer *out)
> > +{
> > +       struct wmi_block *wblock = NULL;
> > +       struct guid_block *gblock;
> > +       acpi_status status;
> > +
> > +       status = find_guid(guid, &wblock);
> > +       if (ACPI_FAILURE(status))
> > +               return AE_NOT_FOUND;
> > +
> > +       gblock = &wblock->gblock;
> > +       if ((gblock->flags & ACPI_WMI_EVENT) && gblock->notify_id
> > == event)
> > +               return get_event_data(wblock, out);
> > +
> > +       return AE_NOT_FOUND;
> > +}
> > +EXPORT_SYMBOL_GPL(wmi_get_event_data_with_guid);
> > +
> >   /**
> >    * wmi_has_guid - Check if a GUID is available
> >    * @guid_string: 36 char string of the form fa50ff2b-f2e8-45de-
> > 83fa-65417f2f49ba
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > index 3015235d65e3..51ac4d6bcae1 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -423,6 +423,9 @@ extern acpi_status wmi_set_block(const char
> > *guid, u8 instance,
> >   extern acpi_status wmi_install_notify_handler(const char *guid,
> >                                         wmi_notify_handler handler,
> > void *data);
> >   extern acpi_status wmi_remove_notify_handler(const char *guid);
> > +extern acpi_status wmi_get_event_data_with_guid(const char *guid,
> > +                                               u32 event,
> > +                                               struct acpi_buffer
> > *out);
> >   extern acpi_status wmi_get_event_data(u32 event, struct
> > acpi_buffer *out);
> >   extern bool wmi_has_guid(const char *guid);
> >   extern char *wmi_get_acpi_device_uid(const char *guid);
> > --
> > 2.38.1
> > 

Re: [PATCH v2 1/2] platform/x86: wmi: Disambiguate WMI event data on some ACPI tables

[Armin Wolf]

Am 13.11.22 um 22:42 schrieb Philipp Jungkamp:

> On Sun, 2022-11-13 at 21:30 +0100, Armin Wolf wrote:
>> Am 13.11.22 um 13:12 schrieb Philipp Jungkamp:
>>
>>> The ACPI DSDT table includes multiple WMI blocks which emit events
>>> with
>>> the same notify_id. The wmi_get_event_data() function chooses the
>>> wmi_block with the _WED handler to call based on the notify_id.
>>> This
>>> function may call the wrong _WED event handler based on the order
>>> the
>>> WMI blocks are parsed.
>>>
>>> This introduces wmi_get_event_data_with_guid() to diambiguate the
>>> _WED
>>> call to get metadata for an event. The GUID here is the one of the
>>> containing WMI block, not the one of the WMI event itself.
>> Hello,
>>
>> maybe it would be better to instead rewrite the driver to utilize the
>> WMI bus infrastructure?
>> Because a GUID is not guaranteed to be unique inside a system, there
>> would still be a chance
>> to call the wrong _WED handler.
>> AFAIK only utilizing the WMI bus infrastructure would fully
>> disambiguate the WMI event data.
>>
>> Armin Wolf
>>
> Hello,
>
> I thought the same, so I implemented the bare minimum to support the
> GUID on my hardware using the WMI bus in the v1 patch.
>
> But I also agree with Hans in that it should probably reside in the
> ideapad-laptop driver, so there aren't two modules for the same
> hardware.
>
> Therefore I'd need to change the WMI handling part of ideapad-laptop in
> a way that does not break other hardware.
>
> I don't know how to write a driver which connects on two buses.
> The driver is currently for a platform device with the corresponding
> probe and remove hooks. How would I coordinate the private data
> allocation/deletion between the  platform-probe/-remove and wmi-probe/-
> remove in a way that is established by other drivers.
>
> Do you have a recommendation on how to setup a module to register on
> two buses or another driver that is good to learn from?
>
> Greetings,
> Philipp Jungkamp

Well, the current driver does the WMI registration when probing the platform device,
so maybe you could just call wmi_driver_register() there?
Since the WMI notify handler may need to access the platform device, i suggest to
store the wmi_driver struct inside the ideapad_private struct, and then use container_of()
to get a reference from wmi_device->dev->driver to ideapad_private.
This should be safe as long as the WMI driver is unregistered on platform device removal.

Armin Wolf

>>> Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
>>> ---
>>> Was separating this change into it's own commit correct?
>>>
>>>    drivers/platform/x86/wmi.c | 30 ++++++++++++++++++++++++++++++
>>>    include/linux/acpi.h       |  3 +++
>>>    2 files changed, 33 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/wmi.c
>>> b/drivers/platform/x86/wmi.c
>>> index 223550a10d4d..56b666f4b40b 100644
>>> --- a/drivers/platform/x86/wmi.c
>>> +++ b/drivers/platform/x86/wmi.c
>>> @@ -659,6 +659,36 @@ acpi_status wmi_get_event_data(u32 event,
>>> struct acpi_buffer *out)
>>>    }
>>>    EXPORT_SYMBOL_GPL(wmi_get_event_data);
>>>
>>> +/**
>>> + * wmi_get_event_data_with_guid - Get WMI data associated with an
>>> event by guid
>>> + *
>>> + * Consider using this instead of wmi_get_event_data() when the
>>> notify_id
>>> + * of the WMI event may not be unique among all WMI blocks of a
>>> device.
>>> + *
>>> + * @guid: GUID of the WMI block for this event
>>> + * @event: Event to find
>>> + * @out: Buffer to hold event data. out->pointer should be freed
>>> with kfree()
>>> + *
>>> + * Returns extra data associated with an event in WMI.
>>> + */
>>> +acpi_status wmi_get_event_data_with_guid(const char *guid, u32
>>> event, struct acpi_buffer *out)
>>> +{
>>> +       struct wmi_block *wblock = NULL;
>>> +       struct guid_block *gblock;
>>> +       acpi_status status;
>>> +
>>> +       status = find_guid(guid, &wblock);
>>> +       if (ACPI_FAILURE(status))
>>> +               return AE_NOT_FOUND;
>>> +
>>> +       gblock = &wblock->gblock;
>>> +       if ((gblock->flags & ACPI_WMI_EVENT) && gblock->notify_id
>>> == event)
>>> +               return get_event_data(wblock, out);
>>> +
>>> +       return AE_NOT_FOUND;
>>> +}
>>> +EXPORT_SYMBOL_GPL(wmi_get_event_data_with_guid);
>>> +
>>>    /**
>>>     * wmi_has_guid - Check if a GUID is available
>>>     * @guid_string: 36 char string of the form fa50ff2b-f2e8-45de-
>>> 83fa-65417f2f49ba
>>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>>> index 3015235d65e3..51ac4d6bcae1 100644
>>> --- a/include/linux/acpi.h
>>> +++ b/include/linux/acpi.h
>>> @@ -423,6 +423,9 @@ extern acpi_status wmi_set_block(const char
>>> *guid, u8 instance,
>>>    extern acpi_status wmi_install_notify_handler(const char *guid,
>>>                                          wmi_notify_handler handler,
>>> void *data);
>>>    extern acpi_status wmi_remove_notify_handler(const char *guid);
>>> +extern acpi_status wmi_get_event_data_with_guid(const char *guid,
>>> +                                               u32 event,
>>> +                                               struct acpi_buffer
>>> *out);
>>>    extern acpi_status wmi_get_event_data(u32 event, struct
>>> acpi_buffer *out);
>>>    extern bool wmi_has_guid(const char *guid);
>>>    extern char *wmi_get_acpi_device_uid(const char *guid);
>>> --
>>> 2.38.1
>>>

[PATCH v3] platform/x86: ideapad-laptop: support for more special keys in WMI

[Philipp Jungkamp]

The event data of the WMI event 0xD0, which is assumed to be the
fn_lock, is used to indicate several special keys on newer Yoga 7/9
laptops.

The notify_id 0xD0 is non-unique in the DSDT of the Yoga 9 14IAP7, this
causes wmi_get_event_data() to report wrong values.
Port the ideapad-laptop WMI code to the wmi bus infrastructure which
does not suffer from the shortcomings of wmi_get_event_data().

Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
---
Hello,

is this about right? It works for me.

What I don't really like here is the dev_set_drvdata() which takes a non-const
void * and I pass it a const pointer. I do cast the value of dev_get_drvdata()
back to a const pointer, but this seems rather ugly.
I preferred it over allocating a single int for the device or casting an enum
to a void *. This additionally removes the need for a remove funtion.

Regards,
Philipp

 drivers/platform/x86/ideapad-laptop.c | 109 +++++++++++++++++++-------
 1 file changed, 80 insertions(+), 29 deletions(-)

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 33b3dfdd1b08..6d35a9e961cf 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -30,6 +30,7 @@
 #include <linux/seq_file.h>
 #include <linux/sysfs.h>
 #include <linux/types.h>
+#include <linux/wmi.h>

 #include <acpi/video.h>

@@ -38,10 +39,19 @@
 #define IDEAPAD_RFKILL_DEV_NUM	3

 #if IS_ENABLED(CONFIG_ACPI_WMI)
-static const char *const ideapad_wmi_fnesc_events[] = {
-	"26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", /* Yoga 3 */
-	"56322276-8493-4CE8-A783-98C991274F5E", /* Yoga 700 */
-	"8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", /* Legion 5 */
+enum ideapad_wmi_event_type {
+	IDEAPAD_WMI_EVENT_ESC,
+	IDEAPAD_WMI_EVENT_FN_KEYS,
+};
+
+enum ideapad_wmi_event_type ideapad_wmi_esc = IDEAPAD_WMI_EVENT_ESC,
+enum ideapad_wmi_event_type ideapad_wmi_fn_keys = IDEAPAD_WMI_EVENT_FN_KEYS;
+
+static const struct wmi_device_id ideapad_wmi_id_table[] = {
+	{ "26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", &ideapad_wmi_esc }, /* Yoga 3 */
+	{ "56322276-8493-4CE8-A783-98C991274F5E", &ideapad_wmi_esc }, /* Yoga 700 */
+	{ "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", &ideapad_wmi_fn_keys }, /* Legion 5 */
+	{}
 };
 #endif

@@ -130,7 +140,7 @@ struct ideapad_private {
 	struct ideapad_dytc_priv *dytc;
 	struct dentry *debug;
 	unsigned long cfg;
-	const char *fnesc_guid;
+	struct wmi_driver wmi_drv;
 	struct {
 		bool conservation_mode    : 1;
 		bool dytc                 : 1;
@@ -1074,6 +1084,7 @@ static void ideapad_sysfs_exit(struct ideapad_private *priv)
 /*
  * input device
  */
+#define IDEAPAD_WMI_KEY 0x100
 static const struct key_entry ideapad_keymap[] = {
 	{ KE_KEY,   6, { KEY_SWITCHVIDEOMODE } },
 	{ KE_KEY,   7, { KEY_CAMERA } },
@@ -1087,6 +1098,26 @@ static const struct key_entry ideapad_keymap[] = {
 	{ KE_KEY,  66, { KEY_TOUCHPAD_OFF } },
 	{ KE_KEY,  67, { KEY_TOUCHPAD_ON } },
 	{ KE_KEY, 128, { KEY_ESC } },
+
+	/*
+	 * WMI keys
+	 */
+
+	/* FnLock (handled by the firmware) */
+	{ KE_IGNORE,	0x02 | IDEAPAD_WMI_KEY },
+	/* Customizable Lenovo Hotkey ("star" with 'S' inside) */
+	{ KE_KEY,	0x01 | IDEAPAD_WMI_KEY, { KEY_FAVORITES } },
+	/* Dark mode toggle */
+	{ KE_KEY,	0x13 | IDEAPAD_WMI_KEY, { KEY_PROG1 } },
+	/* Sound profile switch */
+	{ KE_KEY,	0x12 | IDEAPAD_WMI_KEY, { KEY_PROG2 } },
+	/* Lenovo Virtual Background application */
+	{ KE_KEY,	0x28 | IDEAPAD_WMI_KEY, { KEY_PROG3 } },
+	/* Lenovo Support */
+	{ KE_KEY,	0x27 | IDEAPAD_WMI_KEY, { KEY_HELP } },
+	/* Refresh Rate Toggle */
+	{ KE_KEY,	0x0a | IDEAPAD_WMI_KEY, { KEY_DISPLAYTOGGLE } },
+
 	{ KE_END },
 };

@@ -1491,25 +1522,47 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
 }

 #if IS_ENABLED(CONFIG_ACPI_WMI)
-static void ideapad_wmi_notify(u32 value, void *context)
+static int ideapad_wmi_probe(struct wmi_device *wdev, const void *context)
 {
-	struct ideapad_private *priv = context;
+	dev_set_drvdata(&wdev->dev, (void *) context);
+	return 0;
+}
+
+static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
+{
+	struct wmi_driver *wdrv = container_of(wdev->dev.driver,
+					       struct wmi_driver,
+					       driver);
+	struct ideapad_private *priv = container_of(wdrv,
+						    struct ideapad_private,
+						    wmi_drv);
+	const enum ideapad_wmi_event_type *event = dev_get_drvdata(&wdev->dev);
 	unsigned long result;

-	switch (value) {
-	case 128:
-		ideapad_input_report(priv, value);
+	switch (*event) {
+	case IDEAPAD_WMI_EVENT_ESC:
+		ideapad_input_report(priv, 128);
 		break;
-	case 208:
+	case IDEAPAD_WMI_EVENT_FN_KEYS:
 		if (!eval_hals(priv->adev->handle, &result)) {
 			bool state = test_bit(HALS_FNLOCK_STATE_BIT, &result);

 			exec_sals(priv->adev->handle, state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
 		}
+
+		if (data->type != ACPI_TYPE_INTEGER) {
+			dev_warn(&wdev->dev,
+				 "WMI event data is not an integer\n");
+			break;
+		}
+
+		dev_dbg(&wdev->dev, "WMI fn-key event: 0x%llx\n",
+			data->integer.value);
+
+		ideapad_input_report(priv,
+				     data->integer.value | IDEAPAD_WMI_KEY);
+
 		break;
-	default:
-		dev_info(&priv->platform_device->dev,
-			 "Unknown WMI event: %u\n", value);
 	}
 }
 #endif
@@ -1671,25 +1724,24 @@ static int ideapad_acpi_add(struct platform_device *pdev)
 	}

 #if IS_ENABLED(CONFIG_ACPI_WMI)
-	for (i = 0; i < ARRAY_SIZE(ideapad_wmi_fnesc_events); i++) {
-		status = wmi_install_notify_handler(ideapad_wmi_fnesc_events[i],
-						    ideapad_wmi_notify, priv);
-		if (ACPI_SUCCESS(status)) {
-			priv->fnesc_guid = ideapad_wmi_fnesc_events[i];
-			break;
-		}
-	}
+	priv->wmi_drv = (struct wmi_driver) {
+		.driver = {
+			.name = "ideapad-wmi-fn-keys",
+		},
+		.id_table = ideapad_wmi_id_table,
+		.probe = ideapad_wmi_probe,
+		.notify = ideapad_wmi_notify,
+	};

-	if (ACPI_FAILURE(status) && status != AE_NOT_EXIST) {
-		err = -EIO;
-		goto notification_failed_wmi;
-	}
+	err = wmi_driver_register(&priv->wmi_drv);
+	if (err)
+		goto register_failed_wmi;
 #endif

 	return 0;

 #if IS_ENABLED(CONFIG_ACPI_WMI)
-notification_failed_wmi:
+register_failed_wmi:
 	acpi_remove_notify_handler(priv->adev->handle,
 				   ACPI_DEVICE_NOTIFY,
 				   ideapad_acpi_notify);
@@ -1720,8 +1772,7 @@ static int ideapad_acpi_remove(struct platform_device *pdev)
 	int i;

 #if IS_ENABLED(CONFIG_ACPI_WMI)
-	if (priv->fnesc_guid)
-		wmi_remove_notify_handler(priv->fnesc_guid);
+	wmi_driver_unregister(&priv->wmi_drv);
 #endif

 	acpi_remove_notify_handler(priv->adev->handle,
--
2.38.1

Re: [PATCH v3] platform/x86: ideapad-laptop: support for more special keys in WMI

[Hans de Goede]

Hi Philiph,

On 11/14/22 15:41, Philipp Jungkamp wrote:
> The event data of the WMI event 0xD0, which is assumed to be the
> fn_lock, is used to indicate several special keys on newer Yoga 7/9
> laptops.
> 
> The notify_id 0xD0 is non-unique in the DSDT of the Yoga 9 14IAP7, this
> causes wmi_get_event_data() to report wrong values.
> Port the ideapad-laptop WMI code to the wmi bus infrastructure which
> does not suffer from the shortcomings of wmi_get_event_data().
> 
> Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
> ---
> Hello,
> 
> is this about right? It works for me.
> 
> What I don't really like here is the dev_set_drvdata() which takes a non-const
> void * and I pass it a const pointer. I do cast the value of dev_get_drvdata()
> back to a const pointer, but this seems rather ugly.
> I preferred it over allocating a single int for the device or casting an enum
> to a void *. This additionally removes the need for a remove funtion.

Thank you for both the v2 and this new interesting approach. Unfortunately
I have a bit of a patch backlog so I'm not sure when I will get around to
this. I will try to get this reviewed / commented on no later then sometime
next week.

Regards,

Hans







> 
> Regards,
> Philipp
> 
>  drivers/platform/x86/ideapad-laptop.c | 109 +++++++++++++++++++-------
>  1 file changed, 80 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index 33b3dfdd1b08..6d35a9e961cf 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -30,6 +30,7 @@
>  #include <linux/seq_file.h>
>  #include <linux/sysfs.h>
>  #include <linux/types.h>
> +#include <linux/wmi.h>
> 
>  #include <acpi/video.h>
> 
> @@ -38,10 +39,19 @@
>  #define IDEAPAD_RFKILL_DEV_NUM	3
> 
>  #if IS_ENABLED(CONFIG_ACPI_WMI)
> -static const char *const ideapad_wmi_fnesc_events[] = {
> -	"26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", /* Yoga 3 */
> -	"56322276-8493-4CE8-A783-98C991274F5E", /* Yoga 700 */
> -	"8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", /* Legion 5 */
> +enum ideapad_wmi_event_type {
> +	IDEAPAD_WMI_EVENT_ESC,
> +	IDEAPAD_WMI_EVENT_FN_KEYS,
> +};
> +
> +enum ideapad_wmi_event_type ideapad_wmi_esc = IDEAPAD_WMI_EVENT_ESC,
> +enum ideapad_wmi_event_type ideapad_wmi_fn_keys = IDEAPAD_WMI_EVENT_FN_KEYS;
> +
> +static const struct wmi_device_id ideapad_wmi_id_table[] = {
> +	{ "26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", &ideapad_wmi_esc }, /* Yoga 3 */
> +	{ "56322276-8493-4CE8-A783-98C991274F5E", &ideapad_wmi_esc }, /* Yoga 700 */
> +	{ "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", &ideapad_wmi_fn_keys }, /* Legion 5 */
> +	{}
>  };
>  #endif
> 
> @@ -130,7 +140,7 @@ struct ideapad_private {
>  	struct ideapad_dytc_priv *dytc;
>  	struct dentry *debug;
>  	unsigned long cfg;
> -	const char *fnesc_guid;
> +	struct wmi_driver wmi_drv;
>  	struct {
>  		bool conservation_mode    : 1;
>  		bool dytc                 : 1;
> @@ -1074,6 +1084,7 @@ static void ideapad_sysfs_exit(struct ideapad_private *priv)
>  /*
>   * input device
>   */
> +#define IDEAPAD_WMI_KEY 0x100
>  static const struct key_entry ideapad_keymap[] = {
>  	{ KE_KEY,   6, { KEY_SWITCHVIDEOMODE } },
>  	{ KE_KEY,   7, { KEY_CAMERA } },
> @@ -1087,6 +1098,26 @@ static const struct key_entry ideapad_keymap[] = {
>  	{ KE_KEY,  66, { KEY_TOUCHPAD_OFF } },
>  	{ KE_KEY,  67, { KEY_TOUCHPAD_ON } },
>  	{ KE_KEY, 128, { KEY_ESC } },
> +
> +	/*
> +	 * WMI keys
> +	 */
> +
> +	/* FnLock (handled by the firmware) */
> +	{ KE_IGNORE,	0x02 | IDEAPAD_WMI_KEY },
> +	/* Customizable Lenovo Hotkey ("star" with 'S' inside) */
> +	{ KE_KEY,	0x01 | IDEAPAD_WMI_KEY, { KEY_FAVORITES } },
> +	/* Dark mode toggle */
> +	{ KE_KEY,	0x13 | IDEAPAD_WMI_KEY, { KEY_PROG1 } },
> +	/* Sound profile switch */
> +	{ KE_KEY,	0x12 | IDEAPAD_WMI_KEY, { KEY_PROG2 } },
> +	/* Lenovo Virtual Background application */
> +	{ KE_KEY,	0x28 | IDEAPAD_WMI_KEY, { KEY_PROG3 } },
> +	/* Lenovo Support */
> +	{ KE_KEY,	0x27 | IDEAPAD_WMI_KEY, { KEY_HELP } },
> +	/* Refresh Rate Toggle */
> +	{ KE_KEY,	0x0a | IDEAPAD_WMI_KEY, { KEY_DISPLAYTOGGLE } },
> +
>  	{ KE_END },
>  };
> 
> @@ -1491,25 +1522,47 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
>  }
> 
>  #if IS_ENABLED(CONFIG_ACPI_WMI)
> -static void ideapad_wmi_notify(u32 value, void *context)
> +static int ideapad_wmi_probe(struct wmi_device *wdev, const void *context)
>  {
> -	struct ideapad_private *priv = context;
> +	dev_set_drvdata(&wdev->dev, (void *) context);
> +	return 0;
> +}
> +
> +static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
> +{
> +	struct wmi_driver *wdrv = container_of(wdev->dev.driver,
> +					       struct wmi_driver,
> +					       driver);
> +	struct ideapad_private *priv = container_of(wdrv,
> +						    struct ideapad_private,
> +						    wmi_drv);
> +	const enum ideapad_wmi_event_type *event = dev_get_drvdata(&wdev->dev);
>  	unsigned long result;
> 
> -	switch (value) {
> -	case 128:
> -		ideapad_input_report(priv, value);
> +	switch (*event) {
> +	case IDEAPAD_WMI_EVENT_ESC:
> +		ideapad_input_report(priv, 128);
>  		break;
> -	case 208:
> +	case IDEAPAD_WMI_EVENT_FN_KEYS:
>  		if (!eval_hals(priv->adev->handle, &result)) {
>  			bool state = test_bit(HALS_FNLOCK_STATE_BIT, &result);
> 
>  			exec_sals(priv->adev->handle, state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
>  		}
> +
> +		if (data->type != ACPI_TYPE_INTEGER) {
> +			dev_warn(&wdev->dev,
> +				 "WMI event data is not an integer\n");
> +			break;
> +		}
> +
> +		dev_dbg(&wdev->dev, "WMI fn-key event: 0x%llx\n",
> +			data->integer.value);
> +
> +		ideapad_input_report(priv,
> +				     data->integer.value | IDEAPAD_WMI_KEY);
> +
>  		break;
> -	default:
> -		dev_info(&priv->platform_device->dev,
> -			 "Unknown WMI event: %u\n", value);
>  	}
>  }
>  #endif
> @@ -1671,25 +1724,24 @@ static int ideapad_acpi_add(struct platform_device *pdev)
>  	}
> 
>  #if IS_ENABLED(CONFIG_ACPI_WMI)
> -	for (i = 0; i < ARRAY_SIZE(ideapad_wmi_fnesc_events); i++) {
> -		status = wmi_install_notify_handler(ideapad_wmi_fnesc_events[i],
> -						    ideapad_wmi_notify, priv);
> -		if (ACPI_SUCCESS(status)) {
> -			priv->fnesc_guid = ideapad_wmi_fnesc_events[i];
> -			break;
> -		}
> -	}
> +	priv->wmi_drv = (struct wmi_driver) {
> +		.driver = {
> +			.name = "ideapad-wmi-fn-keys",
> +		},
> +		.id_table = ideapad_wmi_id_table,
> +		.probe = ideapad_wmi_probe,
> +		.notify = ideapad_wmi_notify,
> +	};
> 
> -	if (ACPI_FAILURE(status) && status != AE_NOT_EXIST) {
> -		err = -EIO;
> -		goto notification_failed_wmi;
> -	}
> +	err = wmi_driver_register(&priv->wmi_drv);
> +	if (err)
> +		goto register_failed_wmi;
>  #endif
> 
>  	return 0;
> 
>  #if IS_ENABLED(CONFIG_ACPI_WMI)
> -notification_failed_wmi:
> +register_failed_wmi:
>  	acpi_remove_notify_handler(priv->adev->handle,
>  				   ACPI_DEVICE_NOTIFY,
>  				   ideapad_acpi_notify);
> @@ -1720,8 +1772,7 @@ static int ideapad_acpi_remove(struct platform_device *pdev)
>  	int i;
> 
>  #if IS_ENABLED(CONFIG_ACPI_WMI)
> -	if (priv->fnesc_guid)
> -		wmi_remove_notify_handler(priv->fnesc_guid);
> +	wmi_driver_unregister(&priv->wmi_drv);
>  #endif
> 
>  	acpi_remove_notify_handler(priv->adev->handle,
> --
> 2.38.1
> 

Re: [PATCH v3] platform/x86: ideapad-laptop: support for more special keys in WMI

[Hans de Goede]

Hi,

On 11/14/22 15:41, Philipp Jungkamp wrote:
> The event data of the WMI event 0xD0, which is assumed to be the
> fn_lock, is used to indicate several special keys on newer Yoga 7/9
> laptops.
> 
> The notify_id 0xD0 is non-unique in the DSDT of the Yoga 9 14IAP7, this
> causes wmi_get_event_data() to report wrong values.
> Port the ideapad-laptop WMI code to the wmi bus infrastructure which
> does not suffer from the shortcomings of wmi_get_event_data().
> 
> Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
> ---
> Hello,
> 
> is this about right? It works for me.
> 
> What I don't really like here is the dev_set_drvdata() which takes a non-const
> void * and I pass it a const pointer. I do cast the value of dev_get_drvdata()
> back to a const pointer, but this seems rather ugly.
> I preferred it over allocating a single int for the device or casting an enum
> to a void *. This additionally removes the need for a remove funtion.

I decided just take quick peek and I think the cleanest solution here would
be to add a driver-data struct with the lookup result of:

	struct ideapad_private *priv = container_of(wdrv,
						    struct ideapad_private,
						    wmi_drv);

Stored in there + the enum value and then alloc it with devm_kzalloc
to avoid the need for a remove callback.

I'm also wondering if we could then maybe not move some other variables
only used in the wmi_notify callback to that driver-data struct ?
(I did not check if there are any candidates).

Also this is going to need a big fat warning (comment) that the cute trick
with registering a wmi_driver struct embedded inside the platform_driver
data struct very much relies on there being only one platform_device
instance to which the platform_driver will bind ever, otherwise
we will get multiple wmi_driver's registered for the same WMI GUIDs
and then the container-off will likely return the driver-data of the
first platform device ...

Which makes me wonder if it would not be cleaner to just use a global
pointer for this ?   Note this is an honest open question.

Actually since the platform_device gets instantiated by the ACPI
core there is no guarantee there will be only 1. So I think that
the container_of on the wmi-driver trick needs to go, instead
introduce:

1. A global ideapad_private_data_mutex mutex
2. A global ideapad_private_data pointer

And:

1. In ideapad_acpi_add:
lock the mutex
check that ideapad_private_data is not already set and if it is bail with an error
set ideapad_private_data
unlock the mutex

2. in ideapad_acpi_remove:
lock the mutex
clear the golbal pointer
unlock the mutex

3. In the wmi-driver's notify method:
lock the mutex
check ideapad_private_data is not NULL
process event
unlock the mutex

4. replace module_platform_driver with normal module init + exit
functions which register both drivers / unregister both drivers.

I believe that this will be a more clean approach then the embedded
wmi_driver struct cuteness.

Regards,

Hans




> 
> Regards,
> Philipp
> 
>  drivers/platform/x86/ideapad-laptop.c | 109 +++++++++++++++++++-------
>  1 file changed, 80 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index 33b3dfdd1b08..6d35a9e961cf 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -30,6 +30,7 @@
>  #include <linux/seq_file.h>
>  #include <linux/sysfs.h>
>  #include <linux/types.h>
> +#include <linux/wmi.h>
> 
>  #include <acpi/video.h>
> 
> @@ -38,10 +39,19 @@
>  #define IDEAPAD_RFKILL_DEV_NUM	3
> 
>  #if IS_ENABLED(CONFIG_ACPI_WMI)
> -static const char *const ideapad_wmi_fnesc_events[] = {
> -	"26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", /* Yoga 3 */
> -	"56322276-8493-4CE8-A783-98C991274F5E", /* Yoga 700 */
> -	"8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", /* Legion 5 */
> +enum ideapad_wmi_event_type {
> +	IDEAPAD_WMI_EVENT_ESC,
> +	IDEAPAD_WMI_EVENT_FN_KEYS,
> +};
> +
> +enum ideapad_wmi_event_type ideapad_wmi_esc = IDEAPAD_WMI_EVENT_ESC,
> +enum ideapad_wmi_event_type ideapad_wmi_fn_keys = IDEAPAD_WMI_EVENT_FN_KEYS;
> +
> +static const struct wmi_device_id ideapad_wmi_id_table[] = {
> +	{ "26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", &ideapad_wmi_esc }, /* Yoga 3 */
> +	{ "56322276-8493-4CE8-A783-98C991274F5E", &ideapad_wmi_esc }, /* Yoga 700 */
> +	{ "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", &ideapad_wmi_fn_keys }, /* Legion 5 */
> +	{}
>  };
>  #endif
> 
> @@ -130,7 +140,7 @@ struct ideapad_private {
>  	struct ideapad_dytc_priv *dytc;
>  	struct dentry *debug;
>  	unsigned long cfg;
> -	const char *fnesc_guid;
> +	struct wmi_driver wmi_drv;
>  	struct {
>  		bool conservation_mode    : 1;
>  		bool dytc                 : 1;
> @@ -1074,6 +1084,7 @@ static void ideapad_sysfs_exit(struct ideapad_private *priv)
>  /*
>   * input device
>   */
> +#define IDEAPAD_WMI_KEY 0x100
>  static const struct key_entry ideapad_keymap[] = {
>  	{ KE_KEY,   6, { KEY_SWITCHVIDEOMODE } },
>  	{ KE_KEY,   7, { KEY_CAMERA } },
> @@ -1087,6 +1098,26 @@ static const struct key_entry ideapad_keymap[] = {
>  	{ KE_KEY,  66, { KEY_TOUCHPAD_OFF } },
>  	{ KE_KEY,  67, { KEY_TOUCHPAD_ON } },
>  	{ KE_KEY, 128, { KEY_ESC } },
> +
> +	/*
> +	 * WMI keys
> +	 */
> +
> +	/* FnLock (handled by the firmware) */
> +	{ KE_IGNORE,	0x02 | IDEAPAD_WMI_KEY },
> +	/* Customizable Lenovo Hotkey ("star" with 'S' inside) */
> +	{ KE_KEY,	0x01 | IDEAPAD_WMI_KEY, { KEY_FAVORITES } },
> +	/* Dark mode toggle */
> +	{ KE_KEY,	0x13 | IDEAPAD_WMI_KEY, { KEY_PROG1 } },
> +	/* Sound profile switch */
> +	{ KE_KEY,	0x12 | IDEAPAD_WMI_KEY, { KEY_PROG2 } },
> +	/* Lenovo Virtual Background application */
> +	{ KE_KEY,	0x28 | IDEAPAD_WMI_KEY, { KEY_PROG3 } },
> +	/* Lenovo Support */
> +	{ KE_KEY,	0x27 | IDEAPAD_WMI_KEY, { KEY_HELP } },
> +	/* Refresh Rate Toggle */
> +	{ KE_KEY,	0x0a | IDEAPAD_WMI_KEY, { KEY_DISPLAYTOGGLE } },
> +
>  	{ KE_END },
>  };
> 
> @@ -1491,25 +1522,47 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
>  }
> 
>  #if IS_ENABLED(CONFIG_ACPI_WMI)
> -static void ideapad_wmi_notify(u32 value, void *context)
> +static int ideapad_wmi_probe(struct wmi_device *wdev, const void *context)
>  {
> -	struct ideapad_private *priv = context;
> +	dev_set_drvdata(&wdev->dev, (void *) context);
> +	return 0;
> +}
> +
> +static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
> +{
> +	struct wmi_driver *wdrv = container_of(wdev->dev.driver,
> +					       struct wmi_driver,
> +					       driver);
> +	struct ideapad_private *priv = container_of(wdrv,
> +						    struct ideapad_private,
> +						    wmi_drv);
> +	const enum ideapad_wmi_event_type *event = dev_get_drvdata(&wdev->dev);
>  	unsigned long result;
> 
> -	switch (value) {
> -	case 128:
> -		ideapad_input_report(priv, value);
> +	switch (*event) {
> +	case IDEAPAD_WMI_EVENT_ESC:
> +		ideapad_input_report(priv, 128);
>  		break;
> -	case 208:
> +	case IDEAPAD_WMI_EVENT_FN_KEYS:
>  		if (!eval_hals(priv->adev->handle, &result)) {
>  			bool state = test_bit(HALS_FNLOCK_STATE_BIT, &result);
> 
>  			exec_sals(priv->adev->handle, state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
>  		}
> +
> +		if (data->type != ACPI_TYPE_INTEGER) {
> +			dev_warn(&wdev->dev,
> +				 "WMI event data is not an integer\n");
> +			break;
> +		}
> +
> +		dev_dbg(&wdev->dev, "WMI fn-key event: 0x%llx\n",
> +			data->integer.value);
> +
> +		ideapad_input_report(priv,
> +				     data->integer.value | IDEAPAD_WMI_KEY);
> +
>  		break;
> -	default:
> -		dev_info(&priv->platform_device->dev,
> -			 "Unknown WMI event: %u\n", value);
>  	}
>  }
>  #endif
> @@ -1671,25 +1724,24 @@ static int ideapad_acpi_add(struct platform_device *pdev)
>  	}
> 
>  #if IS_ENABLED(CONFIG_ACPI_WMI)
> -	for (i = 0; i < ARRAY_SIZE(ideapad_wmi_fnesc_events); i++) {
> -		status = wmi_install_notify_handler(ideapad_wmi_fnesc_events[i],
> -						    ideapad_wmi_notify, priv);
> -		if (ACPI_SUCCESS(status)) {
> -			priv->fnesc_guid = ideapad_wmi_fnesc_events[i];
> -			break;
> -		}
> -	}
> +	priv->wmi_drv = (struct wmi_driver) {
> +		.driver = {
> +			.name = "ideapad-wmi-fn-keys",
> +		},
> +		.id_table = ideapad_wmi_id_table,
> +		.probe = ideapad_wmi_probe,
> +		.notify = ideapad_wmi_notify,
> +	};
> 
> -	if (ACPI_FAILURE(status) && status != AE_NOT_EXIST) {
> -		err = -EIO;
> -		goto notification_failed_wmi;
> -	}
> +	err = wmi_driver_register(&priv->wmi_drv);
> +	if (err)
> +		goto register_failed_wmi;
>  #endif
> 
>  	return 0;
> 
>  #if IS_ENABLED(CONFIG_ACPI_WMI)
> -notification_failed_wmi:
> +register_failed_wmi:
>  	acpi_remove_notify_handler(priv->adev->handle,
>  				   ACPI_DEVICE_NOTIFY,
>  				   ideapad_acpi_notify);
> @@ -1720,8 +1772,7 @@ static int ideapad_acpi_remove(struct platform_device *pdev)
>  	int i;
> 
>  #if IS_ENABLED(CONFIG_ACPI_WMI)
> -	if (priv->fnesc_guid)
> -		wmi_remove_notify_handler(priv->fnesc_guid);
> +	wmi_driver_unregister(&priv->wmi_drv);
>  #endif
> 
>  	acpi_remove_notify_handler(priv->adev->handle,
> --
> 2.38.1
> 

[PATCH v4] platform/x86: ideapad-laptop: support for more special keys in WMI

[Philipp Jungkamp]

The event data of the WMI event 0xD0, which is assumed to be the
fn_lock, is used to indicate several special keys on newer Yoga 7/9
laptops.

The notify_id 0xD0 is non-unique in the DSDT of the Yoga 9 14IAP7, this
causes wmi_get_event_data() to report wrong values.
Port the ideapad-laptop WMI code to the wmi bus infrastructure which
does not suffer from the shortcomings of wmi_get_event_data().

Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
---
This now uses the WMI bus infrastructure with both the platform driver and the
wmi driver registered on module load. Is the synchronization used here
sufficient?

Thanks for all the help!

Regards,
Philipp Jungkamp

 drivers/platform/x86/ideapad-laptop.c | 240 ++++++++++++++++++++------
 1 file changed, 188 insertions(+), 52 deletions(-)

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 33b3dfdd1b08..803bf5cf50f9 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -30,6 +30,7 @@
 #include <linux/seq_file.h>
 #include <linux/sysfs.h>
 #include <linux/types.h>
+#include <linux/wmi.h>

 #include <acpi/video.h>

@@ -38,10 +39,13 @@
 #define IDEAPAD_RFKILL_DEV_NUM	3

 #if IS_ENABLED(CONFIG_ACPI_WMI)
-static const char *const ideapad_wmi_fnesc_events[] = {
-	"26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", /* Yoga 3 */
-	"56322276-8493-4CE8-A783-98C991274F5E", /* Yoga 700 */
-	"8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", /* Legion 5 */
+enum ideapad_wmi_event_type {
+	IDEAPAD_WMI_EVENT_ESC,
+	IDEAPAD_WMI_EVENT_FN_KEYS,
+};
+
+struct ideapad_wmi_private {
+	enum ideapad_wmi_event_type event;
 };
 #endif

@@ -130,7 +134,6 @@ struct ideapad_private {
 	struct ideapad_dytc_priv *dytc;
 	struct dentry *debug;
 	unsigned long cfg;
-	const char *fnesc_guid;
 	struct {
 		bool conservation_mode    : 1;
 		bool dytc                 : 1;
@@ -156,6 +159,42 @@ static bool allow_v4_dytc;
 module_param(allow_v4_dytc, bool, 0444);
 MODULE_PARM_DESC(allow_v4_dytc, "Enable DYTC version 4 platform-profile support.");

+/*
+ * shared data
+ */
+
+static struct ideapad_private *ideapad_shared;
+static DEFINE_MUTEX(ideapad_shared_mutex);
+
+static int ideapad_shared_init(struct ideapad_private *priv)
+{
+	int ret;
+
+	mutex_lock(&ideapad_shared_mutex);
+
+	if (!ideapad_shared) {
+		ideapad_shared = priv;
+		ret = 0;
+	} else {
+		dev_warn(&priv->adev->dev, "found multiple platform devices\n");
+		ret = -EINVAL;
+	}
+
+	mutex_unlock(&ideapad_shared_mutex);
+
+	return ret;
+}
+
+static void ideapad_shared_exit(struct ideapad_private *priv)
+{
+	mutex_lock(&ideapad_shared_mutex);
+
+	if (ideapad_shared == priv)
+		ideapad_shared = NULL;
+
+	mutex_unlock(&ideapad_shared_mutex);
+}
+
 /*
  * ACPI Helpers
  */
@@ -1074,6 +1113,7 @@ static void ideapad_sysfs_exit(struct ideapad_private *priv)
 /*
  * input device
  */
+#define IDEAPAD_WMI_KEY 0x100
 static const struct key_entry ideapad_keymap[] = {
 	{ KE_KEY,   6, { KEY_SWITCHVIDEOMODE } },
 	{ KE_KEY,   7, { KEY_CAMERA } },
@@ -1087,6 +1127,28 @@ static const struct key_entry ideapad_keymap[] = {
 	{ KE_KEY,  66, { KEY_TOUCHPAD_OFF } },
 	{ KE_KEY,  67, { KEY_TOUCHPAD_ON } },
 	{ KE_KEY, 128, { KEY_ESC } },
+
+	/*
+	 * WMI keys
+	 */
+
+	/* FnLock (handled by the firmware) */
+	{ KE_IGNORE,	0x02 | IDEAPAD_WMI_KEY },
+	/* Esc (handled by the firmware) */
+	{ KE_IGNORE,	0x03 | IDEAPAD_WMI_KEY },
+	/* Customizable Lenovo Hotkey ("star" with 'S' inside) */
+	{ KE_KEY,	0x01 | IDEAPAD_WMI_KEY, { KEY_FAVORITES } },
+	/* Dark mode toggle */
+	{ KE_KEY,	0x13 | IDEAPAD_WMI_KEY, { KEY_PROG1 } },
+	/* Sound profile switch */
+	{ KE_KEY,	0x12 | IDEAPAD_WMI_KEY, { KEY_PROG2 } },
+	/* Lenovo Virtual Background application */
+	{ KE_KEY,	0x28 | IDEAPAD_WMI_KEY, { KEY_PROG3 } },
+	/* Lenovo Support */
+	{ KE_KEY,	0x27 | IDEAPAD_WMI_KEY, { KEY_HELP } },
+	/* Refresh Rate Toggle */
+	{ KE_KEY,	0x0a | IDEAPAD_WMI_KEY, { KEY_DISPLAYTOGGLE } },
+
 	{ KE_END },
 };

@@ -1490,30 +1552,6 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
 	}
 }

-#if IS_ENABLED(CONFIG_ACPI_WMI)
-static void ideapad_wmi_notify(u32 value, void *context)
-{
-	struct ideapad_private *priv = context;
-	unsigned long result;
-
-	switch (value) {
-	case 128:
-		ideapad_input_report(priv, value);
-		break;
-	case 208:
-		if (!eval_hals(priv->adev->handle, &result)) {
-			bool state = test_bit(HALS_FNLOCK_STATE_BIT, &result);
-
-			exec_sals(priv->adev->handle, state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
-		}
-		break;
-	default:
-		dev_info(&priv->platform_device->dev,
-			 "Unknown WMI event: %u\n", value);
-	}
-}
-#endif
-
 /*
  * Some ideapads have a hardware rfkill switch, but most do not have one.
  * Reading VPCCMD_R_RF always results in 0 on models without a hardware rfkill,
@@ -1589,6 +1627,95 @@ static void ideapad_check_features(struct ideapad_private *priv)
 	}
 }

+#if IS_ENABLED(CONFIG_ACPI_WMI)
+/*
+ * WMI driver
+ */
+static int ideapad_wmi_probe(struct wmi_device *wdev, const void *context)
+{
+	struct ideapad_wmi_private *wpriv;
+
+	wpriv = devm_kzalloc(&wdev->dev, sizeof(*wpriv), GFP_KERNEL);
+	if (!wpriv)
+		return -ENOMEM;
+
+	*wpriv = *(struct ideapad_wmi_private *)context;
+
+	dev_set_drvdata(&wdev->dev, wpriv);
+	return 0;
+}
+
+static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
+{
+	struct ideapad_wmi_private *wpriv = dev_get_drvdata(&wdev->dev);
+	struct ideapad_private *priv;
+	unsigned long result;
+
+	mutex_lock(&ideapad_shared_mutex);
+
+	priv = ideapad_shared;
+	if (!priv)
+		goto unlock;
+
+	switch (wpriv->event) {
+	case IDEAPAD_WMI_EVENT_ESC:
+		ideapad_input_report(priv, 128);
+		break;
+	case IDEAPAD_WMI_EVENT_FN_KEYS:
+		if (!eval_hals(priv->adev->handle, &result)) {
+			bool state = test_bit(HALS_FNLOCK_STATE_BIT, &result);
+
+			exec_sals(priv->adev->handle, state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
+		}
+
+		if (data->type != ACPI_TYPE_INTEGER) {
+			dev_warn(&wdev->dev,
+				 "WMI event data is not an integer\n");
+			break;
+		}
+
+		dev_dbg(&wdev->dev, "WMI fn-key event: 0x%llx\n",
+			data->integer.value);
+
+		ideapad_input_report(priv,
+				     data->integer.value | IDEAPAD_WMI_KEY);
+
+		break;
+	}
+unlock:
+	mutex_unlock(&ideapad_shared_mutex);
+}
+
+struct ideapad_wmi_private ideapad_wmi_context_esc = {
+	.event = IDEAPAD_WMI_EVENT_ESC
+};
+
+struct ideapad_wmi_private ideapad_wmi_context_fn_keys = {
+	.event = IDEAPAD_WMI_EVENT_FN_KEYS
+};
+
+static const struct wmi_device_id ideapad_wmi_ids[] = {
+	{ "26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", &ideapad_wmi_context_esc }, /* Yoga 3 */
+	{ "56322276-8493-4CE8-A783-98C991274F5E", &ideapad_wmi_context_esc }, /* Yoga 700 */
+	{ "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", &ideapad_wmi_context_fn_keys }, /* Legion 5 */
+	{},
+};
+MODULE_DEVICE_TABLE(wmi, ideapad_wmi_ids);
+
+static struct wmi_driver ideapad_wmi_driver = {
+	.driver = {
+		.name = "ideapad_wmi",
+	},
+	.id_table = ideapad_wmi_ids,
+	.probe = ideapad_wmi_probe,
+	.notify = ideapad_wmi_notify,
+};
+
+#endif
+
+/*
+ * ACPI driver
+ */
 static int ideapad_acpi_add(struct platform_device *pdev)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
@@ -1670,30 +1797,16 @@ static int ideapad_acpi_add(struct platform_device *pdev)
 		goto notification_failed;
 	}

-#if IS_ENABLED(CONFIG_ACPI_WMI)
-	for (i = 0; i < ARRAY_SIZE(ideapad_wmi_fnesc_events); i++) {
-		status = wmi_install_notify_handler(ideapad_wmi_fnesc_events[i],
-						    ideapad_wmi_notify, priv);
-		if (ACPI_SUCCESS(status)) {
-			priv->fnesc_guid = ideapad_wmi_fnesc_events[i];
-			break;
-		}
-	}
-
-	if (ACPI_FAILURE(status) && status != AE_NOT_EXIST) {
-		err = -EIO;
-		goto notification_failed_wmi;
-	}
-#endif
+	err = ideapad_shared_init(priv);
+	if (err)
+		goto shared_init_failed;

 	return 0;

-#if IS_ENABLED(CONFIG_ACPI_WMI)
-notification_failed_wmi:
+shared_init_failed:
 	acpi_remove_notify_handler(priv->adev->handle,
 				   ACPI_DEVICE_NOTIFY,
 				   ideapad_acpi_notify);
-#endif

 notification_failed:
 	ideapad_backlight_exit(priv);
@@ -1719,10 +1832,7 @@ static int ideapad_acpi_remove(struct platform_device *pdev)
 	struct ideapad_private *priv = dev_get_drvdata(&pdev->dev);
 	int i;

-#if IS_ENABLED(CONFIG_ACPI_WMI)
-	if (priv->fnesc_guid)
-		wmi_remove_notify_handler(priv->fnesc_guid);
-#endif
+	ideapad_shared_exit(priv);

 	acpi_remove_notify_handler(priv->adev->handle,
 				   ACPI_DEVICE_NOTIFY,
@@ -1774,7 +1884,33 @@ static struct platform_driver ideapad_acpi_driver = {
 	},
 };

-module_platform_driver(ideapad_acpi_driver);
+static int __init ideapad_laptop_init(void)
+{
+	int err;
+
+#if IS_ENABLED(CONFIG_ACPI_WMI)
+	err = wmi_driver_register(&ideapad_wmi_driver);
+	if (err)
+		return err;
+#endif
+
+	err = platform_driver_register(&ideapad_acpi_driver);
+	if (err)
+		return err;
+
+	return 0;
+}
+module_init(ideapad_laptop_init)
+
+static void __exit ideapad_laptop_exit(void)
+{
+#if IS_ENABLED(CONFIG_ACPI_WMI)
+	wmi_driver_unregister(&ideapad_wmi_driver);
+#endif
+
+	platform_driver_unregister(&ideapad_acpi_driver);
+}
+module_exit(ideapad_laptop_exit)

 MODULE_AUTHOR("David Woodhouse <dwmw2@infradead.org>");
 MODULE_DESCRIPTION("IdeaPad ACPI Extras");
--
2.38.1

Re: [PATCH v3] platform/x86: ideapad-laptop: support for more special keys in WMI

[Armin Wolf]

Am 14.11.22 um 17:41 schrieb Hans de Goede:

> Hi,
>
> On 11/14/22 15:41, Philipp Jungkamp wrote:
>> The event data of the WMI event 0xD0, which is assumed to be the
>> fn_lock, is used to indicate several special keys on newer Yoga 7/9
>> laptops.
>>
>> The notify_id 0xD0 is non-unique in the DSDT of the Yoga 9 14IAP7, this
>> causes wmi_get_event_data() to report wrong values.
>> Port the ideapad-laptop WMI code to the wmi bus infrastructure which
>> does not suffer from the shortcomings of wmi_get_event_data().
>>
>> Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
>> ---
>> Hello,
>>
>> is this about right? It works for me.
>>
>> What I don't really like here is the dev_set_drvdata() which takes a non-const
>> void * and I pass it a const pointer. I do cast the value of dev_get_drvdata()
>> back to a const pointer, but this seems rather ugly.
>> I preferred it over allocating a single int for the device or casting an enum
>> to a void *. This additionally removes the need for a remove funtion.
> I decided just take quick peek and I think the cleanest solution here would
> be to add a driver-data struct with the lookup result of:
>
> 	struct ideapad_private *priv = container_of(wdrv,
> 						    struct ideapad_private,
> 						    wmi_drv);
>
> Stored in there + the enum value and then alloc it with devm_kzalloc
> to avoid the need for a remove callback.
>
> I'm also wondering if we could then maybe not move some other variables
> only used in the wmi_notify callback to that driver-data struct ?
> (I did not check if there are any candidates).
>
> Also this is going to need a big fat warning (comment) that the cute trick
> with registering a wmi_driver struct embedded inside the platform_driver
> data struct very much relies on there being only one platform_device
> instance to which the platform_driver will bind ever, otherwise
> we will get multiple wmi_driver's registered for the same WMI GUIDs
> and then the container-off will likely return the driver-data of the
> first platform device ...

Hello,

i think this is not going to be the case. For each platform device instance,
a separate ideapad_private struct is allocated and initialized. This means
that for each platform device instance, a separate WMI driver is registered,
so each WMI driver will access the private data of its platform device when
using container_of(), not just the private data of the first one.

AFAIK, WMI drivers cannot bind to an already bound WMI device, otherwise the
WMI bus would need to create multiple devices for a single GUID instance, which
is, as far as i know, not the case.

Armin Wolf

> Which makes me wonder if it would not be cleaner to just use a global
> pointer for this ?   Note this is an honest open question.
>
> Actually since the platform_device gets instantiated by the ACPI
> core there is no guarantee there will be only 1. So I think that
> the container_of on the wmi-driver trick needs to go, instead
> introduce:
>
> 1. A global ideapad_private_data_mutex mutex
> 2. A global ideapad_private_data pointer
>
> And:
>
> 1. In ideapad_acpi_add:
> lock the mutex
> check that ideapad_private_data is not already set and if it is bail with an error
> set ideapad_private_data
> unlock the mutex
>
> 2. in ideapad_acpi_remove:
> lock the mutex
> clear the golbal pointer
> unlock the mutex
>
> 3. In the wmi-driver's notify method:
> lock the mutex
> check ideapad_private_data is not NULL
> process event
> unlock the mutex
>
> 4. replace module_platform_driver with normal module init + exit
> functions which register both drivers / unregister both drivers.
>
> I believe that this will be a more clean approach then the embedded
> wmi_driver struct cuteness.
>
> Regards,
>
> Hans
>
>
>
>
>> Regards,
>> Philipp
>>
>>   drivers/platform/x86/ideapad-laptop.c | 109 +++++++++++++++++++-------
>>   1 file changed, 80 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
>> index 33b3dfdd1b08..6d35a9e961cf 100644
>> --- a/drivers/platform/x86/ideapad-laptop.c
>> +++ b/drivers/platform/x86/ideapad-laptop.c
>> @@ -30,6 +30,7 @@
>>   #include <linux/seq_file.h>
>>   #include <linux/sysfs.h>
>>   #include <linux/types.h>
>> +#include <linux/wmi.h>
>>
>>   #include <acpi/video.h>
>>
>> @@ -38,10 +39,19 @@
>>   #define IDEAPAD_RFKILL_DEV_NUM	3
>>
>>   #if IS_ENABLED(CONFIG_ACPI_WMI)
>> -static const char *const ideapad_wmi_fnesc_events[] = {
>> -	"26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", /* Yoga 3 */
>> -	"56322276-8493-4CE8-A783-98C991274F5E", /* Yoga 700 */
>> -	"8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", /* Legion 5 */
>> +enum ideapad_wmi_event_type {
>> +	IDEAPAD_WMI_EVENT_ESC,
>> +	IDEAPAD_WMI_EVENT_FN_KEYS,
>> +};
>> +
>> +enum ideapad_wmi_event_type ideapad_wmi_esc = IDEAPAD_WMI_EVENT_ESC,
>> +enum ideapad_wmi_event_type ideapad_wmi_fn_keys = IDEAPAD_WMI_EVENT_FN_KEYS;
>> +
>> +static const struct wmi_device_id ideapad_wmi_id_table[] = {
>> +	{ "26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", &ideapad_wmi_esc }, /* Yoga 3 */
>> +	{ "56322276-8493-4CE8-A783-98C991274F5E", &ideapad_wmi_esc }, /* Yoga 700 */
>> +	{ "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", &ideapad_wmi_fn_keys }, /* Legion 5 */
>> +	{}
>>   };
>>   #endif
>>
>> @@ -130,7 +140,7 @@ struct ideapad_private {
>>   	struct ideapad_dytc_priv *dytc;
>>   	struct dentry *debug;
>>   	unsigned long cfg;
>> -	const char *fnesc_guid;
>> +	struct wmi_driver wmi_drv;
>>   	struct {
>>   		bool conservation_mode    : 1;
>>   		bool dytc                 : 1;
>> @@ -1074,6 +1084,7 @@ static void ideapad_sysfs_exit(struct ideapad_private *priv)
>>   /*
>>    * input device
>>    */
>> +#define IDEAPAD_WMI_KEY 0x100
>>   static const struct key_entry ideapad_keymap[] = {
>>   	{ KE_KEY,   6, { KEY_SWITCHVIDEOMODE } },
>>   	{ KE_KEY,   7, { KEY_CAMERA } },
>> @@ -1087,6 +1098,26 @@ static const struct key_entry ideapad_keymap[] = {
>>   	{ KE_KEY,  66, { KEY_TOUCHPAD_OFF } },
>>   	{ KE_KEY,  67, { KEY_TOUCHPAD_ON } },
>>   	{ KE_KEY, 128, { KEY_ESC } },
>> +
>> +	/*
>> +	 * WMI keys
>> +	 */
>> +
>> +	/* FnLock (handled by the firmware) */
>> +	{ KE_IGNORE,	0x02 | IDEAPAD_WMI_KEY },
>> +	/* Customizable Lenovo Hotkey ("star" with 'S' inside) */
>> +	{ KE_KEY,	0x01 | IDEAPAD_WMI_KEY, { KEY_FAVORITES } },
>> +	/* Dark mode toggle */
>> +	{ KE_KEY,	0x13 | IDEAPAD_WMI_KEY, { KEY_PROG1 } },
>> +	/* Sound profile switch */
>> +	{ KE_KEY,	0x12 | IDEAPAD_WMI_KEY, { KEY_PROG2 } },
>> +	/* Lenovo Virtual Background application */
>> +	{ KE_KEY,	0x28 | IDEAPAD_WMI_KEY, { KEY_PROG3 } },
>> +	/* Lenovo Support */
>> +	{ KE_KEY,	0x27 | IDEAPAD_WMI_KEY, { KEY_HELP } },
>> +	/* Refresh Rate Toggle */
>> +	{ KE_KEY,	0x0a | IDEAPAD_WMI_KEY, { KEY_DISPLAYTOGGLE } },
>> +
>>   	{ KE_END },
>>   };
>>
>> @@ -1491,25 +1522,47 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
>>   }
>>
>>   #if IS_ENABLED(CONFIG_ACPI_WMI)
>> -static void ideapad_wmi_notify(u32 value, void *context)
>> +static int ideapad_wmi_probe(struct wmi_device *wdev, const void *context)
>>   {
>> -	struct ideapad_private *priv = context;
>> +	dev_set_drvdata(&wdev->dev, (void *) context);
>> +	return 0;
>> +}
>> +
>> +static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
>> +{
>> +	struct wmi_driver *wdrv = container_of(wdev->dev.driver,
>> +					       struct wmi_driver,
>> +					       driver);
>> +	struct ideapad_private *priv = container_of(wdrv,
>> +						    struct ideapad_private,
>> +						    wmi_drv);
>> +	const enum ideapad_wmi_event_type *event = dev_get_drvdata(&wdev->dev);
>>   	unsigned long result;
>>
>> -	switch (value) {
>> -	case 128:
>> -		ideapad_input_report(priv, value);
>> +	switch (*event) {
>> +	case IDEAPAD_WMI_EVENT_ESC:
>> +		ideapad_input_report(priv, 128);
>>   		break;
>> -	case 208:
>> +	case IDEAPAD_WMI_EVENT_FN_KEYS:
>>   		if (!eval_hals(priv->adev->handle, &result)) {
>>   			bool state = test_bit(HALS_FNLOCK_STATE_BIT, &result);
>>
>>   			exec_sals(priv->adev->handle, state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
>>   		}
>> +
>> +		if (data->type != ACPI_TYPE_INTEGER) {
>> +			dev_warn(&wdev->dev,
>> +				 "WMI event data is not an integer\n");
>> +			break;
>> +		}
>> +
>> +		dev_dbg(&wdev->dev, "WMI fn-key event: 0x%llx\n",
>> +			data->integer.value);
>> +
>> +		ideapad_input_report(priv,
>> +				     data->integer.value | IDEAPAD_WMI_KEY);
>> +
>>   		break;
>> -	default:
>> -		dev_info(&priv->platform_device->dev,
>> -			 "Unknown WMI event: %u\n", value);
>>   	}
>>   }
>>   #endif
>> @@ -1671,25 +1724,24 @@ static int ideapad_acpi_add(struct platform_device *pdev)
>>   	}
>>
>>   #if IS_ENABLED(CONFIG_ACPI_WMI)
>> -	for (i = 0; i < ARRAY_SIZE(ideapad_wmi_fnesc_events); i++) {
>> -		status = wmi_install_notify_handler(ideapad_wmi_fnesc_events[i],
>> -						    ideapad_wmi_notify, priv);
>> -		if (ACPI_SUCCESS(status)) {
>> -			priv->fnesc_guid = ideapad_wmi_fnesc_events[i];
>> -			break;
>> -		}
>> -	}
>> +	priv->wmi_drv = (struct wmi_driver) {
>> +		.driver = {
>> +			.name = "ideapad-wmi-fn-keys",
>> +		},
>> +		.id_table = ideapad_wmi_id_table,
>> +		.probe = ideapad_wmi_probe,
>> +		.notify = ideapad_wmi_notify,
>> +	};
>>
>> -	if (ACPI_FAILURE(status) && status != AE_NOT_EXIST) {
>> -		err = -EIO;
>> -		goto notification_failed_wmi;
>> -	}
>> +	err = wmi_driver_register(&priv->wmi_drv);
>> +	if (err)
>> +		goto register_failed_wmi;
>>   #endif
>>
>>   	return 0;
>>
>>   #if IS_ENABLED(CONFIG_ACPI_WMI)
>> -notification_failed_wmi:
>> +register_failed_wmi:
>>   	acpi_remove_notify_handler(priv->adev->handle,
>>   				   ACPI_DEVICE_NOTIFY,
>>   				   ideapad_acpi_notify);
>> @@ -1720,8 +1772,7 @@ static int ideapad_acpi_remove(struct platform_device *pdev)
>>   	int i;
>>
>>   #if IS_ENABLED(CONFIG_ACPI_WMI)
>> -	if (priv->fnesc_guid)
>> -		wmi_remove_notify_handler(priv->fnesc_guid);
>> +	wmi_driver_unregister(&priv->wmi_drv);
>>   #endif
>>
>>   	acpi_remove_notify_handler(priv->adev->handle,
>> --
>> 2.38.1
>>

Re: [PATCH v3] platform/x86: ideapad-laptop: support for more special keys in WMI

[Hans de Goede]

Hi Armin,

On 11/15/22 03:27, Armin Wolf wrote:
> Am 14.11.22 um 17:41 schrieb Hans de Goede:
> 
>> Hi,
>>
>> On 11/14/22 15:41, Philipp Jungkamp wrote:
>>> The event data of the WMI event 0xD0, which is assumed to be the
>>> fn_lock, is used to indicate several special keys on newer Yoga 7/9
>>> laptops.
>>>
>>> The notify_id 0xD0 is non-unique in the DSDT of the Yoga 9 14IAP7, this
>>> causes wmi_get_event_data() to report wrong values.
>>> Port the ideapad-laptop WMI code to the wmi bus infrastructure which
>>> does not suffer from the shortcomings of wmi_get_event_data().
>>>
>>> Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
>>> ---
>>> Hello,
>>>
>>> is this about right? It works for me.
>>>
>>> What I don't really like here is the dev_set_drvdata() which takes a non-const
>>> void * and I pass it a const pointer. I do cast the value of dev_get_drvdata()
>>> back to a const pointer, but this seems rather ugly.
>>> I preferred it over allocating a single int for the device or casting an enum
>>> to a void *. This additionally removes the need for a remove funtion.
>> I decided just take quick peek and I think the cleanest solution here would
>> be to add a driver-data struct with the lookup result of:
>>
>>     struct ideapad_private *priv = container_of(wdrv,
>>                             struct ideapad_private,
>>                             wmi_drv);
>>
>> Stored in there + the enum value and then alloc it with devm_kzalloc
>> to avoid the need for a remove callback.
>>
>> I'm also wondering if we could then maybe not move some other variables
>> only used in the wmi_notify callback to that driver-data struct ?
>> (I did not check if there are any candidates).
>>
>> Also this is going to need a big fat warning (comment) that the cute trick
>> with registering a wmi_driver struct embedded inside the platform_driver
>> data struct very much relies on there being only one platform_device
>> instance to which the platform_driver will bind ever, otherwise
>> we will get multiple wmi_driver's registered for the same WMI GUIDs
>> and then the container-off will likely return the driver-data of the
>> first platform device ...
> 
> Hello,
> 
> i think this is not going to be the case. For each platform device instance,
> a separate ideapad_private struct is allocated and initialized. This means
> that for each platform device instance, a separate WMI driver is registered,
> so each WMI driver will access the private data of its platform device when
> using container_of(), not just the private data of the first one.
> 
> AFAIK, WMI drivers cannot bind to an already bound WMI device, otherwise the
> WMI bus would need to create multiple devices for a single GUID instance, which
> is, as far as i know, not the case.

Right that is true, this will only be a problem if a WMI device with
a matching GUID shows up after the driver has been registered, which
could happen if the driver is builtin and thus probed during enumaration
which means the platform device(s) get enumerated before the WMI devices.

This is all quite unlikely, but the whole trick of embedding a driver
structure inside a device-data struct and then using container_of
on it just goes against the whole Linux driver model and gives me
what I can only describe as "an uncomfortable feeling".

I see that you've already posted a v4 using the global shared pointer
approach. This very much is not pretty (either), but at least it is easy
to reason about / easy to follow what is going on without having to
worry about various lifetimes, etc. Much appreciated!

I'll try to review v4 soonish but as mentioned before chances are
I will not get around to this until next week.

Regards,

Hans



>> Which makes me wonder if it would not be cleaner to just use a global
>> pointer for this ?   Note this is an honest open question.
>>
>> Actually since the platform_device gets instantiated by the ACPI
>> core there is no guarantee there will be only 1. So I think that
>> the container_of on the wmi-driver trick needs to go, instead
>> introduce:
>>
>> 1. A global ideapad_private_data_mutex mutex
>> 2. A global ideapad_private_data pointer
>>
>> And:
>>
>> 1. In ideapad_acpi_add:
>> lock the mutex
>> check that ideapad_private_data is not already set and if it is bail with an error
>> set ideapad_private_data
>> unlock the mutex
>>
>> 2. in ideapad_acpi_remove:
>> lock the mutex
>> clear the golbal pointer
>> unlock the mutex
>>
>> 3. In the wmi-driver's notify method:
>> lock the mutex
>> check ideapad_private_data is not NULL
>> process event
>> unlock the mutex
>>
>> 4. replace module_platform_driver with normal module init + exit
>> functions which register both drivers / unregister both drivers.
>>
>> I believe that this will be a more clean approach then the embedded
>> wmi_driver struct cuteness.
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>> Regards,
>>> Philipp
>>>
>>>   drivers/platform/x86/ideapad-laptop.c | 109 +++++++++++++++++++-------
>>>   1 file changed, 80 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
>>> index 33b3dfdd1b08..6d35a9e961cf 100644
>>> --- a/drivers/platform/x86/ideapad-laptop.c
>>> +++ b/drivers/platform/x86/ideapad-laptop.c
>>> @@ -30,6 +30,7 @@
>>>   #include <linux/seq_file.h>
>>>   #include <linux/sysfs.h>
>>>   #include <linux/types.h>
>>> +#include <linux/wmi.h>
>>>
>>>   #include <acpi/video.h>
>>>
>>> @@ -38,10 +39,19 @@
>>>   #define IDEAPAD_RFKILL_DEV_NUM    3
>>>
>>>   #if IS_ENABLED(CONFIG_ACPI_WMI)
>>> -static const char *const ideapad_wmi_fnesc_events[] = {
>>> -    "26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", /* Yoga 3 */
>>> -    "56322276-8493-4CE8-A783-98C991274F5E", /* Yoga 700 */
>>> -    "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", /* Legion 5 */
>>> +enum ideapad_wmi_event_type {
>>> +    IDEAPAD_WMI_EVENT_ESC,
>>> +    IDEAPAD_WMI_EVENT_FN_KEYS,
>>> +};
>>> +
>>> +enum ideapad_wmi_event_type ideapad_wmi_esc = IDEAPAD_WMI_EVENT_ESC,
>>> +enum ideapad_wmi_event_type ideapad_wmi_fn_keys = IDEAPAD_WMI_EVENT_FN_KEYS;
>>> +
>>> +static const struct wmi_device_id ideapad_wmi_id_table[] = {
>>> +    { "26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", &ideapad_wmi_esc }, /* Yoga 3 */
>>> +    { "56322276-8493-4CE8-A783-98C991274F5E", &ideapad_wmi_esc }, /* Yoga 700 */
>>> +    { "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", &ideapad_wmi_fn_keys }, /* Legion 5 */
>>> +    {}
>>>   };
>>>   #endif
>>>
>>> @@ -130,7 +140,7 @@ struct ideapad_private {
>>>       struct ideapad_dytc_priv *dytc;
>>>       struct dentry *debug;
>>>       unsigned long cfg;
>>> -    const char *fnesc_guid;
>>> +    struct wmi_driver wmi_drv;
>>>       struct {
>>>           bool conservation_mode    : 1;
>>>           bool dytc                 : 1;
>>> @@ -1074,6 +1084,7 @@ static void ideapad_sysfs_exit(struct ideapad_private *priv)
>>>   /*
>>>    * input device
>>>    */
>>> +#define IDEAPAD_WMI_KEY 0x100
>>>   static const struct key_entry ideapad_keymap[] = {
>>>       { KE_KEY,   6, { KEY_SWITCHVIDEOMODE } },
>>>       { KE_KEY,   7, { KEY_CAMERA } },
>>> @@ -1087,6 +1098,26 @@ static const struct key_entry ideapad_keymap[] = {
>>>       { KE_KEY,  66, { KEY_TOUCHPAD_OFF } },
>>>       { KE_KEY,  67, { KEY_TOUCHPAD_ON } },
>>>       { KE_KEY, 128, { KEY_ESC } },
>>> +
>>> +    /*
>>> +     * WMI keys
>>> +     */
>>> +
>>> +    /* FnLock (handled by the firmware) */
>>> +    { KE_IGNORE,    0x02 | IDEAPAD_WMI_KEY },
>>> +    /* Customizable Lenovo Hotkey ("star" with 'S' inside) */
>>> +    { KE_KEY,    0x01 | IDEAPAD_WMI_KEY, { KEY_FAVORITES } },
>>> +    /* Dark mode toggle */
>>> +    { KE_KEY,    0x13 | IDEAPAD_WMI_KEY, { KEY_PROG1 } },
>>> +    /* Sound profile switch */
>>> +    { KE_KEY,    0x12 | IDEAPAD_WMI_KEY, { KEY_PROG2 } },
>>> +    /* Lenovo Virtual Background application */
>>> +    { KE_KEY,    0x28 | IDEAPAD_WMI_KEY, { KEY_PROG3 } },
>>> +    /* Lenovo Support */
>>> +    { KE_KEY,    0x27 | IDEAPAD_WMI_KEY, { KEY_HELP } },
>>> +    /* Refresh Rate Toggle */
>>> +    { KE_KEY,    0x0a | IDEAPAD_WMI_KEY, { KEY_DISPLAYTOGGLE } },
>>> +
>>>       { KE_END },
>>>   };
>>>
>>> @@ -1491,25 +1522,47 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
>>>   }
>>>
>>>   #if IS_ENABLED(CONFIG_ACPI_WMI)
>>> -static void ideapad_wmi_notify(u32 value, void *context)
>>> +static int ideapad_wmi_probe(struct wmi_device *wdev, const void *context)
>>>   {
>>> -    struct ideapad_private *priv = context;
>>> +    dev_set_drvdata(&wdev->dev, (void *) context);
>>> +    return 0;
>>> +}
>>> +
>>> +static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
>>> +{
>>> +    struct wmi_driver *wdrv = container_of(wdev->dev.driver,
>>> +                           struct wmi_driver,
>>> +                           driver);
>>> +    struct ideapad_private *priv = container_of(wdrv,
>>> +                            struct ideapad_private,
>>> +                            wmi_drv);
>>> +    const enum ideapad_wmi_event_type *event = dev_get_drvdata(&wdev->dev);
>>>       unsigned long result;
>>>
>>> -    switch (value) {
>>> -    case 128:
>>> -        ideapad_input_report(priv, value);
>>> +    switch (*event) {
>>> +    case IDEAPAD_WMI_EVENT_ESC:
>>> +        ideapad_input_report(priv, 128);
>>>           break;
>>> -    case 208:
>>> +    case IDEAPAD_WMI_EVENT_FN_KEYS:
>>>           if (!eval_hals(priv->adev->handle, &result)) {
>>>               bool state = test_bit(HALS_FNLOCK_STATE_BIT, &result);
>>>
>>>               exec_sals(priv->adev->handle, state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
>>>           }
>>> +
>>> +        if (data->type != ACPI_TYPE_INTEGER) {
>>> +            dev_warn(&wdev->dev,
>>> +                 "WMI event data is not an integer\n");
>>> +            break;
>>> +        }
>>> +
>>> +        dev_dbg(&wdev->dev, "WMI fn-key event: 0x%llx\n",
>>> +            data->integer.value);
>>> +
>>> +        ideapad_input_report(priv,
>>> +                     data->integer.value | IDEAPAD_WMI_KEY);
>>> +
>>>           break;
>>> -    default:
>>> -        dev_info(&priv->platform_device->dev,
>>> -             "Unknown WMI event: %u\n", value);
>>>       }
>>>   }
>>>   #endif
>>> @@ -1671,25 +1724,24 @@ static int ideapad_acpi_add(struct platform_device *pdev)
>>>       }
>>>
>>>   #if IS_ENABLED(CONFIG_ACPI_WMI)
>>> -    for (i = 0; i < ARRAY_SIZE(ideapad_wmi_fnesc_events); i++) {
>>> -        status = wmi_install_notify_handler(ideapad_wmi_fnesc_events[i],
>>> -                            ideapad_wmi_notify, priv);
>>> -        if (ACPI_SUCCESS(status)) {
>>> -            priv->fnesc_guid = ideapad_wmi_fnesc_events[i];
>>> -            break;
>>> -        }
>>> -    }
>>> +    priv->wmi_drv = (struct wmi_driver) {
>>> +        .driver = {
>>> +            .name = "ideapad-wmi-fn-keys",
>>> +        },
>>> +        .id_table = ideapad_wmi_id_table,
>>> +        .probe = ideapad_wmi_probe,
>>> +        .notify = ideapad_wmi_notify,
>>> +    };
>>>
>>> -    if (ACPI_FAILURE(status) && status != AE_NOT_EXIST) {
>>> -        err = -EIO;
>>> -        goto notification_failed_wmi;
>>> -    }
>>> +    err = wmi_driver_register(&priv->wmi_drv);
>>> +    if (err)
>>> +        goto register_failed_wmi;
>>>   #endif
>>>
>>>       return 0;
>>>
>>>   #if IS_ENABLED(CONFIG_ACPI_WMI)
>>> -notification_failed_wmi:
>>> +register_failed_wmi:
>>>       acpi_remove_notify_handler(priv->adev->handle,
>>>                      ACPI_DEVICE_NOTIFY,
>>>                      ideapad_acpi_notify);
>>> @@ -1720,8 +1772,7 @@ static int ideapad_acpi_remove(struct platform_device *pdev)
>>>       int i;
>>>
>>>   #if IS_ENABLED(CONFIG_ACPI_WMI)
>>> -    if (priv->fnesc_guid)
>>> -        wmi_remove_notify_handler(priv->fnesc_guid);
>>> +    wmi_driver_unregister(&priv->wmi_drv);
>>>   #endif
>>>
>>>       acpi_remove_notify_handler(priv->adev->handle,
>>> -- 
>>> 2.38.1
>>>
> 

Re: [PATCH v4] platform/x86: ideapad-laptop: support for more special keys in WMI

[Hans de Goede]

Hi,

I ended up looking at this because of:

https://patchwork.kernel.org/project/platform-driver-x86/patch/2655386.mvXUDI8C0e@wirelessprv-10-193-125-189.near.illinois.edu/

so here is a full review.

I have just pushed a slightly modified version of the above patch to:

https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=fixes

see:

https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=fixes&id=a3c272fae1468984d2ec4499d665ad8989d18388

this should also answer the concerns you mentioned in a previous email about the :

		if (!eval_hals(priv->adev->handle, &result)) {
			bool state = test_bit(HALS_FNLOCK_STATE_BIT, &result);

			exec_sals(priv->adev->handle, state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
		}

part, since this will now be behind a DMI id based allow list,
since doing this everywhere clearly is a bad idea.

Also since you indicate that data->integer.value == 0x02 for
FnLock toggle, I believe that in your new code, this should
look something like this:

static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
{

...

	switch (wpriv->event) {
	case IDEAPAD_WMI_EVENT_ESC:
		ideapad_input_report(priv, 128);
		break;
	case IDEAPAD_WMI_EVENT_FN_KEYS:
		if (data->type != ACPI_TYPE_INTEGER) {
			dev_warn(&wdev->dev,
				 "WMI event data is not an integer\n");
			break;
		}

		dev_dbg(&wdev->dev, "WMI fn-key event: 0x%llx\n",
			data->integer.value);

		if (priv->features.set_fn_lock_led &&
		    data->integer.value == 0x02 &&
		    !eval_hals(priv->adev->handle, &result)) {
			bool state = test_bit(HALS_FNLOCK_STATE_BIT, &result);

			exec_sals(priv->adev->handle, state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
		}

		ideapad_input_report(priv,
				     data->integer.value | IDEAPAD_WMI_KEY);

		break;
	}


Note I have also pushed some other pending ideapad-laptop changes to my review-hans
branch, so please base v5 on top of my latest review-hans branch:

https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

I have some more comments inline below:

On 11/15/22 01:09, Philipp Jungkamp wrote:
> The event data of the WMI event 0xD0, which is assumed to be the
> fn_lock, is used to indicate several special keys on newer Yoga 7/9
> laptops.
> 
> The notify_id 0xD0 is non-unique in the DSDT of the Yoga 9 14IAP7, this
> causes wmi_get_event_data() to report wrong values.
> Port the ideapad-laptop WMI code to the wmi bus infrastructure which
> does not suffer from the shortcomings of wmi_get_event_data().
> 
> Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
> ---
> This now uses the WMI bus infrastructure with both the platform driver and the
> wmi driver registered on module load. Is the synchronization used here
> sufficient?

Yes this looks good to me, thank you for doing it this way.

> Thanks for all the help!

You're welcome. Some small remarks in line.

> 
> Regards,
> Philipp Jungkamp
> 
>  drivers/platform/x86/ideapad-laptop.c | 240 ++++++++++++++++++++------
>  1 file changed, 188 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index 33b3dfdd1b08..803bf5cf50f9 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -30,6 +30,7 @@
>  #include <linux/seq_file.h>
>  #include <linux/sysfs.h>
>  #include <linux/types.h>
> +#include <linux/wmi.h>
> 
>  #include <acpi/video.h>
> 
> @@ -38,10 +39,13 @@
>  #define IDEAPAD_RFKILL_DEV_NUM	3
> 
>  #if IS_ENABLED(CONFIG_ACPI_WMI)
> -static const char *const ideapad_wmi_fnesc_events[] = {
> -	"26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", /* Yoga 3 */
> -	"56322276-8493-4CE8-A783-98C991274F5E", /* Yoga 700 */
> -	"8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", /* Legion 5 */
> +enum ideapad_wmi_event_type {
> +	IDEAPAD_WMI_EVENT_ESC,
> +	IDEAPAD_WMI_EVENT_FN_KEYS,
> +};
> +
> +struct ideapad_wmi_private {
> +	enum ideapad_wmi_event_type event;
>  };
>  #endif
> 
> @@ -130,7 +134,6 @@ struct ideapad_private {
>  	struct ideapad_dytc_priv *dytc;
>  	struct dentry *debug;
>  	unsigned long cfg;
> -	const char *fnesc_guid;
>  	struct {
>  		bool conservation_mode    : 1;
>  		bool dytc                 : 1;
> @@ -156,6 +159,42 @@ static bool allow_v4_dytc;
>  module_param(allow_v4_dytc, bool, 0444);
>  MODULE_PARM_DESC(allow_v4_dytc, "Enable DYTC version 4 platform-profile support.");
> 
> +/*
> + * shared data
> + */
> +
> +static struct ideapad_private *ideapad_shared;
> +static DEFINE_MUTEX(ideapad_shared_mutex);
> +
> +static int ideapad_shared_init(struct ideapad_private *priv)
> +{
> +	int ret;
> +
> +	mutex_lock(&ideapad_shared_mutex);
> +
> +	if (!ideapad_shared) {
> +		ideapad_shared = priv;
> +		ret = 0;
> +	} else {
> +		dev_warn(&priv->adev->dev, "found multiple platform devices\n");
> +		ret = -EINVAL;
> +	}
> +
> +	mutex_unlock(&ideapad_shared_mutex);
> +
> +	return ret;
> +}
> +
> +static void ideapad_shared_exit(struct ideapad_private *priv)
> +{
> +	mutex_lock(&ideapad_shared_mutex);
> +
> +	if (ideapad_shared == priv)
> +		ideapad_shared = NULL;
> +
> +	mutex_unlock(&ideapad_shared_mutex);
> +}
> +
>  /*
>   * ACPI Helpers
>   */
> @@ -1074,6 +1113,7 @@ static void ideapad_sysfs_exit(struct ideapad_private *priv)
>  /*
>   * input device
>   */
> +#define IDEAPAD_WMI_KEY 0x100
>  static const struct key_entry ideapad_keymap[] = {
>  	{ KE_KEY,   6, { KEY_SWITCHVIDEOMODE } },
>  	{ KE_KEY,   7, { KEY_CAMERA } },
> @@ -1087,6 +1127,28 @@ static const struct key_entry ideapad_keymap[] = {
>  	{ KE_KEY,  66, { KEY_TOUCHPAD_OFF } },
>  	{ KE_KEY,  67, { KEY_TOUCHPAD_ON } },
>  	{ KE_KEY, 128, { KEY_ESC } },
> +
> +	/*
> +	 * WMI keys
> +	 */
> +
> +	/* FnLock (handled by the firmware) */
> +	{ KE_IGNORE,	0x02 | IDEAPAD_WMI_KEY },
> +	/* Esc (handled by the firmware) */
> +	{ KE_IGNORE,	0x03 | IDEAPAD_WMI_KEY },
> +	/* Customizable Lenovo Hotkey ("star" with 'S' inside) */
> +	{ KE_KEY,	0x01 | IDEAPAD_WMI_KEY, { KEY_FAVORITES } },
> +	/* Dark mode toggle */
> +	{ KE_KEY,	0x13 | IDEAPAD_WMI_KEY, { KEY_PROG1 } },
> +	/* Sound profile switch */
> +	{ KE_KEY,	0x12 | IDEAPAD_WMI_KEY, { KEY_PROG2 } },
> +	/* Lenovo Virtual Background application */
> +	{ KE_KEY,	0x28 | IDEAPAD_WMI_KEY, { KEY_PROG3 } },
> +	/* Lenovo Support */
> +	{ KE_KEY,	0x27 | IDEAPAD_WMI_KEY, { KEY_HELP } },
> +	/* Refresh Rate Toggle */
> +	{ KE_KEY,	0x0a | IDEAPAD_WMI_KEY, { KEY_DISPLAYTOGGLE } },
> +
>  	{ KE_END },
>  };
> 
> @@ -1490,30 +1552,6 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
>  	}
>  }
> 
> -#if IS_ENABLED(CONFIG_ACPI_WMI)
> -static void ideapad_wmi_notify(u32 value, void *context)
> -{
> -	struct ideapad_private *priv = context;
> -	unsigned long result;
> -
> -	switch (value) {
> -	case 128:
> -		ideapad_input_report(priv, value);
> -		break;
> -	case 208:
> -		if (!eval_hals(priv->adev->handle, &result)) {
> -			bool state = test_bit(HALS_FNLOCK_STATE_BIT, &result);
> -
> -			exec_sals(priv->adev->handle, state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
> -		}
> -		break;
> -	default:
> -		dev_info(&priv->platform_device->dev,
> -			 "Unknown WMI event: %u\n", value);
> -	}
> -}
> -#endif
> -
>  /*
>   * Some ideapads have a hardware rfkill switch, but most do not have one.
>   * Reading VPCCMD_R_RF always results in 0 on models without a hardware rfkill,
> @@ -1589,6 +1627,95 @@ static void ideapad_check_features(struct ideapad_private *priv)
>  	}
>  }
> 
> +#if IS_ENABLED(CONFIG_ACPI_WMI)
> +/*
> + * WMI driver
> + */
> +static int ideapad_wmi_probe(struct wmi_device *wdev, const void *context)
> +{
> +	struct ideapad_wmi_private *wpriv;
> +
> +	wpriv = devm_kzalloc(&wdev->dev, sizeof(*wpriv), GFP_KERNEL);
> +	if (!wpriv)
> +		return -ENOMEM;
> +
> +	*wpriv = *(struct ideapad_wmi_private *)context;
> +
> +	dev_set_drvdata(&wdev->dev, wpriv);
> +	return 0;
> +}
> +
> +static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
> +{
> +	struct ideapad_wmi_private *wpriv = dev_get_drvdata(&wdev->dev);
> +	struct ideapad_private *priv;
> +	unsigned long result;
> +
> +	mutex_lock(&ideapad_shared_mutex);
> +
> +	priv = ideapad_shared;
> +	if (!priv)
> +		goto unlock;
> +
> +	switch (wpriv->event) {
> +	case IDEAPAD_WMI_EVENT_ESC:
> +		ideapad_input_report(priv, 128);
> +		break;
> +	case IDEAPAD_WMI_EVENT_FN_KEYS:
> +		if (!eval_hals(priv->adev->handle, &result)) {
> +			bool state = test_bit(HALS_FNLOCK_STATE_BIT, &result);
> +
> +			exec_sals(priv->adev->handle, state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
> +		}
> +
> +		if (data->type != ACPI_TYPE_INTEGER) {
> +			dev_warn(&wdev->dev,
> +				 "WMI event data is not an integer\n");
> +			break;
> +		}
> +
> +		dev_dbg(&wdev->dev, "WMI fn-key event: 0x%llx\n",
> +			data->integer.value);
> +
> +		ideapad_input_report(priv,
> +				     data->integer.value | IDEAPAD_WMI_KEY);
> +
> +		break;
> +	}
> +unlock:
> +	mutex_unlock(&ideapad_shared_mutex);
> +}
> +
> +struct ideapad_wmi_private ideapad_wmi_context_esc = {
> +	.event = IDEAPAD_WMI_EVENT_ESC
> +};
> +
> +struct ideapad_wmi_private ideapad_wmi_context_fn_keys = {
> +	.event = IDEAPAD_WMI_EVENT_FN_KEYS
> +};
> +
> +static const struct wmi_device_id ideapad_wmi_ids[] = {
> +	{ "26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", &ideapad_wmi_context_esc }, /* Yoga 3 */
> +	{ "56322276-8493-4CE8-A783-98C991274F5E", &ideapad_wmi_context_esc }, /* Yoga 700 */
> +	{ "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", &ideapad_wmi_context_fn_keys }, /* Legion 5 */
> +	{},
> +};
> +MODULE_DEVICE_TABLE(wmi, ideapad_wmi_ids);
> +
> +static struct wmi_driver ideapad_wmi_driver = {
> +	.driver = {
> +		.name = "ideapad_wmi",
> +	},
> +	.id_table = ideapad_wmi_ids,
> +	.probe = ideapad_wmi_probe,
> +	.notify = ideapad_wmi_notify,
> +};
> +

Please add the following here:

static int ideapad_wmi_register_driver(void)
{
	return wmi_driver_register(&ideapad_wmi_driver);
}

static void ideapad_wmi_unregister_driver(void)
{
	return wmi_driver_unregister(&ideapad_wmi_driver);
}

#else

static inline int ideapad_wmi_register_driver(void) { return 0; }
static inline void ideapad_wmi_unregister_driver(void) { }

> +#endif
> +
> +/*
> + * ACPI driver
> + */
>  static int ideapad_acpi_add(struct platform_device *pdev)
>  {
>  	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> @@ -1670,30 +1797,16 @@ static int ideapad_acpi_add(struct platform_device *pdev)
>  		goto notification_failed;
>  	}
> 
> -#if IS_ENABLED(CONFIG_ACPI_WMI)
> -	for (i = 0; i < ARRAY_SIZE(ideapad_wmi_fnesc_events); i++) {
> -		status = wmi_install_notify_handler(ideapad_wmi_fnesc_events[i],
> -						    ideapad_wmi_notify, priv);
> -		if (ACPI_SUCCESS(status)) {
> -			priv->fnesc_guid = ideapad_wmi_fnesc_events[i];
> -			break;
> -		}
> -	}
> -
> -	if (ACPI_FAILURE(status) && status != AE_NOT_EXIST) {
> -		err = -EIO;
> -		goto notification_failed_wmi;
> -	}
> -#endif
> +	err = ideapad_shared_init(priv);
> +	if (err)
> +		goto shared_init_failed;
> 
>  	return 0;
> 
> -#if IS_ENABLED(CONFIG_ACPI_WMI)
> -notification_failed_wmi:
> +shared_init_failed:
>  	acpi_remove_notify_handler(priv->adev->handle,
>  				   ACPI_DEVICE_NOTIFY,
>  				   ideapad_acpi_notify);
> -#endif
> 
>  notification_failed:
>  	ideapad_backlight_exit(priv);
> @@ -1719,10 +1832,7 @@ static int ideapad_acpi_remove(struct platform_device *pdev)
>  	struct ideapad_private *priv = dev_get_drvdata(&pdev->dev);
>  	int i;
> 
> -#if IS_ENABLED(CONFIG_ACPI_WMI)
> -	if (priv->fnesc_guid)
> -		wmi_remove_notify_handler(priv->fnesc_guid);
> -#endif
> +	ideapad_shared_exit(priv);
> 
>  	acpi_remove_notify_handler(priv->adev->handle,
>  				   ACPI_DEVICE_NOTIFY,
> @@ -1774,7 +1884,33 @@ static struct platform_driver ideapad_acpi_driver = {
>  	},
>  };
> 
> -module_platform_driver(ideapad_acpi_driver);
> +static int __init ideapad_laptop_init(void)
> +{
> +	int err;
> +
> +#if IS_ENABLED(CONFIG_ACPI_WMI)
> +	err = wmi_driver_register(&ideapad_wmi_driver);
> +	if (err)
> +		return err;
> +#endif

And use ideapad_wmi_register_driver() here and drop
the #ifdef-s. This using a little helper in the
existing/main #ifdef block + static inline no-op
versions in a #else there is a standard pattern in
the kernel to avoid sprinkling #ifdef-s everywhere.

> +
> +	err = platform_driver_register(&ideapad_acpi_driver);
> +	if (err)
> +		return err;

This needs to be:

	if (err) {
		ideapad_wmi__driver_unregister();
		return err;
	}

And the unregister here is also much cleaner with the wrappers
,avoiding another #ifdef  :)


> +
> +	return 0;
> +}
> +module_init(ideapad_laptop_init)
> +
> +static void __exit ideapad_laptop_exit(void)
> +{
> +#if IS_ENABLED(CONFIG_ACPI_WMI)
> +	wmi_driver_unregister(&ideapad_wmi_driver);
> +#endif

And use ideapad_wmi__driver_unregister here too.

Apart from my few small remarks above this looks good
and I expect that I can merge a v5 addressing these
remarks pretty quickly (so in time for the 6.2 merge window).

Regards,

Hans




> +
> +	platform_driver_unregister(&ideapad_acpi_driver);
> +}
> +module_exit(ideapad_laptop_exit)
> 
>  MODULE_AUTHOR("David Woodhouse <dwmw2@infradead.org>");
>  MODULE_DESCRIPTION("IdeaPad ACPI Extras");
> --
> 2.38.1
> 

[PATCH v5] platform/x86: ideapad-laptop: support for more special keys in WMI

[Philipp Jungkamp]

The event data of the WMI event 0xD0, which is assumed to be the
fn_lock, is used to indicate several special keys on newer Yoga 7/9
laptops.

The notify_id 0xD0 is non-unique in the DSDT of the Yoga 9 14IAP7, this
causes wmi_get_event_data() to report wrong values.
Port the ideapad-laptop WMI code to the wmi bus infrastructure which
does not suffer from the shortcomings of wmi_get_event_data().

Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
---
I did not add the "data->integer.value == 0x02" condition to the wmi_notify
handler. The event data 0x02 and 0x03 are both emitted when changing toggling
the fn-lock, depending on the new setting. Since I don't know whether the
event data checks out on the affected laptops I'd prefer to omit the check.

Regards,
Philipp Jungkamp

 drivers/platform/x86/ideapad-laptop.c | 255 ++++++++++++++++++++------
 1 file changed, 200 insertions(+), 55 deletions(-)

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index a279f41d984b..ac5ad0e36e5e 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -30,6 +30,7 @@
 #include <linux/seq_file.h>
 #include <linux/sysfs.h>
 #include <linux/types.h>
+#include <linux/wmi.h>

 #include <acpi/video.h>

@@ -38,10 +39,13 @@
 #define IDEAPAD_RFKILL_DEV_NUM	3

 #if IS_ENABLED(CONFIG_ACPI_WMI)
-static const char *const ideapad_wmi_fnesc_events[] = {
-	"26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", /* Yoga 3 */
-	"56322276-8493-4CE8-A783-98C991274F5E", /* Yoga 700 */
-	"8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", /* Legion 5 */
+enum ideapad_wmi_event_type {
+	IDEAPAD_WMI_EVENT_ESC,
+	IDEAPAD_WMI_EVENT_FN_KEYS,
+};
+
+struct ideapad_wmi_private {
+	enum ideapad_wmi_event_type event;
 };
 #endif

@@ -141,7 +145,6 @@ struct ideapad_private {
 	struct ideapad_dytc_priv *dytc;
 	struct dentry *debug;
 	unsigned long cfg;
-	const char *fnesc_guid;
 	struct {
 		bool conservation_mode    : 1;
 		bool dytc                 : 1;
@@ -182,6 +185,42 @@ MODULE_PARM_DESC(set_fn_lock_led,
 	"Enable driver based updates of the fn-lock LED on fn-lock changes. "
 	"If you need this please report this to: platform-driver-x86@vger.kernel.org");

+/*
+ * shared data
+ */
+
+static struct ideapad_private *ideapad_shared;
+static DEFINE_MUTEX(ideapad_shared_mutex);
+
+static int ideapad_shared_init(struct ideapad_private *priv)
+{
+	int ret;
+
+	mutex_lock(&ideapad_shared_mutex);
+
+	if (!ideapad_shared) {
+		ideapad_shared = priv;
+		ret = 0;
+	} else {
+		dev_warn(&priv->adev->dev, "found multiple platform devices\n");
+		ret = -EINVAL;
+	}
+
+	mutex_unlock(&ideapad_shared_mutex);
+
+	return ret;
+}
+
+static void ideapad_shared_exit(struct ideapad_private *priv)
+{
+	mutex_lock(&ideapad_shared_mutex);
+
+	if (ideapad_shared == priv)
+		ideapad_shared = NULL;
+
+	mutex_unlock(&ideapad_shared_mutex);
+}
+
 /*
  * ACPI Helpers
  */
@@ -1110,6 +1149,7 @@ static void ideapad_sysfs_exit(struct ideapad_private *priv)
 /*
  * input device
  */
+#define IDEAPAD_WMI_KEY 0x100
 static const struct key_entry ideapad_keymap[] = {
 	{ KE_KEY,   6, { KEY_SWITCHVIDEOMODE } },
 	{ KE_KEY,   7, { KEY_CAMERA } },
@@ -1123,6 +1163,28 @@ static const struct key_entry ideapad_keymap[] = {
 	{ KE_KEY,  66, { KEY_TOUCHPAD_OFF } },
 	{ KE_KEY,  67, { KEY_TOUCHPAD_ON } },
 	{ KE_KEY, 128, { KEY_ESC } },
+
+	/*
+	 * WMI keys
+	 */
+
+	/* FnLock (handled by the firmware) */
+	{ KE_IGNORE,	0x02 | IDEAPAD_WMI_KEY },
+	/* Esc (handled by the firmware) */
+	{ KE_IGNORE,	0x03 | IDEAPAD_WMI_KEY },
+	/* Customizable Lenovo Hotkey ("star" with 'S' inside) */
+	{ KE_KEY,	0x01 | IDEAPAD_WMI_KEY, { KEY_FAVORITES } },
+	/* Dark mode toggle */
+	{ KE_KEY,	0x13 | IDEAPAD_WMI_KEY, { KEY_PROG1 } },
+	/* Sound profile switch */
+	{ KE_KEY,	0x12 | IDEAPAD_WMI_KEY, { KEY_PROG2 } },
+	/* Lenovo Virtual Background application */
+	{ KE_KEY,	0x28 | IDEAPAD_WMI_KEY, { KEY_PROG3 } },
+	/* Lenovo Support */
+	{ KE_KEY,	0x27 | IDEAPAD_WMI_KEY, { KEY_HELP } },
+	/* Refresh Rate Toggle */
+	{ KE_KEY,	0x0a | IDEAPAD_WMI_KEY, { KEY_DISPLAYTOGGLE } },
+
 	{ KE_END },
 };

@@ -1526,33 +1588,6 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
 	}
 }

-#if IS_ENABLED(CONFIG_ACPI_WMI)
-static void ideapad_wmi_notify(u32 value, void *context)
-{
-	struct ideapad_private *priv = context;
-	unsigned long result;
-
-	switch (value) {
-	case 128:
-		ideapad_input_report(priv, value);
-		break;
-	case 208:
-		if (!priv->features.set_fn_lock_led)
-			break;
-
-		if (!eval_hals(priv->adev->handle, &result)) {
-			bool state = test_bit(HALS_FNLOCK_STATE_BIT, &result);
-
-			exec_sals(priv->adev->handle, state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
-		}
-		break;
-	default:
-		dev_info(&priv->platform_device->dev,
-			 "Unknown WMI event: %u\n", value);
-	}
-}
-#endif
-
 /* On some models we need to call exec_sals(SALS_FNLOCK_ON/OFF to set the LED */
 static const struct dmi_system_id set_fn_lock_led_list[] = {
 	{
@@ -1643,6 +1678,110 @@ static void ideapad_check_features(struct ideapad_private *priv)
 	}
 }

+#if IS_ENABLED(CONFIG_ACPI_WMI)
+/*
+ * WMI driver
+ */
+static int ideapad_wmi_probe(struct wmi_device *wdev, const void *context)
+{
+	struct ideapad_wmi_private *wpriv;
+
+	wpriv = devm_kzalloc(&wdev->dev, sizeof(*wpriv), GFP_KERNEL);
+	if (!wpriv)
+		return -ENOMEM;
+
+	*wpriv = *(struct ideapad_wmi_private *)context;
+
+	dev_set_drvdata(&wdev->dev, wpriv);
+	return 0;
+}
+
+static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
+{
+	struct ideapad_wmi_private *wpriv = dev_get_drvdata(&wdev->dev);
+	struct ideapad_private *priv;
+	unsigned long result;
+
+	mutex_lock(&ideapad_shared_mutex);
+
+	priv = ideapad_shared;
+	if (!priv)
+		goto unlock;
+
+	switch (wpriv->event) {
+	case IDEAPAD_WMI_EVENT_ESC:
+		ideapad_input_report(priv, 128);
+		break;
+	case IDEAPAD_WMI_EVENT_FN_KEYS:
+		if (priv->features.set_fn_lock_led &&
+		    !eval_hals(priv->adev->handle, &result)) {
+			bool state = test_bit(HALS_FNLOCK_STATE_BIT, &result);
+
+			exec_sals(priv->adev->handle, state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
+		}
+
+		if (data->type != ACPI_TYPE_INTEGER) {
+			dev_warn(&wdev->dev,
+				 "WMI event data is not an integer\n");
+			break;
+		}
+
+		dev_dbg(&wdev->dev, "WMI fn-key event: 0x%llx\n",
+			data->integer.value);
+
+		ideapad_input_report(priv,
+				     data->integer.value | IDEAPAD_WMI_KEY);
+
+		break;
+	}
+unlock:
+	mutex_unlock(&ideapad_shared_mutex);
+}
+
+struct ideapad_wmi_private ideapad_wmi_context_esc = {
+	.event = IDEAPAD_WMI_EVENT_ESC
+};
+
+struct ideapad_wmi_private ideapad_wmi_context_fn_keys = {
+	.event = IDEAPAD_WMI_EVENT_FN_KEYS
+};
+
+static const struct wmi_device_id ideapad_wmi_ids[] = {
+	{ "26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", &ideapad_wmi_context_esc }, /* Yoga 3 */
+	{ "56322276-8493-4CE8-A783-98C991274F5E", &ideapad_wmi_context_esc }, /* Yoga 700 */
+	{ "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", &ideapad_wmi_context_fn_keys }, /* Legion 5 */
+	{},
+};
+MODULE_DEVICE_TABLE(wmi, ideapad_wmi_ids);
+
+static struct wmi_driver ideapad_wmi_driver = {
+	.driver = {
+		.name = "ideapad_wmi",
+	},
+	.id_table = ideapad_wmi_ids,
+	.probe = ideapad_wmi_probe,
+	.notify = ideapad_wmi_notify,
+};
+
+static int ideapad_wmi_driver_register(void)
+{
+	return wmi_driver_register(&ideapad_wmi_driver);
+}
+
+static void ideapad_wmi_driver_unregister(void)
+{
+	return wmi_driver_unregister(&ideapad_wmi_driver);
+}
+
+#else
+
+static inline int ideapad_wmi_driver_register(void) { return 0; }
+static inline void ideapad_wmi_driver_unregister(void) { }
+#endif
+
+/*
+ * ACPI driver
+ */
 static int ideapad_acpi_add(struct platform_device *pdev)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
@@ -1724,30 +1863,16 @@ static int ideapad_acpi_add(struct platform_device *pdev)
 		goto notification_failed;
 	}

-#if IS_ENABLED(CONFIG_ACPI_WMI)
-	for (i = 0; i < ARRAY_SIZE(ideapad_wmi_fnesc_events); i++) {
-		status = wmi_install_notify_handler(ideapad_wmi_fnesc_events[i],
-						    ideapad_wmi_notify, priv);
-		if (ACPI_SUCCESS(status)) {
-			priv->fnesc_guid = ideapad_wmi_fnesc_events[i];
-			break;
-		}
-	}
-
-	if (ACPI_FAILURE(status) && status != AE_NOT_EXIST) {
-		err = -EIO;
-		goto notification_failed_wmi;
-	}
-#endif
+	err = ideapad_shared_init(priv);
+	if (err)
+		goto shared_init_failed;

 	return 0;

-#if IS_ENABLED(CONFIG_ACPI_WMI)
-notification_failed_wmi:
+shared_init_failed:
 	acpi_remove_notify_handler(priv->adev->handle,
 				   ACPI_DEVICE_NOTIFY,
 				   ideapad_acpi_notify);
-#endif

 notification_failed:
 	ideapad_backlight_exit(priv);
@@ -1773,10 +1898,7 @@ static int ideapad_acpi_remove(struct platform_device *pdev)
 	struct ideapad_private *priv = dev_get_drvdata(&pdev->dev);
 	int i;

-#if IS_ENABLED(CONFIG_ACPI_WMI)
-	if (priv->fnesc_guid)
-		wmi_remove_notify_handler(priv->fnesc_guid);
-#endif
+	ideapad_shared_exit(priv);

 	acpi_remove_notify_handler(priv->adev->handle,
 				   ACPI_DEVICE_NOTIFY,
@@ -1828,7 +1950,30 @@ static struct platform_driver ideapad_acpi_driver = {
 	},
 };

-module_platform_driver(ideapad_acpi_driver);
+static int __init ideapad_laptop_init(void)
+{
+	int err;
+
+	err = ideapad_wmi_driver_register();
+	if (err)
+		return err;
+
+	err = platform_driver_register(&ideapad_acpi_driver);
+	if (err) {
+		ideapad_wmi_driver_unregister();
+		return err;
+	}
+
+	return 0;
+}
+module_init(ideapad_laptop_init)
+
+static void __exit ideapad_laptop_exit(void)
+{
+	ideapad_wmi_driver_unregister();
+	platform_driver_unregister(&ideapad_acpi_driver);
+}
+module_exit(ideapad_laptop_exit)

 MODULE_AUTHOR("David Woodhouse <dwmw2@infradead.org>");
 MODULE_DESCRIPTION("IdeaPad ACPI Extras");
--
2.38.1

Re: [PATCH v5] platform/x86: ideapad-laptop: support for more special keys in WMI

[Hans de Goede]

Hi,

On 11/16/22 12:06, Philipp Jungkamp wrote:
> The event data of the WMI event 0xD0, which is assumed to be the
> fn_lock, is used to indicate several special keys on newer Yoga 7/9
> laptops.
> 
> The notify_id 0xD0 is non-unique in the DSDT of the Yoga 9 14IAP7, this
> causes wmi_get_event_data() to report wrong values.
> Port the ideapad-laptop WMI code to the wmi bus infrastructure which
> does not suffer from the shortcomings of wmi_get_event_data().
> 
> Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
> ---
> I did not add the "data->integer.value == 0x02" condition to the wmi_notify
> handler. The event data 0x02 and 0x03 are both emitted when changing toggling
> the fn-lock, depending on the new setting. Since I don't know whether the
> event data checks out on the affected laptops I'd prefer to omit the check.

Ok, that is fair. Maybe one day we can actually get someone to run some
tests and add an data->integer.value check later.

I have merged this now, with some small fixups, see comments inline / below
for the fixups:

<std patch accepted message>
Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.
</std patch accepted message>


>  drivers/platform/x86/ideapad-laptop.c | 255 ++++++++++++++++++++------
>  1 file changed, 200 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index a279f41d984b..ac5ad0e36e5e 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -30,6 +30,7 @@
>  #include <linux/seq_file.h>
>  #include <linux/sysfs.h>
>  #include <linux/types.h>
> +#include <linux/wmi.h>
> 
>  #include <acpi/video.h>
> 
> @@ -38,10 +39,13 @@
>  #define IDEAPAD_RFKILL_DEV_NUM	3
> 
>  #if IS_ENABLED(CONFIG_ACPI_WMI)
> -static const char *const ideapad_wmi_fnesc_events[] = {
> -	"26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", /* Yoga 3 */
> -	"56322276-8493-4CE8-A783-98C991274F5E", /* Yoga 700 */
> -	"8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", /* Legion 5 */
> +enum ideapad_wmi_event_type {
> +	IDEAPAD_WMI_EVENT_ESC,
> +	IDEAPAD_WMI_EVENT_FN_KEYS,
> +};
> +
> +struct ideapad_wmi_private {
> +	enum ideapad_wmi_event_type event;
>  };
>  #endif

These declarations are not used outside of the other

#if IS_ENABLED(CONFIG_ACPI_WMI) block, so I have
moved them to the top of that block to avoid
the extra #if / #endif pair


> @@ -141,7 +145,6 @@ struct ideapad_private {
>  	struct ideapad_dytc_priv *dytc;
>  	struct dentry *debug;
>  	unsigned long cfg;
> -	const char *fnesc_guid;
>  	struct {
>  		bool conservation_mode    : 1;
>  		bool dytc                 : 1;
> @@ -182,6 +185,42 @@ MODULE_PARM_DESC(set_fn_lock_led,
>  	"Enable driver based updates of the fn-lock LED on fn-lock changes. "
>  	"If you need this please report this to: platform-driver-x86@vger.kernel.org");
> 
> +/*
> + * shared data
> + */
> +
> +static struct ideapad_private *ideapad_shared;
> +static DEFINE_MUTEX(ideapad_shared_mutex);
> +
> +static int ideapad_shared_init(struct ideapad_private *priv)
> +{
> +	int ret;
> +
> +	mutex_lock(&ideapad_shared_mutex);
> +
> +	if (!ideapad_shared) {
> +		ideapad_shared = priv;
> +		ret = 0;
> +	} else {
> +		dev_warn(&priv->adev->dev, "found multiple platform devices\n");
> +		ret = -EINVAL;
> +	}
> +
> +	mutex_unlock(&ideapad_shared_mutex);
> +
> +	return ret;
> +}
> +
> +static void ideapad_shared_exit(struct ideapad_private *priv)
> +{
> +	mutex_lock(&ideapad_shared_mutex);
> +
> +	if (ideapad_shared == priv)
> +		ideapad_shared = NULL;
> +
> +	mutex_unlock(&ideapad_shared_mutex);
> +}
> +
>  /*
>   * ACPI Helpers
>   */
> @@ -1110,6 +1149,7 @@ static void ideapad_sysfs_exit(struct ideapad_private *priv)
>  /*
>   * input device
>   */
> +#define IDEAPAD_WMI_KEY 0x100
>  static const struct key_entry ideapad_keymap[] = {
>  	{ KE_KEY,   6, { KEY_SWITCHVIDEOMODE } },
>  	{ KE_KEY,   7, { KEY_CAMERA } },
> @@ -1123,6 +1163,28 @@ static const struct key_entry ideapad_keymap[] = {
>  	{ KE_KEY,  66, { KEY_TOUCHPAD_OFF } },
>  	{ KE_KEY,  67, { KEY_TOUCHPAD_ON } },
>  	{ KE_KEY, 128, { KEY_ESC } },
> +
> +	/*
> +	 * WMI keys
> +	 */
> +
> +	/* FnLock (handled by the firmware) */
> +	{ KE_IGNORE,	0x02 | IDEAPAD_WMI_KEY },
> +	/* Esc (handled by the firmware) */
> +	{ KE_IGNORE,	0x03 | IDEAPAD_WMI_KEY },
> +	/* Customizable Lenovo Hotkey ("star" with 'S' inside) */
> +	{ KE_KEY,	0x01 | IDEAPAD_WMI_KEY, { KEY_FAVORITES } },
> +	/* Dark mode toggle */
> +	{ KE_KEY,	0x13 | IDEAPAD_WMI_KEY, { KEY_PROG1 } },
> +	/* Sound profile switch */
> +	{ KE_KEY,	0x12 | IDEAPAD_WMI_KEY, { KEY_PROG2 } },
> +	/* Lenovo Virtual Background application */
> +	{ KE_KEY,	0x28 | IDEAPAD_WMI_KEY, { KEY_PROG3 } },
> +	/* Lenovo Support */
> +	{ KE_KEY,	0x27 | IDEAPAD_WMI_KEY, { KEY_HELP } },
> +	/* Refresh Rate Toggle */
> +	{ KE_KEY,	0x0a | IDEAPAD_WMI_KEY, { KEY_DISPLAYTOGGLE } },
> +
>  	{ KE_END },
>  };
> 
> @@ -1526,33 +1588,6 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
>  	}
>  }
> 
> -#if IS_ENABLED(CONFIG_ACPI_WMI)
> -static void ideapad_wmi_notify(u32 value, void *context)
> -{
> -	struct ideapad_private *priv = context;
> -	unsigned long result;
> -
> -	switch (value) {
> -	case 128:
> -		ideapad_input_report(priv, value);
> -		break;
> -	case 208:
> -		if (!priv->features.set_fn_lock_led)
> -			break;
> -
> -		if (!eval_hals(priv->adev->handle, &result)) {
> -			bool state = test_bit(HALS_FNLOCK_STATE_BIT, &result);
> -
> -			exec_sals(priv->adev->handle, state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
> -		}
> -		break;
> -	default:
> -		dev_info(&priv->platform_device->dev,
> -			 "Unknown WMI event: %u\n", value);
> -	}
> -}
> -#endif
> -
>  /* On some models we need to call exec_sals(SALS_FNLOCK_ON/OFF to set the LED */
>  static const struct dmi_system_id set_fn_lock_led_list[] = {
>  	{
> @@ -1643,6 +1678,110 @@ static void ideapad_check_features(struct ideapad_private *priv)
>  	}
>  }
> 
> +#if IS_ENABLED(CONFIG_ACPI_WMI)
> +/*
> + * WMI driver
> + */
> +static int ideapad_wmi_probe(struct wmi_device *wdev, const void *context)
> +{
> +	struct ideapad_wmi_private *wpriv;
> +
> +	wpriv = devm_kzalloc(&wdev->dev, sizeof(*wpriv), GFP_KERNEL);
> +	if (!wpriv)
> +		return -ENOMEM;
> +
> +	*wpriv = *(struct ideapad_wmi_private *)context;
> +
> +	dev_set_drvdata(&wdev->dev, wpriv);
> +	return 0;
> +}
> +
> +static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
> +{
> +	struct ideapad_wmi_private *wpriv = dev_get_drvdata(&wdev->dev);
> +	struct ideapad_private *priv;
> +	unsigned long result;
> +
> +	mutex_lock(&ideapad_shared_mutex);
> +
> +	priv = ideapad_shared;
> +	if (!priv)
> +		goto unlock;
> +
> +	switch (wpriv->event) {
> +	case IDEAPAD_WMI_EVENT_ESC:
> +		ideapad_input_report(priv, 128);
> +		break;
> +	case IDEAPAD_WMI_EVENT_FN_KEYS:
> +		if (priv->features.set_fn_lock_led &&
> +		    !eval_hals(priv->adev->handle, &result)) {
> +			bool state = test_bit(HALS_FNLOCK_STATE_BIT, &result);
> +
> +			exec_sals(priv->adev->handle, state ? SALS_FNLOCK_ON : SALS_FNLOCK_OFF);
> +		}
> +
> +		if (data->type != ACPI_TYPE_INTEGER) {
> +			dev_warn(&wdev->dev,
> +				 "WMI event data is not an integer\n");
> +			break;
> +		}
> +
> +		dev_dbg(&wdev->dev, "WMI fn-key event: 0x%llx\n",
> +			data->integer.value);
> +
> +		ideapad_input_report(priv,
> +				     data->integer.value | IDEAPAD_WMI_KEY);
> +
> +		break;
> +	}
> +unlock:
> +	mutex_unlock(&ideapad_shared_mutex);
> +}
> +
> +struct ideapad_wmi_private ideapad_wmi_context_esc = {
> +	.event = IDEAPAD_WMI_EVENT_ESC
> +};
> +
> +struct ideapad_wmi_private ideapad_wmi_context_fn_keys = {
> +	.event = IDEAPAD_WMI_EVENT_FN_KEYS
> +};

These 2 can be (and should be) "static const struct ..." I've
fixed this up while merging the patch.

Other then this, the patch looks great (and has been merged)
thank you for your work on this.

Regards,

Hans










> +
> +static const struct wmi_device_id ideapad_wmi_ids[] = {
> +	{ "26CAB2E5-5CF1-46AE-AAC3-4A12B6BA50E6", &ideapad_wmi_context_esc }, /* Yoga 3 */
> +	{ "56322276-8493-4CE8-A783-98C991274F5E", &ideapad_wmi_context_esc }, /* Yoga 700 */
> +	{ "8FC0DE0C-B4E4-43FD-B0F3-8871711C1294", &ideapad_wmi_context_fn_keys }, /* Legion 5 */
> +	{},
> +};
> +MODULE_DEVICE_TABLE(wmi, ideapad_wmi_ids);
> +
> +static struct wmi_driver ideapad_wmi_driver = {
> +	.driver = {
> +		.name = "ideapad_wmi",
> +	},
> +	.id_table = ideapad_wmi_ids,
> +	.probe = ideapad_wmi_probe,
> +	.notify = ideapad_wmi_notify,
> +};
> +
> +static int ideapad_wmi_driver_register(void)
> +{
> +	return wmi_driver_register(&ideapad_wmi_driver);
> +}
> +
> +static void ideapad_wmi_driver_unregister(void)
> +{
> +	return wmi_driver_unregister(&ideapad_wmi_driver);
> +}
> +
> +#else
> +
> +static inline int ideapad_wmi_driver_register(void) { return 0; }
> +static inline void ideapad_wmi_driver_unregister(void) { }
> +#endif
> +
> +/*
> + * ACPI driver
> + */
>  static int ideapad_acpi_add(struct platform_device *pdev)
>  {
>  	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> @@ -1724,30 +1863,16 @@ static int ideapad_acpi_add(struct platform_device *pdev)
>  		goto notification_failed;
>  	}
> 
> -#if IS_ENABLED(CONFIG_ACPI_WMI)
> -	for (i = 0; i < ARRAY_SIZE(ideapad_wmi_fnesc_events); i++) {
> -		status = wmi_install_notify_handler(ideapad_wmi_fnesc_events[i],
> -						    ideapad_wmi_notify, priv);
> -		if (ACPI_SUCCESS(status)) {
> -			priv->fnesc_guid = ideapad_wmi_fnesc_events[i];
> -			break;
> -		}
> -	}
> -
> -	if (ACPI_FAILURE(status) && status != AE_NOT_EXIST) {
> -		err = -EIO;
> -		goto notification_failed_wmi;
> -	}
> -#endif
> +	err = ideapad_shared_init(priv);
> +	if (err)
> +		goto shared_init_failed;
> 
>  	return 0;
> 
> -#if IS_ENABLED(CONFIG_ACPI_WMI)
> -notification_failed_wmi:
> +shared_init_failed:
>  	acpi_remove_notify_handler(priv->adev->handle,
>  				   ACPI_DEVICE_NOTIFY,
>  				   ideapad_acpi_notify);
> -#endif
> 
>  notification_failed:
>  	ideapad_backlight_exit(priv);
> @@ -1773,10 +1898,7 @@ static int ideapad_acpi_remove(struct platform_device *pdev)
>  	struct ideapad_private *priv = dev_get_drvdata(&pdev->dev);
>  	int i;
> 
> -#if IS_ENABLED(CONFIG_ACPI_WMI)
> -	if (priv->fnesc_guid)
> -		wmi_remove_notify_handler(priv->fnesc_guid);
> -#endif
> +	ideapad_shared_exit(priv);
> 
>  	acpi_remove_notify_handler(priv->adev->handle,
>  				   ACPI_DEVICE_NOTIFY,
> @@ -1828,7 +1950,30 @@ static struct platform_driver ideapad_acpi_driver = {
>  	},
>  };
> 
> -module_platform_driver(ideapad_acpi_driver);
> +static int __init ideapad_laptop_init(void)
> +{
> +	int err;
> +
> +	err = ideapad_wmi_driver_register();
> +	if (err)
> +		return err;
> +
> +	err = platform_driver_register(&ideapad_acpi_driver);
> +	if (err) {
> +		ideapad_wmi_driver_unregister();
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +module_init(ideapad_laptop_init)
> +
> +static void __exit ideapad_laptop_exit(void)
> +{
> +	ideapad_wmi_driver_unregister();
> +	platform_driver_unregister(&ideapad_acpi_driver);
> +}
> +module_exit(ideapad_laptop_exit)
> 
>  MODULE_AUTHOR("David Woodhouse <dwmw2@infradead.org>");
>  MODULE_DESCRIPTION("IdeaPad ACPI Extras");
> --
> 2.38.1
>