[RFC PATCH] Add Lenovo Yoga Mode Control driver

[RFC PATCH] Add Lenovo Yoga Mode Control driver

[Gergo Koteles]

This is a WMI driver for the Mode Control switch for Lenovo Yoga
notebooks.
The mode is mapped to a SW_TABLET_MODE switch capable input device.

It should work with  models like the Yoga C940, Ideapad flex 14API,
Yoga 9 14IAP7, Yoga 7 14ARB7.
The 14ARB7 model must trigger the EC to send mode change events.
This might be a firmware bug.

Introduces a global variable in the ideapad-laptop driver.
I would like some advice on how to do it without the variable,
or how to do it better.
---
 drivers/platform/x86/Kconfig          |  10 ++
 drivers/platform/x86/Makefile         |   1 +
 drivers/platform/x86/ideapad-laptop.c |  18 +++
 drivers/platform/x86/lenovo-ymc.c     | 185 ++++++++++++++++++++++++++
 4 files changed, 214 insertions(+)
 create mode 100644 drivers/platform/x86/lenovo-ymc.c

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index f2f98e942cf2..72ff6713e447 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -510,6 +510,16 @@ config IDEAPAD_LAPTOP
 	  This is a driver for Lenovo IdeaPad netbooks contains drivers for
 	  rfkill switch, hotkey, fan control and backlight control.
 
+config LENOVO_YMC
+	tristate "Lenovo Yoga Mode Control"
+	depends on ACPI_WMI
+	depends on INPUT
+	depends on IDEAPAD_LAPTOP
+	select ACPI_PLATFORM_PROFILE
+	select INPUT_SPARSEKMAP
+	help
+	  This is a driver for the Mode Control switch for Lenovo Yoga notebooks.
+
 config SENSORS_HDAPS
 	tristate "Thinkpad Hard Drive Active Protection System (hdaps)"
 	depends on INPUT
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 5a428caa654a..f8f0d6a0b43b 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_UV_SYSFS)       += uv_sysfs.o
 # IBM Thinkpad and Lenovo
 obj-$(CONFIG_IBM_RTL)		+= ibm_rtl.o
 obj-$(CONFIG_IDEAPAD_LAPTOP)	+= ideapad-laptop.o
+obj-$(CONFIG_LENOVO_YMC)	+= lenovo-ymc.o
 obj-$(CONFIG_SENSORS_HDAPS)	+= hdaps.o
 obj-$(CONFIG_THINKPAD_ACPI)	+= thinkpad_acpi.o
 obj-$(CONFIG_THINKPAD_LMI)	+= think-lmi.o
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index abd0c81d62c4..d88a4ee72bf6 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -103,6 +103,7 @@ enum {
 	VPCCMD_W_FAN,
 	VPCCMD_R_RF,
 	VPCCMD_W_RF,
+	VPCCMD_W_YMC = 0x2A,
 	VPCCMD_R_FAN = 0x2B,
 	VPCCMD_R_SPECIAL_BUTTONS = 0x31,
 	VPCCMD_W_BL_POWER = 0x33,
@@ -156,6 +157,8 @@ 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.");
 
+static struct ideapad_private *ideapad_priv;
+
 /*
  * ACPI Helpers
  */
@@ -1421,6 +1424,19 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv)
 	}
 }
 
+void ideapad_trigger_ymc_next_read(void)
+{
+	int err;
+	if (ideapad_priv != NULL) {
+		err = write_ec_cmd(ideapad_priv->adev->handle,
+			VPCCMD_W_YMC, 1);
+		if (err)
+			dev_warn(&ideapad_priv->platform_device->dev,
+				"Could not write YMC: %d\n", err);
+	}
+}
+EXPORT_SYMBOL(ideapad_trigger_ymc_next_read);
+
 static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct ideapad_private *priv = data;
@@ -1663,6 +1679,7 @@ static int ideapad_acpi_add(struct platform_device *pdev)
 	}
 #endif
 
+	ideapad_priv = priv;
 	return 0;
 
 #if IS_ENABLED(CONFIG_ACPI_WMI)
@@ -1696,6 +1713,7 @@ static int ideapad_acpi_remove(struct platform_device *pdev)
 	struct ideapad_private *priv = dev_get_drvdata(&pdev->dev);
 	int i;
 
+	ideapad_priv = NULL;
 #if IS_ENABLED(CONFIG_ACPI_WMI)
 	if (priv->fnesc_guid)
 		wmi_remove_notify_handler(priv->fnesc_guid);
diff --git a/drivers/platform/x86/lenovo-ymc.c b/drivers/platform/x86/lenovo-ymc.c
new file mode 100644
index 000000000000..0b899b02e12f
--- /dev/null
+++ b/drivers/platform/x86/lenovo-ymc.c
@@ -0,0 +1,185 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * lenovo-ymc.c - Lenovo Yoga Mode Control driver
+ *
+ * Copyright © 2022 Gergo Koteles <soyer@irl.hu>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/acpi.h>
+#include <linux/dmi.h>
+#include <linux/input.h>
+#include <linux/input/sparse-keymap.h>
+#include <linux/wmi.h>
+
+#define LENOVO_YMC_EVENT_GUID	"06129D99-6083-4164-81AD-F092F9D773A6"
+#define LENOVO_YMC_QUERY_GUID	"09B0EE6E-C3FD-4243-8DA1-7911FF80BB8C"
+
+#define LENOVO_YMC_QUERY_INSTANCE 0
+#define LENOVO_YMC_QUERY_METHOD 0xAB
+
+extern void ideapad_trigger_ymc_next_read(void);
+
+static bool ec_trigger __read_mostly;
+module_param(ec_trigger, bool, 0644);
+MODULE_PARM_DESC(ec_trigger, "Enable EC triggering to emit YMC events");
+
+static int enable_ec_trigger(const struct dmi_system_id *id)
+{
+       pr_debug("Lenovo YMC enable EC triggering.\n");
+       ec_trigger = true;
+       return 0;
+}
+
+static const struct dmi_system_id ec_trigger_quirk_dmi_table[] = {
+	{
+		// Lenovo Yoga 7 14ARB7
+		.callback = enable_ec_trigger,
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "82QF"),
+		},
+	},
+	{ },
+};
+
+static void lenovo_ymc_trigger_ec(void) {
+	if (ec_trigger) {
+		ideapad_trigger_ymc_next_read();
+	}
+}
+
+
+struct lenovo_ymc_private {
+	struct input_dev *input_dev;
+};
+
+
+static const struct key_entry lenovo_ymc_keymap[] = {
+	// Laptop
+	{ KE_SW, 0x01, { .sw = { SW_TABLET_MODE, 0 } } },
+	// Tablet
+	{ KE_SW, 0x02, { .sw = { SW_TABLET_MODE, 1 } } },
+	// Drawing Board
+	{ KE_SW, 0x03, { .sw = { SW_TABLET_MODE, 1 } } },
+	// Tent
+	{ KE_SW, 0x04, { .sw = { SW_TABLET_MODE, 1 } } },
+	{ KE_END },
+};
+
+static void lenovo_ymc_notify(struct wmi_device *wdev, union acpi_object *data)
+{
+	struct lenovo_ymc_private *priv = dev_get_drvdata(&wdev->dev);
+
+	u32 input_val = 0;
+	struct acpi_buffer input = {sizeof(input), &input_val};
+	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
+	acpi_status status;
+	union acpi_object *obj;
+	int code;
+
+	status = wmi_evaluate_method(LENOVO_YMC_QUERY_GUID,
+				LENOVO_YMC_QUERY_INSTANCE,
+				LENOVO_YMC_QUERY_METHOD,
+				&input, &output);
+
+	if (ACPI_FAILURE(status)) {
+		dev_warn(&wdev->dev,
+			"Failed to evaluate query method %ud\n", status);
+		return;
+	}
+
+	obj = (union acpi_object *)output.pointer;
+
+	if (obj->type != ACPI_TYPE_INTEGER) {
+		dev_warn(&wdev->dev,
+			"WMI event data is not an integer\n");
+		goto free_obj;
+	}
+	code = obj->integer.value;
+
+	if (!sparse_keymap_report_event(priv->input_dev, code, 1, true))
+		dev_warn(&wdev->dev, "Unknown key %d pressed\n", code);
+
+free_obj:
+	kfree(obj);
+	lenovo_ymc_trigger_ec();
+}
+
+static void lenovo_ymc_remove(struct wmi_device *wdev)
+{
+	struct lenovo_ymc_private *priv = dev_get_drvdata(&wdev->dev);
+
+	input_unregister_device(priv->input_dev);
+}
+
+static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
+{
+	struct input_dev *input_dev;
+	struct lenovo_ymc_private *priv;
+	int err;
+
+	dmi_check_system(ec_trigger_quirk_dmi_table);
+
+	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	input_dev = input_allocate_device();
+	if (!input_dev) {
+		return -ENOMEM;
+	}
+
+	input_dev->name = "Lenovo Yoga Mode Control switch";
+	input_dev->phys = LENOVO_YMC_EVENT_GUID "/input0";
+	input_dev->id.bustype = BUS_HOST;
+	input_dev->dev.parent = &wdev->dev;
+
+	input_set_capability(input_dev, EV_SW, SW_TABLET_MODE);
+
+	err = sparse_keymap_setup(input_dev, lenovo_ymc_keymap, NULL);
+	if (err) {
+		dev_err(&wdev->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(&wdev->dev,
+			"Could not register input device: %d\n", err);
+		goto err_free_dev;
+	}
+
+	priv->input_dev = input_dev;
+	dev_set_drvdata(&wdev->dev, priv);
+	lenovo_ymc_trigger_ec();
+	return 0;
+
+err_free_dev:
+	input_free_device(input_dev);
+	return err;
+}
+
+static const struct wmi_device_id lenovo_ymc_wmi_id_table[] = {
+	{ .guid_string = LENOVO_YMC_EVENT_GUID },
+	{ }
+};
+MODULE_DEVICE_TABLE(wmi, lenovo_ymc_wmi_id_table);
+
+static struct wmi_driver lenovo_ymc_driver = {
+	.driver = {
+		.name = "lenovo-ymc",
+	},
+	.id_table = lenovo_ymc_wmi_id_table,
+	.probe = lenovo_ymc_probe,
+	.remove = lenovo_ymc_remove,
+	.notify = lenovo_ymc_notify,
+};
+
+module_wmi_driver(lenovo_ymc_driver);
+
+MODULE_AUTHOR("Gergo Koteles <soyer@irl.hu>");
+MODULE_DESCRIPTION("Lenovo Yoga Mode Control driver");
+MODULE_LICENSE("GPL");
-- 
2.37.3

Re: [RFC PATCH] Add Lenovo Yoga Mode Control driver

[Hans de Goede]

Hi Gergo,

On 10/4/22 23:43, Gergo Koteles wrote:
> This is a WMI driver for the Mode Control switch for Lenovo Yoga
> notebooks.
> The mode is mapped to a SW_TABLET_MODE switch capable input device.

Thank you for doing this. This is going to be very helpful for various
people who are having issue with GNOME3 not doing screen autorotation
on yoga style devices when there is no SW_TABLET_MODE reporting.

On a quick scan one thing which stands out it that I believe
that you don't need this, so please drop it:

+	select ACPI_PLATFORM_PROFILE

> It should work with  models like the Yoga C940, Ideapad flex 14API,
> Yoga 9 14IAP7, Yoga 7 14ARB7.
> The 14ARB7 model must trigger the EC to send mode change events.
> This might be a firmware bug.
> 
> Introduces a global variable in the ideapad-laptop driver.
> I would like some advice on how to do it without the variable,
> or how to do it better.

I think it would be cleaner to:

1. Do a new prep commit/patch adding a drivers/platform/x86/ideapad-laptop.h and:
1.1 Move the VPCCMD_* enum there (including adding the command you need)
1.2 Move eval_vpcr(), eval_vpcw(), read_ec_data() and write_ec_cmd() there
as static inline helpers

And then in your driver do

1. #include "ideapad-laptop.h"
2. Add a "struct acpi_device *ec_acpi_dev" to struct lenovo_ymc_private {}
3. On probe do:

	if (quirk) {
		priv->ec_acpi_dev = acpi_dev_get_first_match_dev("VPC2004", NULL, -1);
		if (!priv->ec_acpi_dev) {
			dev_err(...);
			return -ENODEV;
		}
	}

4. When needing to do the write do

	if (priv->ec_acpi_dev)
		write_ec_cmd(priv->ec_acpi_dev->handle, VPCCMD_W_YMC, 1);

This means duplicating a few functions (at compile time) between
the 2 modules, but these will be quite small so I think this will
be fine.

An alternative would be to just shoe-horn this functionality to fit
inside the ideapad-laptop.c driver. But I don't like that idea,
I like the nice clean small separate WMI driver approach you have
gone with a lot better.

I also wonder if maybe the presence, or the _HRV (hardware revision)
field of the VPC2004 device (see DSDT dumps) can be used to check if
we should do the ec-writes or not? But if you cannot find anything
looking like a pattern there just using the DMI quirk approach is fine.

Note if you end up only wanting to do this with a specific _HRV of
VPC2004 then you can just pass that _HRV as third argument to
acpi_dev_get_first_match_dev() and then VPC2004 ACPI devices with
a different _HRV will not be returned.

Regards,

Hans


> ---
>  drivers/platform/x86/Kconfig          |  10 ++
>  drivers/platform/x86/Makefile         |   1 +
>  drivers/platform/x86/ideapad-laptop.c |  18 +++
>  drivers/platform/x86/lenovo-ymc.c     | 185 ++++++++++++++++++++++++++
>  4 files changed, 214 insertions(+)
>  create mode 100644 drivers/platform/x86/lenovo-ymc.c
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index f2f98e942cf2..72ff6713e447 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -510,6 +510,16 @@ config IDEAPAD_LAPTOP
>  	  This is a driver for Lenovo IdeaPad netbooks contains drivers for
>  	  rfkill switch, hotkey, fan control and backlight control.
>  
> +config LENOVO_YMC
> +	tristate "Lenovo Yoga Mode Control"
> +	depends on ACPI_WMI
> +	depends on INPUT
> +	depends on IDEAPAD_LAPTOP
> +	select ACPI_PLATFORM_PROFILE
> +	select INPUT_SPARSEKMAP
> +	help
> +	  This is a driver for the Mode Control switch for Lenovo Yoga notebooks.
> +
>  config SENSORS_HDAPS
>  	tristate "Thinkpad Hard Drive Active Protection System (hdaps)"
>  	depends on INPUT
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 5a428caa654a..f8f0d6a0b43b 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -65,6 +65,7 @@ obj-$(CONFIG_UV_SYSFS)       += uv_sysfs.o
>  # IBM Thinkpad and Lenovo
>  obj-$(CONFIG_IBM_RTL)		+= ibm_rtl.o
>  obj-$(CONFIG_IDEAPAD_LAPTOP)	+= ideapad-laptop.o
> +obj-$(CONFIG_LENOVO_YMC)	+= lenovo-ymc.o
>  obj-$(CONFIG_SENSORS_HDAPS)	+= hdaps.o
>  obj-$(CONFIG_THINKPAD_ACPI)	+= thinkpad_acpi.o
>  obj-$(CONFIG_THINKPAD_LMI)	+= think-lmi.o
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index abd0c81d62c4..d88a4ee72bf6 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -103,6 +103,7 @@ enum {
>  	VPCCMD_W_FAN,
>  	VPCCMD_R_RF,
>  	VPCCMD_W_RF,
> +	VPCCMD_W_YMC = 0x2A,
>  	VPCCMD_R_FAN = 0x2B,
>  	VPCCMD_R_SPECIAL_BUTTONS = 0x31,
>  	VPCCMD_W_BL_POWER = 0x33,
> @@ -156,6 +157,8 @@ 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.");
>  
> +static struct ideapad_private *ideapad_priv;
> +
>  /*
>   * ACPI Helpers
>   */
> @@ -1421,6 +1424,19 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv)
>  	}
>  }
>  
> +void ideapad_trigger_ymc_next_read(void)
> +{
> +	int err;
> +	if (ideapad_priv != NULL) {
> +		err = write_ec_cmd(ideapad_priv->adev->handle,
> +			VPCCMD_W_YMC, 1);
> +		if (err)
> +			dev_warn(&ideapad_priv->platform_device->dev,
> +				"Could not write YMC: %d\n", err);
> +	}
> +}
> +EXPORT_SYMBOL(ideapad_trigger_ymc_next_read);
> +
>  static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
>  {
>  	struct ideapad_private *priv = data;
> @@ -1663,6 +1679,7 @@ static int ideapad_acpi_add(struct platform_device *pdev)
>  	}
>  #endif
>  
> +	ideapad_priv = priv;
>  	return 0;
>  
>  #if IS_ENABLED(CONFIG_ACPI_WMI)
> @@ -1696,6 +1713,7 @@ static int ideapad_acpi_remove(struct platform_device *pdev)
>  	struct ideapad_private *priv = dev_get_drvdata(&pdev->dev);
>  	int i;
>  
> +	ideapad_priv = NULL;
>  #if IS_ENABLED(CONFIG_ACPI_WMI)
>  	if (priv->fnesc_guid)
>  		wmi_remove_notify_handler(priv->fnesc_guid);
> diff --git a/drivers/platform/x86/lenovo-ymc.c b/drivers/platform/x86/lenovo-ymc.c
> new file mode 100644
> index 000000000000..0b899b02e12f
> --- /dev/null
> +++ b/drivers/platform/x86/lenovo-ymc.c
> @@ -0,0 +1,185 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * lenovo-ymc.c - Lenovo Yoga Mode Control driver
> + *
> + * Copyright © 2022 Gergo Koteles <soyer@irl.hu>
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/dmi.h>
> +#include <linux/input.h>
> +#include <linux/input/sparse-keymap.h>
> +#include <linux/wmi.h>
> +
> +#define LENOVO_YMC_EVENT_GUID	"06129D99-6083-4164-81AD-F092F9D773A6"
> +#define LENOVO_YMC_QUERY_GUID	"09B0EE6E-C3FD-4243-8DA1-7911FF80BB8C"
> +
> +#define LENOVO_YMC_QUERY_INSTANCE 0
> +#define LENOVO_YMC_QUERY_METHOD 0xAB
> +
> +extern void ideapad_trigger_ymc_next_read(void);
> +
> +static bool ec_trigger __read_mostly;
> +module_param(ec_trigger, bool, 0644);
> +MODULE_PARM_DESC(ec_trigger, "Enable EC triggering to emit YMC events");
> +
> +static int enable_ec_trigger(const struct dmi_system_id *id)
> +{
> +       pr_debug("Lenovo YMC enable EC triggering.\n");
> +       ec_trigger = true;
> +       return 0;
> +}
> +
> +static const struct dmi_system_id ec_trigger_quirk_dmi_table[] = {
> +	{
> +		// Lenovo Yoga 7 14ARB7
> +		.callback = enable_ec_trigger,
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "82QF"),
> +		},
> +	},
> +	{ },
> +};
> +
> +static void lenovo_ymc_trigger_ec(void) {
> +	if (ec_trigger) {
> +		ideapad_trigger_ymc_next_read();
> +	}
> +}
> +
> +
> +struct lenovo_ymc_private {
> +	struct input_dev *input_dev;
> +};
> +
> +
> +static const struct key_entry lenovo_ymc_keymap[] = {
> +	// Laptop
> +	{ KE_SW, 0x01, { .sw = { SW_TABLET_MODE, 0 } } },
> +	// Tablet
> +	{ KE_SW, 0x02, { .sw = { SW_TABLET_MODE, 1 } } },
> +	// Drawing Board
> +	{ KE_SW, 0x03, { .sw = { SW_TABLET_MODE, 1 } } },
> +	// Tent
> +	{ KE_SW, 0x04, { .sw = { SW_TABLET_MODE, 1 } } },
> +	{ KE_END },
> +};
> +
> +static void lenovo_ymc_notify(struct wmi_device *wdev, union acpi_object *data)
> +{
> +	struct lenovo_ymc_private *priv = dev_get_drvdata(&wdev->dev);
> +
> +	u32 input_val = 0;
> +	struct acpi_buffer input = {sizeof(input), &input_val};
> +	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
> +	acpi_status status;
> +	union acpi_object *obj;
> +	int code;
> +
> +	status = wmi_evaluate_method(LENOVO_YMC_QUERY_GUID,
> +				LENOVO_YMC_QUERY_INSTANCE,
> +				LENOVO_YMC_QUERY_METHOD,
> +				&input, &output);
> +
> +	if (ACPI_FAILURE(status)) {
> +		dev_warn(&wdev->dev,
> +			"Failed to evaluate query method %ud\n", status);
> +		return;
> +	}
> +
> +	obj = (union acpi_object *)output.pointer;
> +
> +	if (obj->type != ACPI_TYPE_INTEGER) {
> +		dev_warn(&wdev->dev,
> +			"WMI event data is not an integer\n");
> +		goto free_obj;
> +	}
> +	code = obj->integer.value;
> +
> +	if (!sparse_keymap_report_event(priv->input_dev, code, 1, true))
> +		dev_warn(&wdev->dev, "Unknown key %d pressed\n", code);
> +
> +free_obj:
> +	kfree(obj);
> +	lenovo_ymc_trigger_ec();
> +}
> +
> +static void lenovo_ymc_remove(struct wmi_device *wdev)
> +{
> +	struct lenovo_ymc_private *priv = dev_get_drvdata(&wdev->dev);
> +
> +	input_unregister_device(priv->input_dev);
> +}
> +
> +static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
> +{
> +	struct input_dev *input_dev;
> +	struct lenovo_ymc_private *priv;
> +	int err;
> +
> +	dmi_check_system(ec_trigger_quirk_dmi_table);
> +
> +	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	input_dev = input_allocate_device();
> +	if (!input_dev) {
> +		return -ENOMEM;
> +	}
> +
> +	input_dev->name = "Lenovo Yoga Mode Control switch";
> +	input_dev->phys = LENOVO_YMC_EVENT_GUID "/input0";
> +	input_dev->id.bustype = BUS_HOST;
> +	input_dev->dev.parent = &wdev->dev;
> +
> +	input_set_capability(input_dev, EV_SW, SW_TABLET_MODE);
> +
> +	err = sparse_keymap_setup(input_dev, lenovo_ymc_keymap, NULL);
> +	if (err) {
> +		dev_err(&wdev->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(&wdev->dev,
> +			"Could not register input device: %d\n", err);
> +		goto err_free_dev;
> +	}
> +
> +	priv->input_dev = input_dev;
> +	dev_set_drvdata(&wdev->dev, priv);
> +	lenovo_ymc_trigger_ec();
> +	return 0;
> +
> +err_free_dev:
> +	input_free_device(input_dev);
> +	return err;
> +}
> +
> +static const struct wmi_device_id lenovo_ymc_wmi_id_table[] = {
> +	{ .guid_string = LENOVO_YMC_EVENT_GUID },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(wmi, lenovo_ymc_wmi_id_table);
> +
> +static struct wmi_driver lenovo_ymc_driver = {
> +	.driver = {
> +		.name = "lenovo-ymc",
> +	},
> +	.id_table = lenovo_ymc_wmi_id_table,
> +	.probe = lenovo_ymc_probe,
> +	.remove = lenovo_ymc_remove,
> +	.notify = lenovo_ymc_notify,
> +};
> +
> +module_wmi_driver(lenovo_ymc_driver);
> +
> +MODULE_AUTHOR("Gergo Koteles <soyer@irl.hu>");
> +MODULE_DESCRIPTION("Lenovo Yoga Mode Control driver");
> +MODULE_LICENSE("GPL");

Re: [RFC PATCH] Add Lenovo Yoga Mode Control driver

[Barnabás Pőcze]

Hi


2022. október 4., kedd 23:43 keltezéssel, Gergo Koteles <soyer@irl.hu> írta:

> This is a WMI driver for the Mode Control switch for Lenovo Yoga
> notebooks.
> The mode is mapped to a SW_TABLET_MODE switch capable input device.
> 
> It should work with  models like the Yoga C940, Ideapad flex 14API,
> Yoga 9 14IAP7, Yoga 7 14ARB7.
> The 14ARB7 model must trigger the EC to send mode change events.
> This might be a firmware bug.

Is it known how that works on Windows?


> 
> Introduces a global variable in the ideapad-laptop driver.
> I would like some advice on how to do it without the variable,
> or how to do it better.
> ---
>  drivers/platform/x86/Kconfig          |  10 ++
>  drivers/platform/x86/Makefile         |   1 +
>  drivers/platform/x86/ideapad-laptop.c |  18 +++
>  drivers/platform/x86/lenovo-ymc.c     | 185 ++++++++++++++++++++++++++
>  4 files changed, 214 insertions(+)
>  create mode 100644 drivers/platform/x86/lenovo-ymc.c
> [...]
> diff --git a/drivers/platform/x86/lenovo-ymc.c b/drivers/platform/x86/lenovo-ymc.c
> new file mode 100644
> index 000000000000..0b899b02e12f
> --- /dev/null
> +++ b/drivers/platform/x86/lenovo-ymc.c
> @@ -0,0 +1,185 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * lenovo-ymc.c - Lenovo Yoga Mode Control driver
> + *
> + * Copyright © 2022 Gergo Koteles <soyer@irl.hu>
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/dmi.h>
> +#include <linux/input.h>
> +#include <linux/input/sparse-keymap.h>
> +#include <linux/wmi.h>
> +
> +#define LENOVO_YMC_EVENT_GUID	"06129D99-6083-4164-81AD-F092F9D773A6"
> +#define LENOVO_YMC_QUERY_GUID	"09B0EE6E-C3FD-4243-8DA1-7911FF80BB8C"
> +
> +#define LENOVO_YMC_QUERY_INSTANCE 0
> +#define LENOVO_YMC_QUERY_METHOD 0xAB
> +
> +extern void ideapad_trigger_ymc_next_read(void);
> +
> +static bool ec_trigger __read_mostly;
> +module_param(ec_trigger, bool, 0644);
> +MODULE_PARM_DESC(ec_trigger, "Enable EC triggering to emit YMC events");
> +
> +static int enable_ec_trigger(const struct dmi_system_id *id)
> +{
> +       pr_debug("Lenovo YMC enable EC triggering.\n");
> +       ec_trigger = true;
> +       return 0;

You seem to be using spaces instead of tabs here. Please run scripts/checkpatch.pl.
The biggest problem currently is probably the missing Signed-off-by tag.

In any case, I think you don't actually need this function because you can do

  ec_trigger |= dmi_check_system(ec_trigger_quirk_dmi_table);

in the probe function.


> +}
> +
> +static const struct dmi_system_id ec_trigger_quirk_dmi_table[] = {
> +	{
> +		// Lenovo Yoga 7 14ARB7
> +		.callback = enable_ec_trigger,
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "82QF"),
> +		},
> +	},
> +	{ },
> +};
> +
> +static void lenovo_ymc_trigger_ec(void) {
> +	if (ec_trigger) {
> +		ideapad_trigger_ymc_next_read();
> +	}
> +}
> +
> +
> +struct lenovo_ymc_private {
> +	struct input_dev *input_dev;
> +};
> +
> +
> +static const struct key_entry lenovo_ymc_keymap[] = {
> +	// Laptop
> +	{ KE_SW, 0x01, { .sw = { SW_TABLET_MODE, 0 } } },
> +	// Tablet
> +	{ KE_SW, 0x02, { .sw = { SW_TABLET_MODE, 1 } } },
> +	// Drawing Board
> +	{ KE_SW, 0x03, { .sw = { SW_TABLET_MODE, 1 } } },
> +	// Tent
> +	{ KE_SW, 0x04, { .sw = { SW_TABLET_MODE, 1 } } },
> +	{ KE_END },
> +};
> +
> +static void lenovo_ymc_notify(struct wmi_device *wdev, union acpi_object *data)
> +{
> +	struct lenovo_ymc_private *priv = dev_get_drvdata(&wdev->dev);
> +
> +	u32 input_val = 0;
> +	struct acpi_buffer input = {sizeof(input), &input_val};

Shouldn't it be `sizeof(input_val)`?


> +	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
> +	acpi_status status;
> +	union acpi_object *obj;
> +	int code;
> +
> +	status = wmi_evaluate_method(LENOVO_YMC_QUERY_GUID,
> +				LENOVO_YMC_QUERY_INSTANCE,
> +				LENOVO_YMC_QUERY_METHOD,
> +				&input, &output);
> +
> +	if (ACPI_FAILURE(status)) {
> +		dev_warn(&wdev->dev,
> +			"Failed to evaluate query method %ud\n", status);

I believe it should be "%u", or maybe even better:
"%s" and `acpi_format_exception(status)`.


> +		return;
> +	}
> +
> +	obj = (union acpi_object *)output.pointer;

Small thing, but this cast is not strictly necessary.


> +
> +	if (obj->type != ACPI_TYPE_INTEGER) {
> +		dev_warn(&wdev->dev,
> +			"WMI event data is not an integer\n");
> +		goto free_obj;
> +	}
> +	code = obj->integer.value;
> +
> +	if (!sparse_keymap_report_event(priv->input_dev, code, 1, true))
> +		dev_warn(&wdev->dev, "Unknown key %d pressed\n", code);
> +
> +free_obj:
> +	kfree(obj);
> +	lenovo_ymc_trigger_ec();
> +}
> +
> +static void lenovo_ymc_remove(struct wmi_device *wdev)
> +{
> +	struct lenovo_ymc_private *priv = dev_get_drvdata(&wdev->dev);
> +
> +	input_unregister_device(priv->input_dev);
> +}
> +
> +static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
> +{
> +	struct input_dev *input_dev;
> +	struct lenovo_ymc_private *priv;
> +	int err;
> +
> +	dmi_check_system(ec_trigger_quirk_dmi_table);
> +
> +	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	input_dev = input_allocate_device();

Is there any reason why you're not using `devm_input_allocate_device()` here?
Then you could get rid of `lenovo_ymc_remove()` and the `err_free_dev` label.


> +	if (!input_dev) {
> +		return -ENOMEM;
> +	}
> +
> +	input_dev->name = "Lenovo Yoga Mode Control switch";
> +	input_dev->phys = LENOVO_YMC_EVENT_GUID "/input0";
> +	input_dev->id.bustype = BUS_HOST;
> +	input_dev->dev.parent = &wdev->dev;
> +
> +	input_set_capability(input_dev, EV_SW, SW_TABLET_MODE);
> +
> +	err = sparse_keymap_setup(input_dev, lenovo_ymc_keymap, NULL);
> +	if (err) {
> +		dev_err(&wdev->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(&wdev->dev,
> +			"Could not register input device: %d\n", err);
> +		goto err_free_dev;
> +	}
> +
> +	priv->input_dev = input_dev;
> +	dev_set_drvdata(&wdev->dev, priv);
> +	lenovo_ymc_trigger_ec();
> +	return 0;
> +
> +err_free_dev:
> +	input_free_device(input_dev);
> +	return err;
> +}
> +
> +static const struct wmi_device_id lenovo_ymc_wmi_id_table[] = {
> +	{ .guid_string = LENOVO_YMC_EVENT_GUID },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(wmi, lenovo_ymc_wmi_id_table);
> +
> +static struct wmi_driver lenovo_ymc_driver = {
> +	.driver = {
> +		.name = "lenovo-ymc",
> +	},
> +	.id_table = lenovo_ymc_wmi_id_table,
> +	.probe = lenovo_ymc_probe,
> +	.remove = lenovo_ymc_remove,
> +	.notify = lenovo_ymc_notify,
> +};
> +
> +module_wmi_driver(lenovo_ymc_driver);
> +
> +MODULE_AUTHOR("Gergo Koteles <soyer@irl.hu>");
> +MODULE_DESCRIPTION("Lenovo Yoga Mode Control driver");
> +MODULE_LICENSE("GPL");
> --
> 2.37.3


Regards,
Barnabás Pőcze

[PATCH 0/2] platform/x86: Add driver for Yoga Tablet mode switch

[Andrew Kallmeyer]

This driver maps the Lenovo Yoga tablet mode switch to a SW_TABLET_MODE input
device. This will make the tablet status available to desktop environments.

This patch series is the result of the discussion at
https://lore.kernel.org/r/CAG4kvq9US=-NjyXFMzJYu2zCJryJWtOc7FGZbrewpgCDjdAkbg@mail.gmail.com/

I decided to follow-up on the patch Gergo wrote and respond to the review
comments to get it merged and available for everyone.

As Gergo said: It should work with  models like the Yoga C940, Ideapad
flex 14API, Yoga 9 14IAP7, Yoga 7 14ARB7.
The 14ARB7 model must trigger the EC to send mode change events. This
might be a firmware bug.

I have additionally tested this on the Yoga 7 14IAL7.

Andrew Kallmeyer (1):
  platform/x86: Move ideapad ACPI helpers to a new header

Gergo Koteles (1):
  platform/x86: Add driver for Yoga Tablet Mode switch

 drivers/platform/x86/Kconfig          |  10 ++
 drivers/platform/x86/Makefile         |   1 +
 drivers/platform/x86/ideapad-laptop.c | 135 +-------------------
 drivers/platform/x86/ideapad-laptop.h | 152 ++++++++++++++++++++++
 drivers/platform/x86/lenovo-ymc.c     | 177 ++++++++++++++++++++++++++
 5 files changed, 341 insertions(+), 134 deletions(-)
 create mode 100644 drivers/platform/x86/ideapad-laptop.h
 create mode 100644 drivers/platform/x86/lenovo-ymc.c

-- 
2.39.2

[PATCH 1/2] platform/x86: Move ideapad ACPI helpers to a new header

[Andrew Kallmeyer]

These functions will be used by a driver written by Gergo Koteles to
detect the tablet mode switch in Lenovo Yoga laptops. These changes were
discussed in review of that patch.

This is the minimal set of functions needed in that driver, there are
several more small functions left in the ACPI Helpers section in
ideapad-laptop.c. The only change is the functions are now marked inline
as requested in the review comments.

Link: https://lore.kernel.org/r/20221004214332.35934-1-soyer@irl.hu/
Signed-off-by: Andrew Kallmeyer <kallmeyeras@gmail.com>
---
 drivers/platform/x86/ideapad-laptop.c | 135 +----------------------
 drivers/platform/x86/ideapad-laptop.h | 151 ++++++++++++++++++++++++++
 2 files changed, 152 insertions(+), 134 deletions(-)
 create mode 100644 drivers/platform/x86/ideapad-laptop.h

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 0eb5bfdd8..55f56697e 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -20,7 +20,6 @@
 #include <linux/init.h>
 #include <linux/input.h>
 #include <linux/input/sparse-keymap.h>
-#include <linux/jiffies.h>
 #include <linux/kernel.h>
 #include <linux/leds.h>
 #include <linux/module.h>
@@ -31,6 +30,7 @@
 #include <linux/sysfs.h>
 #include <linux/types.h>
 #include <linux/wmi.h>
+#include "ideapad-laptop.h"
 
 #include <acpi/video.h>
 
@@ -85,33 +85,6 @@ enum {
 	SALS_FNLOCK_OFF       = 0xf,
 };
 
-enum {
-	VPCCMD_R_VPC1 = 0x10,
-	VPCCMD_R_BL_MAX,
-	VPCCMD_R_BL,
-	VPCCMD_W_BL,
-	VPCCMD_R_WIFI,
-	VPCCMD_W_WIFI,
-	VPCCMD_R_BT,
-	VPCCMD_W_BT,
-	VPCCMD_R_BL_POWER,
-	VPCCMD_R_NOVO,
-	VPCCMD_R_VPC2,
-	VPCCMD_R_TOUCHPAD,
-	VPCCMD_W_TOUCHPAD,
-	VPCCMD_R_CAMERA,
-	VPCCMD_W_CAMERA,
-	VPCCMD_R_3G,
-	VPCCMD_W_3G,
-	VPCCMD_R_ODD, /* 0x21 */
-	VPCCMD_W_FAN,
-	VPCCMD_R_RF,
-	VPCCMD_W_RF,
-	VPCCMD_R_FAN = 0x2B,
-	VPCCMD_R_SPECIAL_BUTTONS = 0x31,
-	VPCCMD_W_BL_POWER = 0x33,
-};
-
 struct ideapad_dytc_priv {
 	enum platform_profile_option current_profile;
 	struct platform_profile_handler pprof;
@@ -227,7 +200,6 @@ static void ideapad_shared_exit(struct ideapad_private *priv)
 /*
  * ACPI Helpers
  */
-#define IDEAPAD_EC_TIMEOUT 200 /* in ms */
 
 static int eval_int(acpi_handle handle, const char *name, unsigned long *res)
 {
@@ -270,116 +242,11 @@ static int exec_sals(acpi_handle handle, unsigned long arg)
 	return exec_simple_method(handle, "SALS", arg);
 }
 
-static int eval_int_with_arg(acpi_handle handle, const char *name, unsigned long arg, unsigned long *res)
-{
-	struct acpi_object_list params;
-	unsigned long long result;
-	union acpi_object in_obj;
-	acpi_status status;
-
-	params.count = 1;
-	params.pointer = &in_obj;
-	in_obj.type = ACPI_TYPE_INTEGER;
-	in_obj.integer.value = arg;
-
-	status = acpi_evaluate_integer(handle, (char *)name, &params, &result);
-	if (ACPI_FAILURE(status))
-		return -EIO;
-
-	if (res)
-		*res = result;
-
-	return 0;
-}
-
 static int eval_dytc(acpi_handle handle, unsigned long cmd, unsigned long *res)
 {
 	return eval_int_with_arg(handle, "DYTC", cmd, res);
 }
 
-static int eval_vpcr(acpi_handle handle, unsigned long cmd, unsigned long *res)
-{
-	return eval_int_with_arg(handle, "VPCR", cmd, res);
-}
-
-static int eval_vpcw(acpi_handle handle, unsigned long cmd, unsigned long data)
-{
-	struct acpi_object_list params;
-	union acpi_object in_obj[2];
-	acpi_status status;
-
-	params.count = 2;
-	params.pointer = in_obj;
-	in_obj[0].type = ACPI_TYPE_INTEGER;
-	in_obj[0].integer.value = cmd;
-	in_obj[1].type = ACPI_TYPE_INTEGER;
-	in_obj[1].integer.value = data;
-
-	status = acpi_evaluate_object(handle, "VPCW", &params, NULL);
-	if (ACPI_FAILURE(status))
-		return -EIO;
-
-	return 0;
-}
-
-static int read_ec_data(acpi_handle handle, unsigned long cmd, unsigned long *data)
-{
-	unsigned long end_jiffies, val;
-	int err;
-
-	err = eval_vpcw(handle, 1, cmd);
-	if (err)
-		return err;
-
-	end_jiffies = jiffies + msecs_to_jiffies(IDEAPAD_EC_TIMEOUT) + 1;
-
-	while (time_before(jiffies, end_jiffies)) {
-		schedule();
-
-		err = eval_vpcr(handle, 1, &val);
-		if (err)
-			return err;
-
-		if (val == 0)
-			return eval_vpcr(handle, 0, data);
-	}
-
-	acpi_handle_err(handle, "timeout in %s\n", __func__);
-
-	return -ETIMEDOUT;
-}
-
-static int write_ec_cmd(acpi_handle handle, unsigned long cmd, unsigned long data)
-{
-	unsigned long end_jiffies, val;
-	int err;
-
-	err = eval_vpcw(handle, 0, data);
-	if (err)
-		return err;
-
-	err = eval_vpcw(handle, 1, cmd);
-	if (err)
-		return err;
-
-	end_jiffies = jiffies + msecs_to_jiffies(IDEAPAD_EC_TIMEOUT) + 1;
-
-	while (time_before(jiffies, end_jiffies)) {
-		schedule();
-
-		err = eval_vpcr(handle, 1, &val);
-		if (err)
-			return err;
-
-		if (val == 0)
-			return 0;
-	}
-
-	acpi_handle_err(handle, "timeout in %s\n", __func__);
-
-	return -ETIMEDOUT;
-}
-
 /*
  * debugfs
  */
diff --git a/drivers/platform/x86/ideapad-laptop.h b/drivers/platform/x86/ideapad-laptop.h
new file mode 100644
index 000000000..7dd8ce027
--- /dev/null
+++ b/drivers/platform/x86/ideapad-laptop.h
@@ -0,0 +1,151 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *  ideapad-laptop.h - Lenovo IdeaPad ACPI Extras
+ *
+ *  Copyright © 2010 Intel Corporation
+ *  Copyright © 2010 David Woodhouse <dwmw2@infradead.org>
+ */
+
+#ifndef _IDEAPAD_LAPTOP_H_
+#define _IDEAPAD_LAPTOP_H_
+
+#include <linux/acpi.h>
+#include <linux/jiffies.h>
+#include <linux/errno.h>
+
+enum {
+	VPCCMD_R_VPC1 = 0x10,
+	VPCCMD_R_BL_MAX,
+	VPCCMD_R_BL,
+	VPCCMD_W_BL,
+	VPCCMD_R_WIFI,
+	VPCCMD_W_WIFI,
+	VPCCMD_R_BT,
+	VPCCMD_W_BT,
+	VPCCMD_R_BL_POWER,
+	VPCCMD_R_NOVO,
+	VPCCMD_R_VPC2,
+	VPCCMD_R_TOUCHPAD,
+	VPCCMD_W_TOUCHPAD,
+	VPCCMD_R_CAMERA,
+	VPCCMD_W_CAMERA,
+	VPCCMD_R_3G,
+	VPCCMD_W_3G,
+	VPCCMD_R_ODD, /* 0x21 */
+	VPCCMD_W_FAN,
+	VPCCMD_R_RF,
+	VPCCMD_W_RF,
+	VPCCMD_R_FAN = 0x2B,
+	VPCCMD_R_SPECIAL_BUTTONS = 0x31,
+	VPCCMD_W_BL_POWER = 0x33,
+};
+
+static inline int eval_int_with_arg(acpi_handle handle, const char *name, unsigned long arg, unsigned long *res)
+{
+	struct acpi_object_list params;
+	unsigned long long result;
+	union acpi_object in_obj;
+	acpi_status status;
+
+	params.count = 1;
+	params.pointer = &in_obj;
+	in_obj.type = ACPI_TYPE_INTEGER;
+	in_obj.integer.value = arg;
+
+	status = acpi_evaluate_integer(handle, (char *)name, &params, &result);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	if (res)
+		*res = result;
+
+	return 0;
+}
+
+static inline int eval_vpcr(acpi_handle handle, unsigned long cmd, unsigned long *res)
+{
+	return eval_int_with_arg(handle, "VPCR", cmd, res);
+}
+
+static inline int eval_vpcw(acpi_handle handle, unsigned long cmd, unsigned long data)
+{
+	struct acpi_object_list params;
+	union acpi_object in_obj[2];
+	acpi_status status;
+
+	params.count = 2;
+	params.pointer = in_obj;
+	in_obj[0].type = ACPI_TYPE_INTEGER;
+	in_obj[0].integer.value = cmd;
+	in_obj[1].type = ACPI_TYPE_INTEGER;
+	in_obj[1].integer.value = data;
+
+	status = acpi_evaluate_object(handle, "VPCW", &params, NULL);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+
+	return 0;
+}
+
+#define IDEAPAD_EC_TIMEOUT 200 /* in ms */
+
+static inline int read_ec_data(acpi_handle handle, unsigned long cmd, unsigned long *data)
+{
+	unsigned long end_jiffies, val;
+	int err;
+
+	err = eval_vpcw(handle, 1, cmd);
+	if (err)
+		return err;
+
+	end_jiffies = jiffies + msecs_to_jiffies(IDEAPAD_EC_TIMEOUT) + 1;
+
+	while (time_before(jiffies, end_jiffies)) {
+		schedule();
+
+		err = eval_vpcr(handle, 1, &val);
+		if (err)
+			return err;
+
+		if (val == 0)
+			return eval_vpcr(handle, 0, data);
+	}
+
+	acpi_handle_err(handle, "timeout in %s\n", __func__);
+
+	return -ETIMEDOUT;
+}
+
+static inline int write_ec_cmd(acpi_handle handle, unsigned long cmd, unsigned long data)
+{
+	unsigned long end_jiffies, val;
+	int err;
+
+	err = eval_vpcw(handle, 0, data);
+	if (err)
+		return err;
+
+	err = eval_vpcw(handle, 1, cmd);
+	if (err)
+		return err;
+
+	end_jiffies = jiffies + msecs_to_jiffies(IDEAPAD_EC_TIMEOUT) + 1;
+
+	while (time_before(jiffies, end_jiffies)) {
+		schedule();
+
+		err = eval_vpcr(handle, 1, &val);
+		if (err)
+			return err;
+
+		if (val == 0)
+			return 0;
+	}
+
+	acpi_handle_err(handle, "timeout in %s\n", __func__);
+
+	return -ETIMEDOUT;
+}
+
+#undef IDEAPAD_EC_TIMEOUT
+#endif /* !_IDEAPAD_LAPTOP_H_ */
-- 
2.39.2

[PATCH 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Andrew Kallmeyer]

This WMI driver for the tablet mode control switch for Lenovo Yoga
notebooks was originally written by Gergo Koteles. The mode is mapped to
a SW_TABLET_MODE switch capable input device.

I (Andrew) followed the suggestions that were posted in reply to Gergo's
RFC patch to follow-up and get it merged. Additionally I merged the
lenovo_ymc_trigger_ec function with the ideapad_trigger_ymc_next_read
function since it was no longer external. I have also added the word
"Tablet" to the driver description to hopefully make it more clear.

As Gergo said: It should work with  models like the Yoga C940, Ideapad
flex 14API, Yoga 9 14IAP7, Yoga 7 14ARB7.
The 14ARB7 model must trigger the EC to send mode change events. This
might be a firmware bug.

I have additionally tested this on the Yoga 7 14IAL7.

Link: https://lore.kernel.org/r/20221004214332.35934-1-soyer@irl.hu/
Co-developed-by: Andrew Kallmeyer <kallmeyeras@gmail.com>
Signed-off-by: Andrew Kallmeyer <kallmeyeras@gmail.com>
---
 drivers/platform/x86/Kconfig          |  10 ++
 drivers/platform/x86/Makefile         |   1 +
 drivers/platform/x86/ideapad-laptop.h |   1 +
 drivers/platform/x86/lenovo-ymc.c     | 177 ++++++++++++++++++++++++++
 4 files changed, 189 insertions(+)
 create mode 100644 drivers/platform/x86/lenovo-ymc.c

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 5692385e2..858be0c65 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -470,6 +470,16 @@ config IDEAPAD_LAPTOP
 	  This is a driver for Lenovo IdeaPad netbooks contains drivers for
 	  rfkill switch, hotkey, fan control and backlight control.
 
+config LENOVO_YMC
+	tristate "Lenovo Yoga Tablet Mode Control"
+	depends on ACPI_WMI
+	depends on INPUT
+	depends on IDEAPAD_LAPTOP
+	select INPUT_SPARSEKMAP
+	help
+	  This driver maps the Tablet Mode Control switch to SW_TABLET_MODE input
+	  events for Lenovo Yoga notebooks.
+
 config SENSORS_HDAPS
 	tristate "Thinkpad Hard Drive Active Protection System (hdaps)"
 	depends on INPUT
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 1d3d1b025..10054cdea 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -63,6 +63,7 @@ obj-$(CONFIG_UV_SYSFS)       += uv_sysfs.o
 # IBM Thinkpad and Lenovo
 obj-$(CONFIG_IBM_RTL)		+= ibm_rtl.o
 obj-$(CONFIG_IDEAPAD_LAPTOP)	+= ideapad-laptop.o
+obj-$(CONFIG_LENOVO_YMC)	+= lenovo-ymc.o
 obj-$(CONFIG_SENSORS_HDAPS)	+= hdaps.o
 obj-$(CONFIG_THINKPAD_ACPI)	+= thinkpad_acpi.o
 obj-$(CONFIG_THINKPAD_LMI)	+= think-lmi.o
diff --git a/drivers/platform/x86/ideapad-laptop.h b/drivers/platform/x86/ideapad-laptop.h
index 7dd8ce027..2564cb1cd 100644
--- a/drivers/platform/x86/ideapad-laptop.h
+++ b/drivers/platform/x86/ideapad-laptop.h
@@ -35,6 +35,7 @@ enum {
 	VPCCMD_W_FAN,
 	VPCCMD_R_RF,
 	VPCCMD_W_RF,
+	VPCCMD_W_YMC = 0x2A,
 	VPCCMD_R_FAN = 0x2B,
 	VPCCMD_R_SPECIAL_BUTTONS = 0x31,
 	VPCCMD_W_BL_POWER = 0x33,
diff --git a/drivers/platform/x86/lenovo-ymc.c b/drivers/platform/x86/lenovo-ymc.c
new file mode 100644
index 000000000..e29ada1b4
--- /dev/null
+++ b/drivers/platform/x86/lenovo-ymc.c
@@ -0,0 +1,177 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * lenovo-ymc.c - Lenovo Yoga Mode Control driver
+ *
+ * Copyright © 2022 Gergo Koteles <soyer@irl.hu>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/acpi.h>
+#include <linux/dmi.h>
+#include <linux/input.h>
+#include <linux/input/sparse-keymap.h>
+#include <linux/wmi.h>
+#include "ideapad-laptop.h"
+
+#define LENOVO_YMC_EVENT_GUID	"06129D99-6083-4164-81AD-F092F9D773A6"
+#define LENOVO_YMC_QUERY_GUID	"09B0EE6E-C3FD-4243-8DA1-7911FF80BB8C"
+
+#define LENOVO_YMC_QUERY_INSTANCE 0
+#define LENOVO_YMC_QUERY_METHOD 0xAB
+
+static bool ec_trigger __read_mostly;
+module_param(ec_trigger, bool, 0644);
+MODULE_PARM_DESC(ec_trigger, "Enable EC triggering to emit YMC events");
+
+static const struct dmi_system_id ec_trigger_quirk_dmi_table[] = {
+	{
+		// Lenovo Yoga 7 14ARB7
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "82QF"),
+		},
+	},
+	{ },
+};
+
+struct lenovo_ymc_private {
+	struct input_dev *input_dev;
+	struct acpi_device *ec_acpi_dev;
+};
+
+static void lenovo_ymc_trigger_ec(struct wmi_device *wdev, struct lenovo_ymc_private *priv)
+{
+	int err;
+
+	if (!ec_trigger || !priv || !priv->ec_acpi_dev)
+		return;
+	err = write_ec_cmd(priv->ec_acpi_dev->handle, VPCCMD_W_YMC, 1);
+	if (err)
+		dev_warn(&wdev->dev, "Could not write YMC: %d\n", err);
+}
+
+static const struct key_entry lenovo_ymc_keymap[] = {
+	// Laptop
+	{ KE_SW, 0x01, { .sw = { SW_TABLET_MODE, 0 } } },
+	// Tablet
+	{ KE_SW, 0x02, { .sw = { SW_TABLET_MODE, 1 } } },
+	// Drawing Board
+	{ KE_SW, 0x03, { .sw = { SW_TABLET_MODE, 1 } } },
+	// Tent
+	{ KE_SW, 0x04, { .sw = { SW_TABLET_MODE, 1 } } },
+	{ KE_END },
+};
+
+static void lenovo_ymc_notify(struct wmi_device *wdev, union acpi_object *data)
+{
+	struct lenovo_ymc_private *priv = dev_get_drvdata(&wdev->dev);
+
+	u32 input_val = 0;
+	struct acpi_buffer input = {sizeof(input_val), &input_val};
+	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
+	acpi_status status;
+	union acpi_object *obj;
+	int code;
+
+	status = wmi_evaluate_method(LENOVO_YMC_QUERY_GUID,
+				LENOVO_YMC_QUERY_INSTANCE,
+				LENOVO_YMC_QUERY_METHOD,
+				&input, &output);
+
+	if (ACPI_FAILURE(status)) {
+		dev_warn(&wdev->dev,
+			"Failed to evaluate query method: %s\n",
+			acpi_format_exception(status));
+		return;
+	}
+
+	obj = output.pointer;
+
+	if (obj->type != ACPI_TYPE_INTEGER) {
+		dev_warn(&wdev->dev,
+			"WMI event data is not an integer\n");
+		goto free_obj;
+	}
+	code = obj->integer.value;
+
+	if (!sparse_keymap_report_event(priv->input_dev, code, 1, true))
+		dev_warn(&wdev->dev, "Unknown key %d pressed\n", code);
+
+free_obj:
+	kfree(obj);
+	lenovo_ymc_trigger_ec(wdev, priv);
+}
+
+static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
+{
+	struct input_dev *input_dev;
+	struct lenovo_ymc_private *priv;
+	int err;
+
+	ec_trigger |= dmi_check_system(ec_trigger_quirk_dmi_table);
+
+	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	if (ec_trigger) {
+		pr_debug("Lenovo YMC enable EC triggering.\n");
+		priv->ec_acpi_dev = acpi_dev_get_first_match_dev("VCP2004", NULL, -1);
+		if (!priv->ec_acpi_dev) {
+			dev_err(&wdev->dev, "Could not find EC ACPI device.\n");
+			return -ENODEV;
+		}
+	}
+
+	input_dev = devm_input_allocate_device(&wdev->dev);
+	if (!input_dev)
+		return -ENOMEM;
+
+	input_dev->name = "Lenovo Yoga Tablet Mode Control switch";
+	input_dev->phys = LENOVO_YMC_EVENT_GUID "/input0";
+	input_dev->id.bustype = BUS_HOST;
+	input_dev->dev.parent = &wdev->dev;
+
+	input_set_capability(input_dev, EV_SW, SW_TABLET_MODE);
+
+	err = sparse_keymap_setup(input_dev, lenovo_ymc_keymap, NULL);
+	if (err) {
+		dev_err(&wdev->dev,
+			"Could not set up input device keymap: %d\n", err);
+		return err;
+	}
+
+	err = input_register_device(input_dev);
+	if (err) {
+		dev_err(&wdev->dev,
+			"Could not register input device: %d\n", err);
+		return err;
+	}
+
+	priv->input_dev = input_dev;
+	dev_set_drvdata(&wdev->dev, priv);
+	lenovo_ymc_trigger_ec(wdev, priv);
+	return 0;
+}
+
+static const struct wmi_device_id lenovo_ymc_wmi_id_table[] = {
+	{ .guid_string = LENOVO_YMC_EVENT_GUID },
+	{ }
+};
+MODULE_DEVICE_TABLE(wmi, lenovo_ymc_wmi_id_table);
+
+static struct wmi_driver lenovo_ymc_driver = {
+	.driver = {
+		.name = "lenovo-ymc",
+	},
+	.id_table = lenovo_ymc_wmi_id_table,
+	.probe = lenovo_ymc_probe,
+	.notify = lenovo_ymc_notify,
+};
+
+module_wmi_driver(lenovo_ymc_driver);
+
+MODULE_AUTHOR("Gergo Koteles <soyer@irl.hu>");
+MODULE_DESCRIPTION("Lenovo Yoga Mode Control driver");
+MODULE_LICENSE("GPL");
-- 
2.39.2

Re: [PATCH 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Armin Wolf]

Am 10.03.23 um 05:17 schrieb Andrew Kallmeyer:

> This WMI driver for the tablet mode control switch for Lenovo Yoga
> notebooks was originally written by Gergo Koteles. The mode is mapped to
> a SW_TABLET_MODE switch capable input device.
>
> I (Andrew) followed the suggestions that were posted in reply to Gergo's
> RFC patch to follow-up and get it merged. Additionally I merged the
> lenovo_ymc_trigger_ec function with the ideapad_trigger_ymc_next_read
> function since it was no longer external. I have also added the word
> "Tablet" to the driver description to hopefully make it more clear.
>
> As Gergo said: It should work with  models like the Yoga C940, Ideapad
> flex 14API, Yoga 9 14IAP7, Yoga 7 14ARB7.
> The 14ARB7 model must trigger the EC to send mode change events. This
> might be a firmware bug.
>
> I have additionally tested this on the Yoga 7 14IAL7.
>
> Link: https://lore.kernel.org/r/20221004214332.35934-1-soyer@irl.hu/
> Co-developed-by: Andrew Kallmeyer <kallmeyeras@gmail.com>
> Signed-off-by: Andrew Kallmeyer <kallmeyeras@gmail.com>
> ---
>   drivers/platform/x86/Kconfig          |  10 ++
>   drivers/platform/x86/Makefile         |   1 +
>   drivers/platform/x86/ideapad-laptop.h |   1 +
>   drivers/platform/x86/lenovo-ymc.c     | 177 ++++++++++++++++++++++++++
>   4 files changed, 189 insertions(+)
>   create mode 100644 drivers/platform/x86/lenovo-ymc.c
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 5692385e2..858be0c65 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -470,6 +470,16 @@ config IDEAPAD_LAPTOP
>   	  This is a driver for Lenovo IdeaPad netbooks contains drivers for
>   	  rfkill switch, hotkey, fan control and backlight control.
>
> +config LENOVO_YMC
> +	tristate "Lenovo Yoga Tablet Mode Control"
> +	depends on ACPI_WMI
> +	depends on INPUT
> +	depends on IDEAPAD_LAPTOP
> +	select INPUT_SPARSEKMAP
> +	help
> +	  This driver maps the Tablet Mode Control switch to SW_TABLET_MODE input
> +	  events for Lenovo Yoga notebooks.
> +
>   config SENSORS_HDAPS
>   	tristate "Thinkpad Hard Drive Active Protection System (hdaps)"
>   	depends on INPUT
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 1d3d1b025..10054cdea 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -63,6 +63,7 @@ obj-$(CONFIG_UV_SYSFS)       += uv_sysfs.o
>   # IBM Thinkpad and Lenovo
>   obj-$(CONFIG_IBM_RTL)		+= ibm_rtl.o
>   obj-$(CONFIG_IDEAPAD_LAPTOP)	+= ideapad-laptop.o
> +obj-$(CONFIG_LENOVO_YMC)	+= lenovo-ymc.o
>   obj-$(CONFIG_SENSORS_HDAPS)	+= hdaps.o
>   obj-$(CONFIG_THINKPAD_ACPI)	+= thinkpad_acpi.o
>   obj-$(CONFIG_THINKPAD_LMI)	+= think-lmi.o
> diff --git a/drivers/platform/x86/ideapad-laptop.h b/drivers/platform/x86/ideapad-laptop.h
> index 7dd8ce027..2564cb1cd 100644
> --- a/drivers/platform/x86/ideapad-laptop.h
> +++ b/drivers/platform/x86/ideapad-laptop.h
> @@ -35,6 +35,7 @@ enum {
>   	VPCCMD_W_FAN,
>   	VPCCMD_R_RF,
>   	VPCCMD_W_RF,
> +	VPCCMD_W_YMC = 0x2A,
>   	VPCCMD_R_FAN = 0x2B,
>   	VPCCMD_R_SPECIAL_BUTTONS = 0x31,
>   	VPCCMD_W_BL_POWER = 0x33,
> diff --git a/drivers/platform/x86/lenovo-ymc.c b/drivers/platform/x86/lenovo-ymc.c
> new file mode 100644
> index 000000000..e29ada1b4
> --- /dev/null
> +++ b/drivers/platform/x86/lenovo-ymc.c
> @@ -0,0 +1,177 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * lenovo-ymc.c - Lenovo Yoga Mode Control driver
> + *
> + * Copyright © 2022 Gergo Koteles <soyer@irl.hu>
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/dmi.h>
> +#include <linux/input.h>
> +#include <linux/input/sparse-keymap.h>
> +#include <linux/wmi.h>
> +#include "ideapad-laptop.h"
> +
> +#define LENOVO_YMC_EVENT_GUID	"06129D99-6083-4164-81AD-F092F9D773A6"
> +#define LENOVO_YMC_QUERY_GUID	"09B0EE6E-C3FD-4243-8DA1-7911FF80BB8C"
> +
> +#define LENOVO_YMC_QUERY_INSTANCE 0
> +#define LENOVO_YMC_QUERY_METHOD 0xAB

Hi,

according to the embedded MOF data, the method id for retrieving the mode data
is 0x1, not 0xAB.

> +
> +static bool ec_trigger __read_mostly;
> +module_param(ec_trigger, bool, 0644);
> +MODULE_PARM_DESC(ec_trigger, "Enable EC triggering to emit YMC events");
> +
> +static const struct dmi_system_id ec_trigger_quirk_dmi_table[] = {
> +	{
> +		// Lenovo Yoga 7 14ARB7
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "82QF"),
> +		},
> +	},
> +	{ },
> +};
> +
> +struct lenovo_ymc_private {
> +	struct input_dev *input_dev;
> +	struct acpi_device *ec_acpi_dev;
> +};
> +
> +static void lenovo_ymc_trigger_ec(struct wmi_device *wdev, struct lenovo_ymc_private *priv)
> +{
> +	int err;
> +
> +	if (!ec_trigger || !priv || !priv->ec_acpi_dev)
> +		return;
> +	err = write_ec_cmd(priv->ec_acpi_dev->handle, VPCCMD_W_YMC, 1);
> +	if (err)
> +		dev_warn(&wdev->dev, "Could not write YMC: %d\n", err);
> +}
> +
> +static const struct key_entry lenovo_ymc_keymap[] = {
> +	// Laptop
> +	{ KE_SW, 0x01, { .sw = { SW_TABLET_MODE, 0 } } },
> +	// Tablet
> +	{ KE_SW, 0x02, { .sw = { SW_TABLET_MODE, 1 } } },
> +	// Drawing Board
> +	{ KE_SW, 0x03, { .sw = { SW_TABLET_MODE, 1 } } },
> +	// Tent
> +	{ KE_SW, 0x04, { .sw = { SW_TABLET_MODE, 1 } } },
> +	{ KE_END },
> +};
> +
> +static void lenovo_ymc_notify(struct wmi_device *wdev, union acpi_object *data)
> +{
> +	struct lenovo_ymc_private *priv = dev_get_drvdata(&wdev->dev);
> +
> +	u32 input_val = 0;
> +	struct acpi_buffer input = {sizeof(input_val), &input_val};
> +	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
> +	acpi_status status;
> +	union acpi_object *obj;
> +	int code;
> +
> +	status = wmi_evaluate_method(LENOVO_YMC_QUERY_GUID,
> +				LENOVO_YMC_QUERY_INSTANCE,
> +				LENOVO_YMC_QUERY_METHOD,
> +				&input, &output);

Using wmi_evaluate_method() is deprecated as WMI GUIDs are not guaranteed to be
unique for a given machine. I think both WMI GUIDs should be handled by separate
drivers, and the driver for the event GUID uses a notifier call chain to inform
the driver for the data GUID that the usage mode changed, which then handles the
input device stuff.

This way the driver does not rely on deprecated WMI APIs, and everything would
still work should Lenovo decide to duplicate some WMI GUIDs.

> +
> +	if (ACPI_FAILURE(status)) {
> +		dev_warn(&wdev->dev,
> +			"Failed to evaluate query method: %s\n",
> +			acpi_format_exception(status));
> +		return;
> +	}
> +
> +	obj = output.pointer;
> +
> +	if (obj->type != ACPI_TYPE_INTEGER) {
> +		dev_warn(&wdev->dev,
> +			"WMI event data is not an integer\n");
> +		goto free_obj;
> +	}
> +	code = obj->integer.value;
> +
> +	if (!sparse_keymap_report_event(priv->input_dev, code, 1, true))
> +		dev_warn(&wdev->dev, "Unknown key %d pressed\n", code);
> +
> +free_obj:
> +	kfree(obj);
> +	lenovo_ymc_trigger_ec(wdev, priv);
> +}
> +
> +static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
> +{
> +	struct input_dev *input_dev;
> +	struct lenovo_ymc_private *priv;
> +	int err;
> +
> +	ec_trigger |= dmi_check_system(ec_trigger_quirk_dmi_table);
> +
> +	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	if (ec_trigger) {
> +		pr_debug("Lenovo YMC enable EC triggering.\n");
> +		priv->ec_acpi_dev = acpi_dev_get_first_match_dev("VCP2004", NULL, -1);

acpi_dev_put() is missing.

> +		if (!priv->ec_acpi_dev) {
> +			dev_err(&wdev->dev, "Could not find EC ACPI device.\n");
> +			return -ENODEV;
> +		}
> +	}
> +
> +	input_dev = devm_input_allocate_device(&wdev->dev);
> +	if (!input_dev)
> +		return -ENOMEM;
> +
> +	input_dev->name = "Lenovo Yoga Tablet Mode Control switch";
> +	input_dev->phys = LENOVO_YMC_EVENT_GUID "/input0";
> +	input_dev->id.bustype = BUS_HOST;
> +	input_dev->dev.parent = &wdev->dev;
> +
> +	input_set_capability(input_dev, EV_SW, SW_TABLET_MODE);
> +
> +	err = sparse_keymap_setup(input_dev, lenovo_ymc_keymap, NULL);
> +	if (err) {
> +		dev_err(&wdev->dev,
> +			"Could not set up input device keymap: %d\n", err);
> +		return err;
> +	}
> +
> +	err = input_register_device(input_dev);
> +	if (err) {
> +		dev_err(&wdev->dev,
> +			"Could not register input device: %d\n", err);
> +		return err;
> +	}

Maybe it would be beneficial to allow userspace to get the current usage mode without having
to wait for an WMI event. This way, userspace could still react to situations like the device
already being in tablet mode when this driver is loaded.

> +
> +	priv->input_dev = input_dev;
> +	dev_set_drvdata(&wdev->dev, priv);
> +	lenovo_ymc_trigger_ec(wdev, priv);
> +	return 0;
> +}
> +
> +static const struct wmi_device_id lenovo_ymc_wmi_id_table[] = {
> +	{ .guid_string = LENOVO_YMC_EVENT_GUID },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(wmi, lenovo_ymc_wmi_id_table);

If the drivers handling the event and data GUIDs are fine with being instantiated multiple
times, then adding the WMI GUIDs to the allow_duplicates[] list in drivers/platform/x86/wmi.c
will allow the WMI driver core to handle duplicated event and data GUIDs.

Armin Wolf

> +
> +static struct wmi_driver lenovo_ymc_driver = {
> +	.driver = {
> +		.name = "lenovo-ymc",
> +	},
> +	.id_table = lenovo_ymc_wmi_id_table,
> +	.probe = lenovo_ymc_probe,
> +	.notify = lenovo_ymc_notify,
> +};
> +
> +module_wmi_driver(lenovo_ymc_driver);
> +
> +MODULE_AUTHOR("Gergo Koteles <soyer@irl.hu>");
> +MODULE_DESCRIPTION("Lenovo Yoga Mode Control driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Andrew Kallmeyer]

On Fri, Mar 10, 2023 at 2:28 AM Armin Wolf <W_Armin@gmx.de> wrote:
>
> Hi,
>
> according to the embedded MOF data, the method id for retrieving the mode data
> is 0x1, not 0xAB.

Thanks! I can see from your earlier email in the other thread that
this is right. Strangely, when I tested 0xAB had almost exactly the
same behavior as 0x01. I think you're right though, I have updated my
local copy to 0x01.

I have also fixed the missing cleanup calls and

> Using wmi_evaluate_method() is deprecated as WMI GUIDs are not guaranteed to be
> unique for a given machine. I think both WMI GUIDs should be handled by separate
> drivers, and the driver for the event GUID uses a notifier call chain to inform
> the driver for the data GUID that the usage mode changed, which then handles the
> input device stuff.
>
> This way the driver does not rely on deprecated WMI APIs, and everything would
> still work should Lenovo decide to duplicate some WMI GUIDs.

After reading many times, I believe I understand this and can figure
out the implementation. How should I attribute the commits? Should
this be a 3rd patch in a v2 patch series with myself as the author? Or
should these drivers be introduced in one commit without the
intermediate single driver that uses the deprecated API?

Also have I correctly set Gergo as the commit author for this PATCH
2/2 email like Hans said to? I wasn't sure if I had it right and I
could fix it in the v2 series.

> acpi_dev_put() is missing.

Thanks! Not sure why I thought it was okay to delete lenovo_ymc_remove
but I have added that back in with the input_unregister_device call.

> Maybe it would be beneficial to allow userspace to get the current usage mode without having
> to wait for an WMI event. This way, userspace could still react to situations like the device
> already being in tablet mode when this driver is loaded.

I'm not following how to implement this, not familiar enough with WMI.
Could you explain more?

> If the drivers handling the event and data GUIDs are fine with being instantiated multiple
> times, then adding the WMI GUIDs to the allow_duplicates[] list in drivers/platform/x86/wmi.c
> will allow the WMI driver core to handle duplicated event and data GUIDs.

Is there an easy way to test if it's fine to run multiple copies?
Currently testing by compiling the module with an inlined
ideapad-laptop.h out of the kernel tree and using the insmod command
to load it.

Re: [PATCH 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Armin Wolf]

Am 15.03.23 um 04:37 schrieb Andrew Kallmeyer:

> On Fri, Mar 10, 2023 at 2:28 AM Armin Wolf <W_Armin@gmx.de> wrote:
>> Hi,
>>
>> according to the embedded MOF data, the method id for retrieving the mode data
>> is 0x1, not 0xAB.
> Thanks! I can see from your earlier email in the other thread that
> this is right. Strangely, when I tested 0xAB had almost exactly the
> same behavior as 0x01. I think you're right though, I have updated my
> local copy to 0x01.

Hi,

the reason for this is that most hardware vendors will omit any input checks
if they think they are unnecessary. The WMI interface on my Dell notebook
does the same, sadly.

> I have also fixed the missing cleanup calls and
>
>> Using wmi_evaluate_method() is deprecated as WMI GUIDs are not guaranteed to be
>> unique for a given machine. I think both WMI GUIDs should be handled by separate
>> drivers, and the driver for the event GUID uses a notifier call chain to inform
>> the driver for the data GUID that the usage mode changed, which then handles the
>> input device stuff.
>>
>> This way the driver does not rely on deprecated WMI APIs, and everything would
>> still work should Lenovo decide to duplicate some WMI GUIDs.
> After reading many times, I believe I understand this and can figure
> out the implementation. How should I attribute the commits? Should
> this be a 3rd patch in a v2 patch series with myself as the author? Or
> should these drivers be introduced in one commit without the
> intermediate single driver that uses the deprecated API?

I thing the new driver(s) should be introduced in one commit. I am not sure about the
new author of the commit. If the original driver was significantly altered, then AFAIK
you should be the new author. Still, the original author should at least be mentioned
with a "Co-developed by" tag. You can then omit your "Co-developed by" tag.

As a site note, i recommend to use the get_maintainer.pl scripts under scripts/ to find
any additional maintainers which should be CC-ed. Since your patch series touches the
ideapad-laptop driver, its maintainer (Ike Panhc <ike.pan@canonical.com>) should also be
notified about this patch series.

> Also have I correctly set Gergo as the commit author for this PATCH
> 2/2 email like Hans said to? I wasn't sure if I had it right and I
> could fix it in the v2 series.
>
You are still the author of the second patch. Also you should not send a patch series as
a reply to any previous emails.

>> acpi_dev_put() is missing.
> Thanks! Not sure why I thought it was okay to delete lenovo_ymc_remove
> but I have added that back in with the input_unregister_device call.
>
>> Maybe it would be beneficial to allow userspace to get the current usage mode without having
>> to wait for an WMI event. This way, userspace could still react to situations like the device
>> already being in tablet mode when this driver is loaded.
> I'm not following how to implement this, not familiar enough with WMI.
> Could you explain more?

I meant that your driver should, after (!) setting up the input device and event handling, call
sparse_keymap_report_event() with the code obtained from
wmidev_evaluate_method() so that userspace knows about the initial state
of the input device. You might also CC linux-input@vger.kernel.org for
the next patch series, so that the input driver maintainers can also
review your patch series.

>> If the drivers handling the event and data GUIDs are fine with being instantiated multiple
>> times, then adding the WMI GUIDs to the allow_duplicates[] list in drivers/platform/x86/wmi.c
>> will allow the WMI driver core to handle duplicated event and data GUIDs.
> Is there an easy way to test if it's fine to run multiple copies?
> Currently testing by compiling the module with an inlined
> ideapad-laptop.h out of the kernel tree and using the insmod command
> to load it.

Drivers can be instantiated multiple times, and each time their probe callback is invoked,
and many older WMI drivers cannot do this, so the allowlist exists.
The section "State Container" in Documentation/driver-api/driver-model/design-patterns.rst
explains how to write drivers which can be instantiated multiple times.

If your driver is not a singleton, i.e. it can safely be instantiated multiple times, then
you can add its WMI GUID to the allowlist.

Re: [PATCH 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Armin Wolf]

Am 15.03.23 um 23:33 schrieb Armin Wolf:

> Am 15.03.23 um 04:37 schrieb Andrew Kallmeyer:
>
>> On Fri, Mar 10, 2023 at 2:28 AM Armin Wolf <W_Armin@gmx.de> wrote:
>>> Hi,
>>>
>>> according to the embedded MOF data, the method id for retrieving the
>>> mode data
>>> is 0x1, not 0xAB.
>> Thanks! I can see from your earlier email in the other thread that
>> this is right. Strangely, when I tested 0xAB had almost exactly the
>> same behavior as 0x01. I think you're right though, I have updated my
>> local copy to 0x01.
>
> Hi,
>
> the reason for this is that most hardware vendors will omit any input
> checks
> if they think they are unnecessary. The WMI interface on my Dell notebook
> does the same, sadly.
>
>> I have also fixed the missing cleanup calls and
>>
>>> Using wmi_evaluate_method() is deprecated as WMI GUIDs are not
>>> guaranteed to be
>>> unique for a given machine. I think both WMI GUIDs should be handled
>>> by separate
>>> drivers, and the driver for the event GUID uses a notifier call
>>> chain to inform
>>> the driver for the data GUID that the usage mode changed, which then
>>> handles the
>>> input device stuff.
>>>
>>> This way the driver does not rely on deprecated WMI APIs, and
>>> everything would
>>> still work should Lenovo decide to duplicate some WMI GUIDs.
>> After reading many times, I believe I understand this and can figure
>> out the implementation. How should I attribute the commits? Should
>> this be a 3rd patch in a v2 patch series with myself as the author? Or
>> should these drivers be introduced in one commit without the
>> intermediate single driver that uses the deprecated API?
>
> I thing the new driver(s) should be introduced in one commit. I am not
> sure about the
> new author of the commit. If the original driver was significantly
> altered, then AFAIK
> you should be the new author. Still, the original author should at
> least be mentioned
> with a "Co-developed by" tag. You can then omit your "Co-developed by"
> tag.
>
> As a site note, i recommend to use the get_maintainer.pl scripts under
> scripts/ to find
> any additional maintainers which should be CC-ed. Since your patch
> series touches the
> ideapad-laptop driver, its maintainer (Ike Panhc
> <ike.pan@canonical.com>) should also be
> notified about this patch series.
>
I forgot to mention that your patches have to title, please add one for the next revision.

Armin Wolf

>> Also have I correctly set Gergo as the commit author for this PATCH
>> 2/2 email like Hans said to? I wasn't sure if I had it right and I
>> could fix it in the v2 series.
>>
> You are still the author of the second patch. Also you should not send
> a patch series as
> a reply to any previous emails.
>
>>> acpi_dev_put() is missing.
>> Thanks! Not sure why I thought it was okay to delete lenovo_ymc_remove
>> but I have added that back in with the input_unregister_device call.
>>
>>> Maybe it would be beneficial to allow userspace to get the current
>>> usage mode without having
>>> to wait for an WMI event. This way, userspace could still react to
>>> situations like the device
>>> already being in tablet mode when this driver is loaded.
>> I'm not following how to implement this, not familiar enough with WMI.
>> Could you explain more?
>
> I meant that your driver should, after (!) setting up the input device
> and event handling, call
> sparse_keymap_report_event() with the code obtained from
> wmidev_evaluate_method() so that userspace knows about the initial state
> of the input device. You might also CC linux-input@vger.kernel.org for
> the next patch series, so that the input driver maintainers can also
> review your patch series.
>
>>> If the drivers handling the event and data GUIDs are fine with being
>>> instantiated multiple
>>> times, then adding the WMI GUIDs to the allow_duplicates[] list in
>>> drivers/platform/x86/wmi.c
>>> will allow the WMI driver core to handle duplicated event and data
>>> GUIDs.
>> Is there an easy way to test if it's fine to run multiple copies?
>> Currently testing by compiling the module with an inlined
>> ideapad-laptop.h out of the kernel tree and using the insmod command
>> to load it.
>
> Drivers can be instantiated multiple times, and each time their probe
> callback is invoked,
> and many older WMI drivers cannot do this, so the allowlist exists.
> The section "State Container" in
> Documentation/driver-api/driver-model/design-patterns.rst
> explains how to write drivers which can be instantiated multiple times.
>
> If your driver is not a singleton, i.e. it can safely be instantiated
> multiple times, then
> you can add its WMI GUID to the allowlist.
>

Re: [PATCH 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Hans de Goede]

Hi,

Sorry for being a bit late with replying.

Andrew, thank you for your work on this, at a quick glance it looks great.

Armin, thank you for your help / input on this.

On 3/15/23 23:33, Armin Wolf wrote:
> Am 15.03.23 um 04:37 schrieb Andrew Kallmeyer:
> 
>> On Fri, Mar 10, 2023 at 2:28 AM Armin Wolf <W_Armin@gmx.de> wrote:
>>> Hi,
>>>
>>> according to the embedded MOF data, the method id for retrieving the mode data
>>> is 0x1, not 0xAB.
>> Thanks! I can see from your earlier email in the other thread that
>> this is right. Strangely, when I tested 0xAB had almost exactly the
>> same behavior as 0x01. I think you're right though, I have updated my
>> local copy to 0x01.
> 
> Hi,
> 
> the reason for this is that most hardware vendors will omit any input checks
> if they think they are unnecessary. The WMI interface on my Dell notebook
> does the same, sadly.

Ack.


>> I have also fixed the missing cleanup calls and
>>
>>> Using wmi_evaluate_method() is deprecated as WMI GUIDs are not guaranteed to be
>>> unique for a given machine. I think both WMI GUIDs should be handled by separate
>>> drivers, and the driver for the event GUID uses a notifier call chain to inform
>>> the driver for the data GUID that the usage mode changed, which then handles the
>>> input device stuff.
>>>
>>> This way the driver does not rely on deprecated WMI APIs, and everything would
>>> still work should Lenovo decide to duplicate some WMI GUIDs.

Jumping into the discussion a bit late here, but I disagree with this, using wmi_evaluate_method() here is fine. The chances of there ever being 2 instances of the LENOVO_YMC_QUERY_GUID are very small and we can deal with that if it ever happens.

So I really so no need to make the code needlessly complicated with 2 sub-drivers which then notify each other. Let keep things KISS and keep this as is, so for the next version only change
the method_id from 0xab to 0x01.

>> After reading many times, I believe I understand this and can figure
>> out the implementation. How should I attribute the commits? Should
>> this be a 3rd patch in a v2 patch series with myself as the author? Or
>> should these drivers be introduced in one commit without the
>> intermediate single driver that uses the deprecated API?
> 
> I thing the new driver(s) should be introduced in one commit. I am not sure about the
> new author of the commit. If the original driver was significantly altered, then AFAIK
> you should be the new author. Still, the original author should at least be mentioned
> with a "Co-developed by" tag. You can then omit your "Co-developed by" tag.

Ack to introducing the new driver in 1 commit. Since compared to this version only the method_id is going to make change t make it makes sense to keep Gergo as author and Andrew as Co-developed-by .

> As a site note, i recommend to use the get_maintainer.pl scripts under scripts/ to find
> any additional maintainers which should be CC-ed. Since your patch series touches the
> ideapad-laptop driver, its maintainer (Ike Panhc <ike.pan@canonical.com>) should also be
> notified about this patch series.
>
>> Also have I correctly set Gergo as the commit author for this PATCH
>> 2/2 email like Hans said to? I wasn't sure if I had it right and I
>> could fix it in the v2 series.
>>
> You are still the author of the second patch. Also you should not send a patch series as
> a reply to any previous emails.
> 
>>> acpi_dev_put() is missing.
>> Thanks! Not sure why I thought it was okay to delete lenovo_ymc_remove
>> but I have added that back in with the input_unregister_device call.
>>
>>> Maybe it would be beneficial to allow userspace to get the current usage mode without having
>>> to wait for an WMI event. This way, userspace could still react to situations like the device
>>> already being in tablet mode when this driver is loaded.
>> I'm not following how to implement this, not familiar enough with WMI.
>> Could you explain more?
> 
> I meant that your driver should, after (!) setting up the input device and event handling, call
> sparse_keymap_report_event() with the code obtained from
> wmidev_evaluate_method() so that userspace knows about the initial state
> of the input device. You might also CC linux-input@vger.kernel.org for
> the next patch series, so that the input driver maintainers can also
> review your patch series.

Ack, reporting the initial state would be a good addition. To keep the authorship clear I think this can be added as a follow-up 3/3 patch in the next version.

I realize this contradicts a bit with my previous lets introduce the driver in 1 commit thing, but in this case I think this makes the most sense.

>>> If the drivers handling the event and data GUIDs are fine with being instantiated multiple
>>> times, then adding the WMI GUIDs to the allow_duplicates[] list in drivers/platform/x86/wmi.c
>>> will allow the WMI driver core to handle duplicated event and data GUIDs.
>> Is there an easy way to test if it's fine to run multiple copies?
>> Currently testing by compiling the module with an inlined
>> ideapad-laptop.h out of the kernel tree and using the insmod command
>> to load it.
> 
> Drivers can be instantiated multiple times, and each time their probe callback is invoked,
> and many older WMI drivers cannot do this, so the allowlist exists.
> The section "State Container" in Documentation/driver-api/driver-model/design-patterns.rst
> explains how to write drivers which can be instantiated multiple times.
> 
> If your driver is not a singleton, i.e. it can safely be instantiated multiple times, then
> you can add its WMI GUID to the allowlist.

I'm not sure about adding this to the allowlist, using the new API is good (and nice and clean) but this is still expected to be a singleton.

Regards,

Hans

Re: [PATCH 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Hans de Goede]

Hi

On 3/15/23 23:39, Armin Wolf wrote:

<snip>

>> As a site note, i recommend to use the get_maintainer.pl scripts under
>> scripts/ to find
>> any additional maintainers which should be CC-ed. Since your patch
>> series touches the
>> ideapad-laptop driver, its maintainer (Ike Panhc
>> <ike.pan@canonical.com>) should also be
>> notified about this patch series.
>>
> I forgot to mention that your patches have to title, please add one for the next revision.

Armin, I'm not sure what you mean with this ?

For me the patches have a good $subject / first line of the commit message ?

Regards,

Hans





> 
> Armin Wolf
> 
>>> Also have I correctly set Gergo as the commit author for this PATCH
>>> 2/2 email like Hans said to? I wasn't sure if I had it right and I
>>> could fix it in the v2 series.
>>>
>> You are still the author of the second patch. Also you should not send
>> a patch series as
>> a reply to any previous emails.
>>
>>>> acpi_dev_put() is missing.
>>> Thanks! Not sure why I thought it was okay to delete lenovo_ymc_remove
>>> but I have added that back in with the input_unregister_device call.
>>>
>>>> Maybe it would be beneficial to allow userspace to get the current
>>>> usage mode without having
>>>> to wait for an WMI event. This way, userspace could still react to
>>>> situations like the device
>>>> already being in tablet mode when this driver is loaded.
>>> I'm not following how to implement this, not familiar enough with WMI.
>>> Could you explain more?
>>
>> I meant that your driver should, after (!) setting up the input device
>> and event handling, call
>> sparse_keymap_report_event() with the code obtained from
>> wmidev_evaluate_method() so that userspace knows about the initial state
>> of the input device. You might also CC linux-input@vger.kernel.org for
>> the next patch series, so that the input driver maintainers can also
>> review your patch series.
>>
>>>> If the drivers handling the event and data GUIDs are fine with being
>>>> instantiated multiple
>>>> times, then adding the WMI GUIDs to the allow_duplicates[] list in
>>>> drivers/platform/x86/wmi.c
>>>> will allow the WMI driver core to handle duplicated event and data
>>>> GUIDs.
>>> Is there an easy way to test if it's fine to run multiple copies?
>>> Currently testing by compiling the module with an inlined
>>> ideapad-laptop.h out of the kernel tree and using the insmod command
>>> to load it.
>>
>> Drivers can be instantiated multiple times, and each time their probe
>> callback is invoked,
>> and many older WMI drivers cannot do this, so the allowlist exists.
>> The section "State Container" in
>> Documentation/driver-api/driver-model/design-patterns.rst
>> explains how to write drivers which can be instantiated multiple times.
>>
>> If your driver is not a singleton, i.e. it can safely be instantiated
>> multiple times, then
>> you can add its WMI GUID to the allowlist.
>>
> 

Re: [PATCH 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Armin Wolf]

Am 16.03.23 um 10:02 schrieb Hans de Goede:

> Hi
>
> On 3/15/23 23:39, Armin Wolf wrote:
>
> <snip>
>
>>> As a site note, i recommend to use the get_maintainer.pl scripts under
>>> scripts/ to find
>>> any additional maintainers which should be CC-ed. Since your patch
>>> series touches the
>>> ideapad-laptop driver, its maintainer (Ike Panhc
>>> <ike.pan@canonical.com>) should also be
>>> notified about this patch series.
>>>
>> I forgot to mention that your patches have to title, please add one for the next revision.
> Armin, I'm not sure what you mean with this ?
>
> For me the patches have a good $subject / first line of the commit message ?
>
> Regards,
>
> Hans
>
>
My fault, i messed up. Sorry for the unnecessary noise.

Armin Wolf

>
>
>> Armin Wolf
>>
>>>> Also have I correctly set Gergo as the commit author for this PATCH
>>>> 2/2 email like Hans said to? I wasn't sure if I had it right and I
>>>> could fix it in the v2 series.
>>>>
>>> You are still the author of the second patch. Also you should not send
>>> a patch series as
>>> a reply to any previous emails.
>>>
>>>>> acpi_dev_put() is missing.
>>>> Thanks! Not sure why I thought it was okay to delete lenovo_ymc_remove
>>>> but I have added that back in with the input_unregister_device call.
>>>>
>>>>> Maybe it would be beneficial to allow userspace to get the current
>>>>> usage mode without having
>>>>> to wait for an WMI event. This way, userspace could still react to
>>>>> situations like the device
>>>>> already being in tablet mode when this driver is loaded.
>>>> I'm not following how to implement this, not familiar enough with WMI.
>>>> Could you explain more?
>>> I meant that your driver should, after (!) setting up the input device
>>> and event handling, call
>>> sparse_keymap_report_event() with the code obtained from
>>> wmidev_evaluate_method() so that userspace knows about the initial state
>>> of the input device. You might also CC linux-input@vger.kernel.org for
>>> the next patch series, so that the input driver maintainers can also
>>> review your patch series.
>>>
>>>>> If the drivers handling the event and data GUIDs are fine with being
>>>>> instantiated multiple
>>>>> times, then adding the WMI GUIDs to the allow_duplicates[] list in
>>>>> drivers/platform/x86/wmi.c
>>>>> will allow the WMI driver core to handle duplicated event and data
>>>>> GUIDs.
>>>> Is there an easy way to test if it's fine to run multiple copies?
>>>> Currently testing by compiling the module with an inlined
>>>> ideapad-laptop.h out of the kernel tree and using the insmod command
>>>> to load it.
>>> Drivers can be instantiated multiple times, and each time their probe
>>> callback is invoked,
>>> and many older WMI drivers cannot do this, so the allowlist exists.
>>> The section "State Container" in
>>> Documentation/driver-api/driver-model/design-patterns.rst
>>> explains how to write drivers which can be instantiated multiple times.
>>>
>>> If your driver is not a singleton, i.e. it can safely be instantiated
>>> multiple times, then
>>> you can add its WMI GUID to the allowlist.
>>>

Re: [PATCH 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Armin Wolf]

Am 16.03.23 um 10:00 schrieb Hans de Goede:

> Hi,
>
> Sorry for being a bit late with replying.
>
> Andrew, thank you for your work on this, at a quick glance it looks great.
>
> Armin, thank you for your help / input on this.
>
> On 3/15/23 23:33, Armin Wolf wrote:
>> Am 15.03.23 um 04:37 schrieb Andrew Kallmeyer:
>>
>>> On Fri, Mar 10, 2023 at 2:28 AM Armin Wolf<W_Armin@gmx.de>  wrote:
>>>> Hi,
>>>>
>>>> according to the embedded MOF data, the method id for retrieving the mode data
>>>> is 0x1, not 0xAB.
>>> Thanks! I can see from your earlier email in the other thread that
>>> this is right. Strangely, when I tested 0xAB had almost exactly the
>>> same behavior as 0x01. I think you're right though, I have updated my
>>> local copy to 0x01.
>> Hi,
>>
>> the reason for this is that most hardware vendors will omit any input checks
>> if they think they are unnecessary. The WMI interface on my Dell notebook
>> does the same, sadly.
> Ack.
>
>
>>> I have also fixed the missing cleanup calls and
>>>
>>>> Using wmi_evaluate_method() is deprecated as WMI GUIDs are not guaranteed to be
>>>> unique for a given machine. I think both WMI GUIDs should be handled by separate
>>>> drivers, and the driver for the event GUID uses a notifier call chain to inform
>>>> the driver for the data GUID that the usage mode changed, which then handles the
>>>> input device stuff.
>>>>
>>>> This way the driver does not rely on deprecated WMI APIs, and everything would
>>>> still work should Lenovo decide to duplicate some WMI GUIDs.
> Jumping into the discussion a bit late here, but I disagree with this, using wmi_evaluate_method() here is fine. The chances of there ever being 2 instances of the LENOVO_YMC_QUERY_GUID are very small and we can deal with that if it ever happens.
>
> So I really so no need to make the code needlessly complicated with 2 sub-drivers which then notify each other. Let keep things KISS and keep this as is, so for the next version only change
> the method_id from 0xab to 0x01.

I think that using wmi_evaluate_method() is deprecated and we should try to minimize its usage whenever possible. As for the handling of the WMI GUIDs, i believe that
just using the first matching WMI device is not a stable solution. We simply do not know if Lenovo considers both WMI GUIDs singletons or not. This means they could
for example decide to have multiple independent data sources for tablet mode events. The chances for this are indeed small, but it will still create a problem for users
using such machines. By having two drivers and maybe a global notifier call chain, we would enable the driver to handle such "unlikely situations" correctly.

This would also allow the driver to work on machines missing the WMI event GUID. In such a case, userspace could then just poll the data WMI GUID for input, but
this is just an idea.

>>> After reading many times, I believe I understand this and can figure
>>> out the implementation. How should I attribute the commits? Should
>>> this be a 3rd patch in a v2 patch series with myself as the author? Or
>>> should these drivers be introduced in one commit without the
>>> intermediate single driver that uses the deprecated API?
>> I thing the new driver(s) should be introduced in one commit. I am not sure about the
>> new author of the commit. If the original driver was significantly altered, then AFAIK
>> you should be the new author. Still, the original author should at least be mentioned
>> with a "Co-developed by" tag. You can then omit your "Co-developed by" tag.
> Ack to introducing the new driver in 1 commit. Since compared to this version only the method_id is going to make change t make it makes sense to keep Gergo as author and Andrew as Co-developed-by .
>
>> As a site note, i recommend to use the get_maintainer.pl scripts under scripts/ to find
>> any additional maintainers which should be CC-ed. Since your patch series touches the
>> ideapad-laptop driver, its maintainer (Ike Panhc<ike.pan@canonical.com>) should also be
>> notified about this patch series.
>>
>>> Also have I correctly set Gergo as the commit author for this PATCH
>>> 2/2 email like Hans said to? I wasn't sure if I had it right and I
>>> could fix it in the v2 series.
>>>
>> You are still the author of the second patch. Also you should not send a patch series as
>> a reply to any previous emails.
>>
>>>> acpi_dev_put() is missing.
>>> Thanks! Not sure why I thought it was okay to delete lenovo_ymc_remove
>>> but I have added that back in with the input_unregister_device call.
>>>
>>>> Maybe it would be beneficial to allow userspace to get the current usage mode without having
>>>> to wait for an WMI event. This way, userspace could still react to situations like the device
>>>> already being in tablet mode when this driver is loaded.
>>> I'm not following how to implement this, not familiar enough with WMI.
>>> Could you explain more?
>> I meant that your driver should, after (!) setting up the input device and event handling, call
>> sparse_keymap_report_event() with the code obtained from
>> wmidev_evaluate_method() so that userspace knows about the initial state
>> of the input device. You might also CClinux-input@vger.kernel.org  for
>> the next patch series, so that the input driver maintainers can also
>> review your patch series.
> Ack, reporting the initial state would be a good addition. To keep the authorship clear I think this can be added as a follow-up 3/3 patch in the next version.
>
> I realize this contradicts a bit with my previous lets introduce the driver in 1 commit thing, but in this case I think this makes the most sense.
>
>>>> If the drivers handling the event and data GUIDs are fine with being instantiated multiple
>>>> times, then adding the WMI GUIDs to the allow_duplicates[] list in drivers/platform/x86/wmi.c
>>>> will allow the WMI driver core to handle duplicated event and data GUIDs.
>>> Is there an easy way to test if it's fine to run multiple copies?
>>> Currently testing by compiling the module with an inlined
>>> ideapad-laptop.h out of the kernel tree and using the insmod command
>>> to load it.
>> Drivers can be instantiated multiple times, and each time their probe callback is invoked,
>> and many older WMI drivers cannot do this, so the allowlist exists.
>> The section "State Container" in Documentation/driver-api/driver-model/design-patterns.rst
>> explains how to write drivers which can be instantiated multiple times.
>>
>> If your driver is not a singleton, i.e. it can safely be instantiated multiple times, then
>> you can add its WMI GUID to the allowlist.
> I'm not sure about adding this to the allowlist, using the new API is good (and nice and clean) but this is still expected to be a singleton.

The allowlist is dealing with drivers not jet converted to the WMI bus model. The allowlist should ideally disappear once the conversion has been
completed, something which would become difficult if WMI drivers would continue to rely on the older GUID singleton behavior which is not compliant
with the ACPI WMI spec AFAIK. If we know that our WMI GUID is a singleton (which we do not), then we should handle this inside our driver, not inside
the WMI probing code.

> Regards,
>
> Hans
>

Re: [PATCH 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Andrew Kallmeyer]

On Fri, Mar 17, 2023 at 5:39 AM Armin Wolf <W_Armin@gmx.de> wrote:
>
> Am 16.03.23 um 10:00 schrieb Hans de Goede:
>>
>> So I really so no need to make the code needlessly complicated with 2 sub-drivers which then notify each other. Let keep things KISS and keep this as is, so for the next version only change
>> the method_id from 0xab to 0x01.
>
> I think that using wmi_evaluate_method() is deprecated and we should try to minimize its usage whenever possible. As for the handling of the WMI GUIDs, i believe that
> just using the first matching WMI device is not a stable solution. We simply do not know if Lenovo considers both WMI GUIDs singletons or not. This means they could
> for example decide to have multiple independent data sources for tablet mode events. The chances for this are indeed small, but it will still create a problem for users
> using such machines. By having two drivers and maybe a global notifier call chain, we would enable the driver to handle such "unlikely situations" correctly.
>
> This would also allow the driver to work on machines missing the WMI event GUID. In such a case, userspace could then just poll the data WMI GUID for input, but
> this is just an idea.

Hi Armin, would it work to add the second GUID to the existing
wmi_driver wmi_device_id array? Then I could save the wmi_device in
the driver data on probe. Later when I get the notification on the
other GUID I would just call wmidev_evaluate_method on the saved
pointer out of the private data.

I would just need a way to distinguish the two wmi_device structs.
Seems like the notifier setup wouldn't be needed and it could stay as
one module for one feature.

I have the code ready to mail a v2 patch series with the remove
function added and the fixed method id and the input triggering on
probe, but still using wmi_evaluate_method. Without having much kernel
experience, I sort of agree with Hans that it would be best to be
simpler and not have two modules for one feature, the notifier setup
looks somewhat involved. However if we can do something like the above
idea, that doesn't seem to make it much more complicated to avoid the
deprecated API and I can mail that out instead.

So let me know what you think.

>>> Drivers can be instantiated multiple times, and each time their probe callback is invoked,
>>> and many older WMI drivers cannot do this, so the allowlist exists.
>>> The section "State Container" in Documentation/driver-api/driver-model/design-patterns.rst
>>> explains how to write drivers which can be instantiated multiple times.
>>>
>>> If your driver is not a singleton, i.e. it can safely be instantiated multiple times, then
>>> you can add its WMI GUID to the allowlist.
>> I'm not sure about adding this to the allowlist, using the new API is good (and nice and clean) but this is still expected to be a singleton.
>
> The allowlist is dealing with drivers not jet converted to the WMI bus model. The allowlist should ideally disappear once the conversion has been
> completed, something which would become difficult if WMI drivers would continue to rely on the older GUID singleton behavior which is not compliant
> with the ACPI WMI spec AFAIK. If we know that our WMI GUID is a singleton (which we do not), then we should handle this inside our driver, not inside
> the WMI probing code.

Would it not work to convert this driver to the WMI bus model now? I
wasn't able to find anything about this bus model code.

If I'm understanding
Documentation/driver-api/driver-model/design-patterns.rst correctly
then I think this driver would be okay to have the same GUID probed
multiple times. Each device would get its own private data allocated
for it and that would be cleaned up which each device is removed.
Although, it does look like you would get double input events. I'm not
sure if that's the correct behavior without access to a machine with
two of these switch devices to test on.

- Andrew

Re: [PATCH 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Andrew Kallmeyer]

On Sat, Mar 18, 2023 at 10:50 AM Andrew Kallmeyer <kallmeyeras@gmail.com> wrote:
>
> Hi Armin, would it work to add the second GUID to the existing
> wmi_driver wmi_device_id array? Then I could save the wmi_device in
> the driver data on probe. Later when I get the notification on the
> other GUID I would just call wmidev_evaluate_method on the saved
> pointer out of the private data.

Now that I have understood the multiple probe calls and went back to
read this, I realized that I would not be able to access the private
data of the notify device when handling the probe call for the query
device. Maybe you will have a good idea to solve this problem.

If not I do have this working with the deprecated call still.

- Andrew

Re: [PATCH 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Hans de Goede]

Hi Armin,

On 3/17/23 13:39, Armin Wolf wrote:
> Am 16.03.23 um 10:00 schrieb Hans de Goede:
> 
>> Hi,
>>
>> Sorry for being a bit late with replying.
>>
>> Andrew, thank you for your work on this, at a quick glance it looks great.
>>
>> Armin, thank you for your help / input on this.
>>
>> On 3/15/23 23:33, Armin Wolf wrote:
>>> Am 15.03.23 um 04:37 schrieb Andrew Kallmeyer:
>>>
>>>> On Fri, Mar 10, 2023 at 2:28 AM Armin Wolf<W_Armin@gmx.de>  wrote:
>>>>> Hi,
>>>>>
>>>>> according to the embedded MOF data, the method id for retrieving the mode data
>>>>> is 0x1, not 0xAB.
>>>> Thanks! I can see from your earlier email in the other thread that
>>>> this is right. Strangely, when I tested 0xAB had almost exactly the
>>>> same behavior as 0x01. I think you're right though, I have updated my
>>>> local copy to 0x01.
>>> Hi,
>>>
>>> the reason for this is that most hardware vendors will omit any input checks
>>> if they think they are unnecessary. The WMI interface on my Dell notebook
>>> does the same, sadly.
>> Ack.
>>
>>
>>>> I have also fixed the missing cleanup calls and
>>>>
>>>>> Using wmi_evaluate_method() is deprecated as WMI GUIDs are not guaranteed to be
>>>>> unique for a given machine. I think both WMI GUIDs should be handled by separate
>>>>> drivers, and the driver for the event GUID uses a notifier call chain to inform
>>>>> the driver for the data GUID that the usage mode changed, which then handles the
>>>>> input device stuff.
>>>>>
>>>>> This way the driver does not rely on deprecated WMI APIs, and everything would
>>>>> still work should Lenovo decide to duplicate some WMI GUIDs.
>> Jumping into the discussion a bit late here, but I disagree with this, using wmi_evaluate_method() here is fine. The chances of there ever being 2 instances of the LENOVO_YMC_QUERY_GUID are very small and we can deal with that if it ever happens.
>>
>> So I really so no need to make the code needlessly complicated with 2 sub-drivers which then notify each other. Let keep things KISS and keep this as is, so for the next version only change
>> the method_id from 0xab to 0x01.
> 
> I think that using wmi_evaluate_method() is deprecated and we should try to minimize its usage whenever possible. As for the handling of the WMI GUIDs, i believe that
> just using the first matching WMI device is not a stable solution. We simply do not know if Lenovo considers both WMI GUIDs singletons or not. This means they could
> for example decide to have multiple independent data sources for tablet mode events. The chances for this are indeed small, but it will still create a problem for users
> using such machines. By having two drivers and maybe a global notifier call chain, we would enable the driver to handle such "unlikely situations" correctly.

Armin, first of all let me say tht I really appreciate your input / help with
reviewing pdx86 patches.

The problem is that your suggestion with 2 drivers attaching themselves
to 2 different wmi_device-s is that we will need to introduce some
shared global data / variable for the 2 to communicate. Notifiers
don't magically work. They need a shared notifier head between the
producer and consumer(s) to work. So you are still introducing
a singleton.

Sometimes this is a good solution, but in this case this really
seems like an overly complicated solution to me. Making the code
needlessly complicated to deal with a theoretical scenario which
I don't believe will ever happen.

Also see below.

> This would also allow the driver to work on machines missing the WMI event GUID. In such a case, userspace could then just poll the data WMI GUID for input, but
> this is just an idea.
> 
>>>> After reading many times, I believe I understand this and can figure
>>>> out the implementation. How should I attribute the commits? Should
>>>> this be a 3rd patch in a v2 patch series with myself as the author? Or
>>>> should these drivers be introduced in one commit without the
>>>> intermediate single driver that uses the deprecated API?
>>> I thing the new driver(s) should be introduced in one commit. I am not sure about the
>>> new author of the commit. If the original driver was significantly altered, then AFAIK
>>> you should be the new author. Still, the original author should at least be mentioned
>>> with a "Co-developed by" tag. You can then omit your "Co-developed by" tag.
>> Ack to introducing the new driver in 1 commit. Since compared to this version only the method_id is going to make change t make it makes sense to keep Gergo as author and Andrew as Co-developed-by .
>>
>>> As a site note, i recommend to use the get_maintainer.pl scripts under scripts/ to find
>>> any additional maintainers which should be CC-ed. Since your patch series touches the
>>> ideapad-laptop driver, its maintainer (Ike Panhc<ike.pan@canonical.com>) should also be
>>> notified about this patch series.
>>>
>>>> Also have I correctly set Gergo as the commit author for this PATCH
>>>> 2/2 email like Hans said to? I wasn't sure if I had it right and I
>>>> could fix it in the v2 series.
>>>>
>>> You are still the author of the second patch. Also you should not send a patch series as
>>> a reply to any previous emails.
>>>
>>>>> acpi_dev_put() is missing.
>>>> Thanks! Not sure why I thought it was okay to delete lenovo_ymc_remove
>>>> but I have added that back in with the input_unregister_device call.
>>>>
>>>>> Maybe it would be beneficial to allow userspace to get the current usage mode without having
>>>>> to wait for an WMI event. This way, userspace could still react to situations like the device
>>>>> already being in tablet mode when this driver is loaded.
>>>> I'm not following how to implement this, not familiar enough with WMI.
>>>> Could you explain more?
>>> I meant that your driver should, after (!) setting up the input device and event handling, call
>>> sparse_keymap_report_event() with the code obtained from
>>> wmidev_evaluate_method() so that userspace knows about the initial state
>>> of the input device. You might also CClinux-input@vger.kernel.org  for
>>> the next patch series, so that the input driver maintainers can also
>>> review your patch series.
>> Ack, reporting the initial state would be a good addition. To keep the authorship clear I think this can be added as a follow-up 3/3 patch in the next version.
>>
>> I realize this contradicts a bit with my previous lets introduce the driver in 1 commit thing, but in this case I think this makes the most sense.
>>
>>>>> If the drivers handling the event and data GUIDs are fine with being instantiated multiple
>>>>> times, then adding the WMI GUIDs to the allow_duplicates[] list in drivers/platform/x86/wmi.c
>>>>> will allow the WMI driver core to handle duplicated event and data GUIDs.
>>>> Is there an easy way to test if it's fine to run multiple copies?
>>>> Currently testing by compiling the module with an inlined
>>>> ideapad-laptop.h out of the kernel tree and using the insmod command
>>>> to load it.
>>> Drivers can be instantiated multiple times, and each time their probe callback is invoked,
>>> and many older WMI drivers cannot do this, so the allowlist exists.
>>> The section "State Container" in Documentation/driver-api/driver-model/design-patterns.rst
>>> explains how to write drivers which can be instantiated multiple times.
>>>
>>> If your driver is not a singleton, i.e. it can safely be instantiated multiple times, then
>>> you can add its WMI GUID to the allowlist.
>> I'm not sure about adding this to the allowlist, using the new API is good (and nice and clean) but this is still expected to be a singleton.
> 
> The allowlist is dealing with drivers not jet converted to the WMI bus model. The allowlist should ideally disappear once the conversion has been
> completed, something which would become difficult if WMI drivers would continue to rely on the older GUID singleton behavior which is not compliant
> with the ACPI WMI spec AFAIK. If we know that our WMI GUID is a singleton (which we do not), then we should handle this inside our driver, not inside
> the WMI probing code.

Ah I see (wrt the above discussion), your aiming for some ideal scenario here
were all WMI code gets converted to the new model and the singleton assuming
functions can simply be removed.

I'm afraid that this will never happen. The old way of doing things is deprecated
but if you look at drivers like e.g. acer-wmi not only does it deal with
7 different WMI GUIDs in a single driver. It also exposes sysfs interface under
a platform_device which its registers. Moving drivers like this over to
binding to a wmi_device instead of to a self-created platform-device, will
break userspace API so this simply cannot be done.

For reasons of these I have let go the idea of the ideal scenario being
converting all existing WMI code to the new model. I don't believe that
this is a realistic plan.

As such although I don't want to see new WMI drivers which completely
use the old model. I'm ok with using some of the old model when
single driver needs to deal with more then 1 GUID and the expectations
are that the involved GUIDs will always be and stay singletons.

Andrew, sorry for the conflicting advice you are getting here.
Unfortunately this sometimes happens.

In the end I think both approaches (2 separate driver for the
GUIDs; or just keep using wmi_evaluate_method()) are fine.

So since you are doing the work here, you get to choice which
solution you want to pick for the next version.

Regards,

Hans

Re: [PATCH 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Hans de Goede]

Hi Andrew,

On 3/18/23 18:50, Andrew Kallmeyer wrote:
> On Fri, Mar 17, 2023 at 5:39 AM Armin Wolf <W_Armin@gmx.de> wrote:
>>
>> Am 16.03.23 um 10:00 schrieb Hans de Goede:
>>>
>>> So I really so no need to make the code needlessly complicated with 2 sub-drivers which then notify each other. Let keep things KISS and keep this as is, so for the next version only change
>>> the method_id from 0xab to 0x01.
>>
>> I think that using wmi_evaluate_method() is deprecated and we should try to minimize its usage whenever possible. As for the handling of the WMI GUIDs, i believe that
>> just using the first matching WMI device is not a stable solution. We simply do not know if Lenovo considers both WMI GUIDs singletons or not. This means they could
>> for example decide to have multiple independent data sources for tablet mode events. The chances for this are indeed small, but it will still create a problem for users
>> using such machines. By having two drivers and maybe a global notifier call chain, we would enable the driver to handle such "unlikely situations" correctly.
>>
>> This would also allow the driver to work on machines missing the WMI event GUID. In such a case, userspace could then just poll the data WMI GUID for input, but
>> this is just an idea.
> 
> Hi Armin, would it work to add the second GUID to the existing
> wmi_driver wmi_device_id array? Then I could save the wmi_device in
> the driver data on probe. Later when I get the notification on the
> other GUID I would just call wmidev_evaluate_method on the saved
> pointer out of the private data.
> 
> I would just need a way to distinguish the two wmi_device structs.
> Seems like the notifier setup wouldn't be needed and it could stay as
> one module for one feature.
> 
> I have the code ready to mail a v2 patch series with the remove
> function added and the fixed method id and the input triggering on
> probe, but still using wmi_evaluate_method. Without having much kernel
> experience, I sort of agree with Hans that it would be best to be
> simpler and not have two modules for one feature, the notifier setup
> looks somewhat involved.

Yes the notifier setup is somewhat involved. I believe posting
the v2 which you have ready to post as is is fine.

> However if we can do something like the above
> idea, that doesn't seem to make it much more complicated to avoid the
> deprecated API and I can mail that out instead.
> 
> So let me know what you think.
> 
>>>> Drivers can be instantiated multiple times, and each time their probe callback is invoked,
>>>> and many older WMI drivers cannot do this, so the allowlist exists.
>>>> The section "State Container" in Documentation/driver-api/driver-model/design-patterns.rst
>>>> explains how to write drivers which can be instantiated multiple times.
>>>>
>>>> If your driver is not a singleton, i.e. it can safely be instantiated multiple times, then
>>>> you can add its WMI GUID to the allowlist.
>>> I'm not sure about adding this to the allowlist, using the new API is good (and nice and clean) but this is still expected to be a singleton.
>>
>> The allowlist is dealing with drivers not jet converted to the WMI bus model. The allowlist should ideally disappear once the conversion has been
>> completed, something which would become difficult if WMI drivers would continue to rely on the older GUID singleton behavior which is not compliant
>> with the ACPI WMI spec AFAIK. If we know that our WMI GUID is a singleton (which we do not), then we should handle this inside our driver, not inside
>> the WMI probing code.
> 
> Would it not work to convert this driver to the WMI bus model now? I
> wasn't able to find anything about this bus model code.

Assuming that with "this driver" you mean the Yoga Tablet Mode switch
driver you are working on that already is a WMI BUS driver since
it is a wmi_driver which attached itself to a wmi_device.

The old way of doing things was with drivers which instantiated
their own platform_device-s and then attached to those. These
used the old specify a WMI object to operate on through GUID only
methods like wmi_evaluate_method() everywhere.

Regards,

Hans

Re: [PATCH 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Hans de Goede]

Hi,

On 3/18/23 18:55, Andrew Kallmeyer wrote:
> On Sat, Mar 18, 2023 at 10:50 AM Andrew Kallmeyer <kallmeyeras@gmail.com> wrote:
>>
>> Hi Armin, would it work to add the second GUID to the existing
>> wmi_driver wmi_device_id array? Then I could save the wmi_device in
>> the driver data on probe. Later when I get the notification on the
>> other GUID I would just call wmidev_evaluate_method on the saved
>> pointer out of the private data.
> 
> Now that I have understood the multiple probe calls and went back to
> read this, I realized that I would not be able to access the private
> data of the notify device when handling the probe call for the query
> device. Maybe you will have a good idea to solve this problem.

Right in this model you would need a global shared notifier_head
variable on which the event GUID driver would do a notify and
on which the other GUId driver would then register a notifier
to get called when the event GUID driver does the notify.

So as said this is somewhat involved and I'm fine with the simpler
current solution.

Regards,

Hans

Re: [PATCH 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Gergo Koteles]

Hi Andrew,

Thanks for picking this up. Sorry for the late reply.
I no longer think this driver should do the same workaround as ymc.exe 
does, it shouldn't make non-WMI calls.
I think the 14ARB7 should be fixed in the BIOS to work like the other 
Yogas with the same WMI IDs.

So PATCH 1/2 and codes related to ec_trigger are unnecessary.

I only have the 14ARB7 and I can't test this without the ec_trigger.
For this reason, I don't want to be the author of this module.
Could you be the author?

The working C940, 14API reports are from 
https://github.com/lukas-w/yoga-usage-mode module, which uses the same 
WMI IDs.

Regards,
Gergo

Co-developed-by: Gergo Koteles <soyer@irl.hu>
Signed-off-by: Gergo Koteles <soyer@irl.hu>


On 2023. 03. 10. 5:17, Andrew Kallmeyer wrote:
> This WMI driver for the tablet mode control switch for Lenovo Yoga
> notebooks was originally written by Gergo Koteles. The mode is mapped to
> a SW_TABLET_MODE switch capable input device.
> 
> I (Andrew) followed the suggestions that were posted in reply to Gergo's
> RFC patch to follow-up and get it merged. Additionally I merged the
> lenovo_ymc_trigger_ec function with the ideapad_trigger_ymc_next_read
> function since it was no longer external. I have also added the word
> "Tablet" to the driver description to hopefully make it more clear.
> 
> As Gergo said: It should work with  models like the Yoga C940, Ideapad
> flex 14API, Yoga 9 14IAP7, Yoga 7 14ARB7.
> The 14ARB7 model must trigger the EC to send mode change events. This
> might be a firmware bug.
> 
> I have additionally tested this on the Yoga 7 14IAL7.
> 
> Link: https://lore.kernel.org/r/20221004214332.35934-1-soyer@irl.hu/
> Co-developed-by: Andrew Kallmeyer <kallmeyeras@gmail.com>
> Signed-off-by: Andrew Kallmeyer <kallmeyeras@gmail.com>
> ---
>   drivers/platform/x86/Kconfig          |  10 ++
>   drivers/platform/x86/Makefile         |   1 +
>   drivers/platform/x86/ideapad-laptop.h |   1 +
>   drivers/platform/x86/lenovo-ymc.c     | 177 ++++++++++++++++++++++++++
>   4 files changed, 189 insertions(+)
>   create mode 100644 drivers/platform/x86/lenovo-ymc.c
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 5692385e2..858be0c65 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -470,6 +470,16 @@ config IDEAPAD_LAPTOP
>   	  This is a driver for Lenovo IdeaPad netbooks contains drivers for
>   	  rfkill switch, hotkey, fan control and backlight control.
>   
> +config LENOVO_YMC
> +	tristate "Lenovo Yoga Tablet Mode Control"
> +	depends on ACPI_WMI
> +	depends on INPUT
> +	depends on IDEAPAD_LAPTOP
> +	select INPUT_SPARSEKMAP
> +	help
> +	  This driver maps the Tablet Mode Control switch to SW_TABLET_MODE input
> +	  events for Lenovo Yoga notebooks.
> +
>   config SENSORS_HDAPS
>   	tristate "Thinkpad Hard Drive Active Protection System (hdaps)"
>   	depends on INPUT
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 1d3d1b025..10054cdea 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -63,6 +63,7 @@ obj-$(CONFIG_UV_SYSFS)       += uv_sysfs.o
>   # IBM Thinkpad and Lenovo
>   obj-$(CONFIG_IBM_RTL)		+= ibm_rtl.o
>   obj-$(CONFIG_IDEAPAD_LAPTOP)	+= ideapad-laptop.o
> +obj-$(CONFIG_LENOVO_YMC)	+= lenovo-ymc.o
>   obj-$(CONFIG_SENSORS_HDAPS)	+= hdaps.o
>   obj-$(CONFIG_THINKPAD_ACPI)	+= thinkpad_acpi.o
>   obj-$(CONFIG_THINKPAD_LMI)	+= think-lmi.o
> diff --git a/drivers/platform/x86/ideapad-laptop.h b/drivers/platform/x86/ideapad-laptop.h
> index 7dd8ce027..2564cb1cd 100644
> --- a/drivers/platform/x86/ideapad-laptop.h
> +++ b/drivers/platform/x86/ideapad-laptop.h
> @@ -35,6 +35,7 @@ enum {
>   	VPCCMD_W_FAN,
>   	VPCCMD_R_RF,
>   	VPCCMD_W_RF,
> +	VPCCMD_W_YMC = 0x2A,
>   	VPCCMD_R_FAN = 0x2B,
>   	VPCCMD_R_SPECIAL_BUTTONS = 0x31,
>   	VPCCMD_W_BL_POWER = 0x33,
> diff --git a/drivers/platform/x86/lenovo-ymc.c b/drivers/platform/x86/lenovo-ymc.c
> new file mode 100644
> index 000000000..e29ada1b4
> --- /dev/null
> +++ b/drivers/platform/x86/lenovo-ymc.c
> @@ -0,0 +1,177 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * lenovo-ymc.c - Lenovo Yoga Mode Control driver
> + *
> + * Copyright © 2022 Gergo Koteles <soyer@irl.hu>
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/dmi.h>
> +#include <linux/input.h>
> +#include <linux/input/sparse-keymap.h>
> +#include <linux/wmi.h>
> +#include "ideapad-laptop.h"
> +
> +#define LENOVO_YMC_EVENT_GUID	"06129D99-6083-4164-81AD-F092F9D773A6"
> +#define LENOVO_YMC_QUERY_GUID	"09B0EE6E-C3FD-4243-8DA1-7911FF80BB8C"
> +
> +#define LENOVO_YMC_QUERY_INSTANCE 0
> +#define LENOVO_YMC_QUERY_METHOD 0xAB
> +
> +static bool ec_trigger __read_mostly;
> +module_param(ec_trigger, bool, 0644);
> +MODULE_PARM_DESC(ec_trigger, "Enable EC triggering to emit YMC events");
> +
> +static const struct dmi_system_id ec_trigger_quirk_dmi_table[] = {
> +	{
> +		// Lenovo Yoga 7 14ARB7
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "82QF"),
> +		},
> +	},
> +	{ },
> +};
> +
> +struct lenovo_ymc_private {
> +	struct input_dev *input_dev;
> +	struct acpi_device *ec_acpi_dev;
> +};
> +
> +static void lenovo_ymc_trigger_ec(struct wmi_device *wdev, struct lenovo_ymc_private *priv)
> +{
> +	int err;
> +
> +	if (!ec_trigger || !priv || !priv->ec_acpi_dev)
> +		return;
> +	err = write_ec_cmd(priv->ec_acpi_dev->handle, VPCCMD_W_YMC, 1);
> +	if (err)
> +		dev_warn(&wdev->dev, "Could not write YMC: %d\n", err);
> +}
> +
> +static const struct key_entry lenovo_ymc_keymap[] = {
> +	// Laptop
> +	{ KE_SW, 0x01, { .sw = { SW_TABLET_MODE, 0 } } },
> +	// Tablet
> +	{ KE_SW, 0x02, { .sw = { SW_TABLET_MODE, 1 } } },
> +	// Drawing Board
> +	{ KE_SW, 0x03, { .sw = { SW_TABLET_MODE, 1 } } },
> +	// Tent
> +	{ KE_SW, 0x04, { .sw = { SW_TABLET_MODE, 1 } } },
> +	{ KE_END },
> +};
> +
> +static void lenovo_ymc_notify(struct wmi_device *wdev, union acpi_object *data)
> +{
> +	struct lenovo_ymc_private *priv = dev_get_drvdata(&wdev->dev);
> +
> +	u32 input_val = 0;
> +	struct acpi_buffer input = {sizeof(input_val), &input_val};
> +	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
> +	acpi_status status;
> +	union acpi_object *obj;
> +	int code;
> +
> +	status = wmi_evaluate_method(LENOVO_YMC_QUERY_GUID,
> +				LENOVO_YMC_QUERY_INSTANCE,
> +				LENOVO_YMC_QUERY_METHOD,
> +				&input, &output);
> +
> +	if (ACPI_FAILURE(status)) {
> +		dev_warn(&wdev->dev,
> +			"Failed to evaluate query method: %s\n",
> +			acpi_format_exception(status));
> +		return;
> +	}
> +
> +	obj = output.pointer;
> +
> +	if (obj->type != ACPI_TYPE_INTEGER) {
> +		dev_warn(&wdev->dev,
> +			"WMI event data is not an integer\n");
> +		goto free_obj;
> +	}
> +	code = obj->integer.value;
> +
> +	if (!sparse_keymap_report_event(priv->input_dev, code, 1, true))
> +		dev_warn(&wdev->dev, "Unknown key %d pressed\n", code);
> +
> +free_obj:
> +	kfree(obj);
> +	lenovo_ymc_trigger_ec(wdev, priv);
> +}
> +
> +static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
> +{
> +	struct input_dev *input_dev;
> +	struct lenovo_ymc_private *priv;
> +	int err;
> +
> +	ec_trigger |= dmi_check_system(ec_trigger_quirk_dmi_table);
> +
> +	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	if (ec_trigger) {
> +		pr_debug("Lenovo YMC enable EC triggering.\n");
> +		priv->ec_acpi_dev = acpi_dev_get_first_match_dev("VCP2004", NULL, -1);
> +		if (!priv->ec_acpi_dev) {
> +			dev_err(&wdev->dev, "Could not find EC ACPI device.\n");
> +			return -ENODEV;
> +		}
> +	}
> +
> +	input_dev = devm_input_allocate_device(&wdev->dev);
> +	if (!input_dev)
> +		return -ENOMEM;
> +
> +	input_dev->name = "Lenovo Yoga Tablet Mode Control switch";
> +	input_dev->phys = LENOVO_YMC_EVENT_GUID "/input0";
> +	input_dev->id.bustype = BUS_HOST;
> +	input_dev->dev.parent = &wdev->dev;
> +
> +	input_set_capability(input_dev, EV_SW, SW_TABLET_MODE);
> +
> +	err = sparse_keymap_setup(input_dev, lenovo_ymc_keymap, NULL);
> +	if (err) {
> +		dev_err(&wdev->dev,
> +			"Could not set up input device keymap: %d\n", err);
> +		return err;
> +	}
> +
> +	err = input_register_device(input_dev);
> +	if (err) {
> +		dev_err(&wdev->dev,
> +			"Could not register input device: %d\n", err);
> +		return err;
> +	}
> +
> +	priv->input_dev = input_dev;
> +	dev_set_drvdata(&wdev->dev, priv);
> +	lenovo_ymc_trigger_ec(wdev, priv);
> +	return 0;
> +}
> +
> +static const struct wmi_device_id lenovo_ymc_wmi_id_table[] = {
> +	{ .guid_string = LENOVO_YMC_EVENT_GUID },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(wmi, lenovo_ymc_wmi_id_table);
> +
> +static struct wmi_driver lenovo_ymc_driver = {
> +	.driver = {
> +		.name = "lenovo-ymc",
> +	},
> +	.id_table = lenovo_ymc_wmi_id_table,
> +	.probe = lenovo_ymc_probe,
> +	.notify = lenovo_ymc_notify,
> +};
> +
> +module_wmi_driver(lenovo_ymc_driver);
> +
> +MODULE_AUTHOR("Gergo Koteles <soyer@irl.hu>");
> +MODULE_DESCRIPTION("Lenovo Yoga Mode Control driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Andrew Kallmeyer]

On Mon, Mar 20, 2023 at 6:05 PM Gergo Koteles <soyer@irl.hu> wrote:
>
> Hi Andrew,
>
> Thanks for picking this up. Sorry for the late reply.
> I no longer think this driver should do the same workaround as ymc.exe
> does, it shouldn't make non-WMI calls.
> I think the 14ARB7 should be fixed in the BIOS to work like the other
> Yogas with the same WMI IDs.
>
> So PATCH 1/2 and codes related to ec_trigger are unnecessary.
>
> I only have the 14ARB7 and I can't test this without the ec_trigger.
> For this reason, I don't want to be the author of this module.
> Could you be the author?
>
> The working C940, 14API reports are from
> https://github.com/lukas-w/yoga-usage-mode module, which uses the same
> WMI IDs.
>
> Regards,
> Gergo
>
> Co-developed-by: Gergo Koteles <soyer@irl.hu>
> Signed-off-by: Gergo Koteles <soyer@irl.hu>

Hi Gergo, happy to hear from you!

Now it makes sense why this never got submitted. I suppose the 0xAB
method ID came from decompiling that ymc.exe as well? It looks like
the github repo uses 0x01 which is what we found in the acpidump.

I will remove that code like you say, make myself the module author,
and add your Co-developed-by and Signed-off-by tags to the commit.

Thanks,
Andrew

Re: [PATCH 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Hans de Goede]

Hi Gergo,

On 3/21/23 02:05, Gergo Koteles wrote:
> Hi Andrew,
> 
> Thanks for picking this up. Sorry for the late reply.
> I no longer think this driver should do the same workaround as ymc.exe does, it shouldn't make non-WMI calls.
> I think the 14ARB7 should be fixed in the BIOS to work like the other Yogas with the same WMI IDs.
> 
> So PATCH 1/2 and codes related to ec_trigger are unnecessary.

Good to hear from you. May I ask why you think that the ec_trigger is unnecessary ?

I agree that it should be limited to only models which need it,
but if it is necessary on some models to make things work and
if it is what the Lenovo Windows software does on this model,
then I think we should do this in Linux too to make things
work on models which need the EC trigger.

In my experience waiting for a BIOS update to fix this properly
is idle hope and such a BIOS update is very unlikely to happen.

Lenovo seems to have worked around this in their Windows software
with the EC trigger thing, so there is no reason for Lenovo
to change the BIOS.

> I only have the 14ARB7 and I can't test this without the ec_trigger.
> For this reason, I don't want to be the author of this module.
> Could you be the author?

Note that making Andrew the primary author of the patch is
fine regardless of if we keep the EC trigger or not.

Regards,

Hans




> 
> The working C940, 14API reports are from https://github.com/lukas-w/yoga-usage-mode module, which uses the same WMI IDs.
> 
> Regards,
> Gergo
> 
> Co-developed-by: Gergo Koteles <soyer@irl.hu>
> Signed-off-by: Gergo Koteles <soyer@irl.hu>
> 
> 
> On 2023. 03. 10. 5:17, Andrew Kallmeyer wrote:
>> This WMI driver for the tablet mode control switch for Lenovo Yoga
>> notebooks was originally written by Gergo Koteles. The mode is mapped to
>> a SW_TABLET_MODE switch capable input device.
>>
>> I (Andrew) followed the suggestions that were posted in reply to Gergo's
>> RFC patch to follow-up and get it merged. Additionally I merged the
>> lenovo_ymc_trigger_ec function with the ideapad_trigger_ymc_next_read
>> function since it was no longer external. I have also added the word
>> "Tablet" to the driver description to hopefully make it more clear.
>>
>> As Gergo said: It should work with  models like the Yoga C940, Ideapad
>> flex 14API, Yoga 9 14IAP7, Yoga 7 14ARB7.
>> The 14ARB7 model must trigger the EC to send mode change events. This
>> might be a firmware bug.
>>
>> I have additionally tested this on the Yoga 7 14IAL7.
>>
>> Link: https://lore.kernel.org/r/20221004214332.35934-1-soyer@irl.hu/
>> Co-developed-by: Andrew Kallmeyer <kallmeyeras@gmail.com>
>> Signed-off-by: Andrew Kallmeyer <kallmeyeras@gmail.com>
>> ---
>>   drivers/platform/x86/Kconfig          |  10 ++
>>   drivers/platform/x86/Makefile         |   1 +
>>   drivers/platform/x86/ideapad-laptop.h |   1 +
>>   drivers/platform/x86/lenovo-ymc.c     | 177 ++++++++++++++++++++++++++
>>   4 files changed, 189 insertions(+)
>>   create mode 100644 drivers/platform/x86/lenovo-ymc.c
>>
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index 5692385e2..858be0c65 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -470,6 +470,16 @@ config IDEAPAD_LAPTOP
>>         This is a driver for Lenovo IdeaPad netbooks contains drivers for
>>         rfkill switch, hotkey, fan control and backlight control.
>>   +config LENOVO_YMC
>> +    tristate "Lenovo Yoga Tablet Mode Control"
>> +    depends on ACPI_WMI
>> +    depends on INPUT
>> +    depends on IDEAPAD_LAPTOP
>> +    select INPUT_SPARSEKMAP
>> +    help
>> +      This driver maps the Tablet Mode Control switch to SW_TABLET_MODE input
>> +      events for Lenovo Yoga notebooks.
>> +
>>   config SENSORS_HDAPS
>>       tristate "Thinkpad Hard Drive Active Protection System (hdaps)"
>>       depends on INPUT
>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
>> index 1d3d1b025..10054cdea 100644
>> --- a/drivers/platform/x86/Makefile
>> +++ b/drivers/platform/x86/Makefile
>> @@ -63,6 +63,7 @@ obj-$(CONFIG_UV_SYSFS)       += uv_sysfs.o
>>   # IBM Thinkpad and Lenovo
>>   obj-$(CONFIG_IBM_RTL)        += ibm_rtl.o
>>   obj-$(CONFIG_IDEAPAD_LAPTOP)    += ideapad-laptop.o
>> +obj-$(CONFIG_LENOVO_YMC)    += lenovo-ymc.o
>>   obj-$(CONFIG_SENSORS_HDAPS)    += hdaps.o
>>   obj-$(CONFIG_THINKPAD_ACPI)    += thinkpad_acpi.o
>>   obj-$(CONFIG_THINKPAD_LMI)    += think-lmi.o
>> diff --git a/drivers/platform/x86/ideapad-laptop.h b/drivers/platform/x86/ideapad-laptop.h
>> index 7dd8ce027..2564cb1cd 100644
>> --- a/drivers/platform/x86/ideapad-laptop.h
>> +++ b/drivers/platform/x86/ideapad-laptop.h
>> @@ -35,6 +35,7 @@ enum {
>>       VPCCMD_W_FAN,
>>       VPCCMD_R_RF,
>>       VPCCMD_W_RF,
>> +    VPCCMD_W_YMC = 0x2A,
>>       VPCCMD_R_FAN = 0x2B,
>>       VPCCMD_R_SPECIAL_BUTTONS = 0x31,
>>       VPCCMD_W_BL_POWER = 0x33,
>> diff --git a/drivers/platform/x86/lenovo-ymc.c b/drivers/platform/x86/lenovo-ymc.c
>> new file mode 100644
>> index 000000000..e29ada1b4
>> --- /dev/null
>> +++ b/drivers/platform/x86/lenovo-ymc.c
>> @@ -0,0 +1,177 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * lenovo-ymc.c - Lenovo Yoga Mode Control driver
>> + *
>> + * Copyright © 2022 Gergo Koteles <soyer@irl.hu>
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/dmi.h>
>> +#include <linux/input.h>
>> +#include <linux/input/sparse-keymap.h>
>> +#include <linux/wmi.h>
>> +#include "ideapad-laptop.h"
>> +
>> +#define LENOVO_YMC_EVENT_GUID    "06129D99-6083-4164-81AD-F092F9D773A6"
>> +#define LENOVO_YMC_QUERY_GUID    "09B0EE6E-C3FD-4243-8DA1-7911FF80BB8C"
>> +
>> +#define LENOVO_YMC_QUERY_INSTANCE 0
>> +#define LENOVO_YMC_QUERY_METHOD 0xAB
>> +
>> +static bool ec_trigger __read_mostly;
>> +module_param(ec_trigger, bool, 0644);
>> +MODULE_PARM_DESC(ec_trigger, "Enable EC triggering to emit YMC events");
>> +
>> +static const struct dmi_system_id ec_trigger_quirk_dmi_table[] = {
>> +    {
>> +        // Lenovo Yoga 7 14ARB7
>> +        .matches = {
>> +            DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>> +            DMI_MATCH(DMI_PRODUCT_NAME, "82QF"),
>> +        },
>> +    },
>> +    { },
>> +};
>> +
>> +struct lenovo_ymc_private {
>> +    struct input_dev *input_dev;
>> +    struct acpi_device *ec_acpi_dev;
>> +};
>> +
>> +static void lenovo_ymc_trigger_ec(struct wmi_device *wdev, struct lenovo_ymc_private *priv)
>> +{
>> +    int err;
>> +
>> +    if (!ec_trigger || !priv || !priv->ec_acpi_dev)
>> +        return;
>> +    err = write_ec_cmd(priv->ec_acpi_dev->handle, VPCCMD_W_YMC, 1);
>> +    if (err)
>> +        dev_warn(&wdev->dev, "Could not write YMC: %d\n", err);
>> +}
>> +
>> +static const struct key_entry lenovo_ymc_keymap[] = {
>> +    // Laptop
>> +    { KE_SW, 0x01, { .sw = { SW_TABLET_MODE, 0 } } },
>> +    // Tablet
>> +    { KE_SW, 0x02, { .sw = { SW_TABLET_MODE, 1 } } },
>> +    // Drawing Board
>> +    { KE_SW, 0x03, { .sw = { SW_TABLET_MODE, 1 } } },
>> +    // Tent
>> +    { KE_SW, 0x04, { .sw = { SW_TABLET_MODE, 1 } } },
>> +    { KE_END },
>> +};
>> +
>> +static void lenovo_ymc_notify(struct wmi_device *wdev, union acpi_object *data)
>> +{
>> +    struct lenovo_ymc_private *priv = dev_get_drvdata(&wdev->dev);
>> +
>> +    u32 input_val = 0;
>> +    struct acpi_buffer input = {sizeof(input_val), &input_val};
>> +    struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
>> +    acpi_status status;
>> +    union acpi_object *obj;
>> +    int code;
>> +
>> +    status = wmi_evaluate_method(LENOVO_YMC_QUERY_GUID,
>> +                LENOVO_YMC_QUERY_INSTANCE,
>> +                LENOVO_YMC_QUERY_METHOD,
>> +                &input, &output);
>> +
>> +    if (ACPI_FAILURE(status)) {
>> +        dev_warn(&wdev->dev,
>> +            "Failed to evaluate query method: %s\n",
>> +            acpi_format_exception(status));
>> +        return;
>> +    }
>> +
>> +    obj = output.pointer;
>> +
>> +    if (obj->type != ACPI_TYPE_INTEGER) {
>> +        dev_warn(&wdev->dev,
>> +            "WMI event data is not an integer\n");
>> +        goto free_obj;
>> +    }
>> +    code = obj->integer.value;
>> +
>> +    if (!sparse_keymap_report_event(priv->input_dev, code, 1, true))
>> +        dev_warn(&wdev->dev, "Unknown key %d pressed\n", code);
>> +
>> +free_obj:
>> +    kfree(obj);
>> +    lenovo_ymc_trigger_ec(wdev, priv);
>> +}
>> +
>> +static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
>> +{
>> +    struct input_dev *input_dev;
>> +    struct lenovo_ymc_private *priv;
>> +    int err;
>> +
>> +    ec_trigger |= dmi_check_system(ec_trigger_quirk_dmi_table);
>> +
>> +    priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
>> +    if (!priv)
>> +        return -ENOMEM;
>> +
>> +    if (ec_trigger) {
>> +        pr_debug("Lenovo YMC enable EC triggering.\n");
>> +        priv->ec_acpi_dev = acpi_dev_get_first_match_dev("VCP2004", NULL, -1);
>> +        if (!priv->ec_acpi_dev) {
>> +            dev_err(&wdev->dev, "Could not find EC ACPI device.\n");
>> +            return -ENODEV;
>> +        }
>> +    }
>> +
>> +    input_dev = devm_input_allocate_device(&wdev->dev);
>> +    if (!input_dev)
>> +        return -ENOMEM;
>> +
>> +    input_dev->name = "Lenovo Yoga Tablet Mode Control switch";
>> +    input_dev->phys = LENOVO_YMC_EVENT_GUID "/input0";
>> +    input_dev->id.bustype = BUS_HOST;
>> +    input_dev->dev.parent = &wdev->dev;
>> +
>> +    input_set_capability(input_dev, EV_SW, SW_TABLET_MODE);
>> +
>> +    err = sparse_keymap_setup(input_dev, lenovo_ymc_keymap, NULL);
>> +    if (err) {
>> +        dev_err(&wdev->dev,
>> +            "Could not set up input device keymap: %d\n", err);
>> +        return err;
>> +    }
>> +
>> +    err = input_register_device(input_dev);
>> +    if (err) {
>> +        dev_err(&wdev->dev,
>> +            "Could not register input device: %d\n", err);
>> +        return err;
>> +    }
>> +
>> +    priv->input_dev = input_dev;
>> +    dev_set_drvdata(&wdev->dev, priv);
>> +    lenovo_ymc_trigger_ec(wdev, priv);
>> +    return 0;
>> +}
>> +
>> +static const struct wmi_device_id lenovo_ymc_wmi_id_table[] = {
>> +    { .guid_string = LENOVO_YMC_EVENT_GUID },
>> +    { }
>> +};
>> +MODULE_DEVICE_TABLE(wmi, lenovo_ymc_wmi_id_table);
>> +
>> +static struct wmi_driver lenovo_ymc_driver = {
>> +    .driver = {
>> +        .name = "lenovo-ymc",
>> +    },
>> +    .id_table = lenovo_ymc_wmi_id_table,
>> +    .probe = lenovo_ymc_probe,
>> +    .notify = lenovo_ymc_notify,
>> +};
>> +
>> +module_wmi_driver(lenovo_ymc_driver);
>> +
>> +MODULE_AUTHOR("Gergo Koteles <soyer@irl.hu>");
>> +MODULE_DESCRIPTION("Lenovo Yoga Mode Control driver");
>> +MODULE_LICENSE("GPL");
> 

Re: [PATCH 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Gergo Koteles]

Hi Andrew,

On 2023. 03. 21. 3:14, Andrew Kallmeyer wrote:
> On Mon, Mar 20, 2023 at 6:05 PM Gergo Koteles <soyer@irl.hu> wrote:
>>
>> Hi Andrew,
>>
>> Thanks for picking this up. Sorry for the late reply.
>> I no longer think this driver should do the same workaround as ymc.exe
>> does, it shouldn't make non-WMI calls.
>> I think the 14ARB7 should be fixed in the BIOS to work like the other
>> Yogas with the same WMI IDs.
>>
>> So PATCH 1/2 and codes related to ec_trigger are unnecessary.
>>
>> I only have the 14ARB7 and I can't test this without the ec_trigger.
>> For this reason, I don't want to be the author of this module.
>> Could you be the author?
>>
>> The working C940, 14API reports are from
>> https://github.com/lukas-w/yoga-usage-mode module, which uses the same
>> WMI IDs.
>>
>> Regards,
>> Gergo
>>
>> Co-developed-by: Gergo Koteles <soyer@irl.hu>
>> Signed-off-by: Gergo Koteles <soyer@irl.hu>
> 
> Hi Gergo, happy to hear from you!
> 
> Now it makes sense why this never got submitted. I suppose the 0xAB
> method ID came from decompiling that ymc.exe as well? It looks like
> the github repo uses 0x01 which is what we found in the acpidump.
> 

I didn't decompile the ymc.exe, just watched the acpi/wmi trace logs, 
while stopped and started the ymc service, and figured out what it can 
do. After mode changes I saw the VPCW,VPCW,VPCR pattern, what I saw 
before in the ideapad-laptop.c as write_ec_cmd. Then tried with the 
unknown cmds, and the 0x2A worked.

The 0xAB came from the object_id of debug_dump_wdg by mistake.

[    1.562801] wmi: 09B0EE6E-C3FD-4243-8DA1-7911FF80BB8C:
[    1.562802] wmi: 	object_id: AB
[    1.562803] wmi: 	instance_count: 1
[    1.562804] wmi: 	flags: 0x2 ACPI_WMI_METHOD

The correct one is the 0x01 here also.

[WMI, Dynamic, Provider("WmiProv"), Locale("MS\\0x409"), 
Description("LENOVO_GSENSOR_DATA class"), 
guid("{09B0EE6E-C3FD-4243-8DA1-7911FF80BB8C}")]
class LENOVO_GSENSOR_DATA {
   [key, read] string InstanceName;
   [read] boolean Active;

   [WmiMethodId(1), Implemented, Description("Mode Data")] void 
GetUsageMode([out, Description("Mode Data")] uint32 Data);
   [WmiMethodId(2), Implemented, Description("Get Xaxis Value")] void 
GetXaxisValue([out, Description("Get Xaxis Value")] uint32 Data);
   [WmiMethodId(3), Implemented, Description("Get Yaxis Value")] void 
GetYaxisValue([out, Description("Get Yaxis Value")] uint32 Data);
   [WmiMethodId(4), Implemented, Description("Get Zaxis Value")] void 
GetZaxisValue([out, Description("Get Zaxis Value")] uint32 Data);
   [WmiMethodId(5), Implemented, Description("Base to Ground")] void 
GetAngle4Value([out, Description("Base to Ground")] uint32 Data);
   [WmiMethodId(6), Implemented, Description("Screen to Ground")] void 
GetAngle5Value([out, Description("Screen to Ground")] uint32 Data);
   [WmiMethodId(7), Implemented, Description("Screen to Base")] void 
GetAngle6Value([out, Description("Screen to Base")] uint32 Data);
};

It works with any id
Method (WMAB, 3, NotSerialized)
{
    Return (^^PCI0.LPC0.EC0.YGAM) /* \_SB_.PCI0.LPC0.EC0_.YGAM */
}


I've found one thing, in this line

priv->ec_acpi_dev = acpi_dev_get_first_match_dev("VCP2004", NULL, -1);

the correct hw id is "VPC2004", not "VCP2004".

With this modification, it works well.


> I will remove that code like you say, make myself the module author,
> and add your Co-developed-by and Signed-off-by tags to the commit.

According to Hans, it's unrealistic that Lenovo will change this 
triggering behavior, so it can remain.

Thanks,
Gergo

> 
> Thanks,
> Andrew

Re: [PATCH 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Gergo Koteles]

Hi Hans,

On 2023. 03. 21. 10:13, Hans de Goede wrote:
> Hi Gergo,
> 
> On 3/21/23 02:05, Gergo Koteles wrote:
>> Hi Andrew,
>>
>> Thanks for picking this up. Sorry for the late reply.
>> I no longer think this driver should do the same workaround as ymc.exe does, it shouldn't make non-WMI calls.
>> I think the 14ARB7 should be fixed in the BIOS to work like the other Yogas with the same WMI IDs.
>>
>> So PATCH 1/2 and codes related to ec_trigger are unnecessary.
> 
> Good to hear from you. May I ask why you think that the ec_trigger is unnecessary ?
> 

Just the usual struggle for simplicity.

> I agree that it should be limited to only models which need it,
> but if it is necessary on some models to make things work and
> if it is what the Lenovo Windows software does on this model,
> then I think we should do this in Linux too to make things
> work on models which need the EC trigger.
> 
> In my experience waiting for a BIOS update to fix this properly
> is idle hope and such a BIOS update is very unlikely to happen.
> 
> Lenovo seems to have worked around this in their Windows software
> with the EC trigger thing, so there is no reason for Lenovo
> to change the BIOS.
> 
You are right, it's unlikely they'll change this.

Thanks for bringing me back to reality,
Gergo

>> I only have the 14ARB7 and I can't test this without the ec_trigger.
>> For this reason, I don't want to be the author of this module.
>> Could you be the author?
> 
> Note that making Andrew the primary author of the patch is
> fine regardless of if we keep the EC trigger or not.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
>>
>> The working C940, 14API reports are from https://github.com/lukas-w/yoga-usage-mode module, which uses the same WMI IDs.
>>
>> Regards,
>> Gergo
>>
>> Co-developed-by: Gergo Koteles <soyer@irl.hu>
>> Signed-off-by: Gergo Koteles <soyer@irl.hu>
>>
>>
>> On 2023. 03. 10. 5:17, Andrew Kallmeyer wrote:
>>> This WMI driver for the tablet mode control switch for Lenovo Yoga
>>> notebooks was originally written by Gergo Koteles. The mode is mapped to
>>> a SW_TABLET_MODE switch capable input device.
>>>
>>> I (Andrew) followed the suggestions that were posted in reply to Gergo's
>>> RFC patch to follow-up and get it merged. Additionally I merged the
>>> lenovo_ymc_trigger_ec function with the ideapad_trigger_ymc_next_read
>>> function since it was no longer external. I have also added the word
>>> "Tablet" to the driver description to hopefully make it more clear.
>>>
>>> As Gergo said: It should work with  models like the Yoga C940, Ideapad
>>> flex 14API, Yoga 9 14IAP7, Yoga 7 14ARB7.
>>> The 14ARB7 model must trigger the EC to send mode change events. This
>>> might be a firmware bug.
>>>
>>> I have additionally tested this on the Yoga 7 14IAL7.
>>>
>>> Link: https://lore.kernel.org/r/20221004214332.35934-1-soyer@irl.hu/
>>> Co-developed-by: Andrew Kallmeyer <kallmeyeras@gmail.com>
>>> Signed-off-by: Andrew Kallmeyer <kallmeyeras@gmail.com>
>>> ---
>>>    drivers/platform/x86/Kconfig          |  10 ++
>>>    drivers/platform/x86/Makefile         |   1 +
>>>    drivers/platform/x86/ideapad-laptop.h |   1 +
>>>    drivers/platform/x86/lenovo-ymc.c     | 177 ++++++++++++++++++++++++++
>>>    4 files changed, 189 insertions(+)
>>>    create mode 100644 drivers/platform/x86/lenovo-ymc.c
>>>
>>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>>> index 5692385e2..858be0c65 100644
>>> --- a/drivers/platform/x86/Kconfig
>>> +++ b/drivers/platform/x86/Kconfig
>>> @@ -470,6 +470,16 @@ config IDEAPAD_LAPTOP
>>>          This is a driver for Lenovo IdeaPad netbooks contains drivers for
>>>          rfkill switch, hotkey, fan control and backlight control.
>>>    +config LENOVO_YMC
>>> +    tristate "Lenovo Yoga Tablet Mode Control"
>>> +    depends on ACPI_WMI
>>> +    depends on INPUT
>>> +    depends on IDEAPAD_LAPTOP
>>> +    select INPUT_SPARSEKMAP
>>> +    help
>>> +      This driver maps the Tablet Mode Control switch to SW_TABLET_MODE input
>>> +      events for Lenovo Yoga notebooks.
>>> +
>>>    config SENSORS_HDAPS
>>>        tristate "Thinkpad Hard Drive Active Protection System (hdaps)"
>>>        depends on INPUT
>>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
>>> index 1d3d1b025..10054cdea 100644
>>> --- a/drivers/platform/x86/Makefile
>>> +++ b/drivers/platform/x86/Makefile
>>> @@ -63,6 +63,7 @@ obj-$(CONFIG_UV_SYSFS)       += uv_sysfs.o
>>>    # IBM Thinkpad and Lenovo
>>>    obj-$(CONFIG_IBM_RTL)        += ibm_rtl.o
>>>    obj-$(CONFIG_IDEAPAD_LAPTOP)    += ideapad-laptop.o
>>> +obj-$(CONFIG_LENOVO_YMC)    += lenovo-ymc.o
>>>    obj-$(CONFIG_SENSORS_HDAPS)    += hdaps.o
>>>    obj-$(CONFIG_THINKPAD_ACPI)    += thinkpad_acpi.o
>>>    obj-$(CONFIG_THINKPAD_LMI)    += think-lmi.o
>>> diff --git a/drivers/platform/x86/ideapad-laptop.h b/drivers/platform/x86/ideapad-laptop.h
>>> index 7dd8ce027..2564cb1cd 100644
>>> --- a/drivers/platform/x86/ideapad-laptop.h
>>> +++ b/drivers/platform/x86/ideapad-laptop.h
>>> @@ -35,6 +35,7 @@ enum {
>>>        VPCCMD_W_FAN,
>>>        VPCCMD_R_RF,
>>>        VPCCMD_W_RF,
>>> +    VPCCMD_W_YMC = 0x2A,
>>>        VPCCMD_R_FAN = 0x2B,
>>>        VPCCMD_R_SPECIAL_BUTTONS = 0x31,
>>>        VPCCMD_W_BL_POWER = 0x33,
>>> diff --git a/drivers/platform/x86/lenovo-ymc.c b/drivers/platform/x86/lenovo-ymc.c
>>> new file mode 100644
>>> index 000000000..e29ada1b4
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/lenovo-ymc.c
>>> @@ -0,0 +1,177 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +/*
>>> + * lenovo-ymc.c - Lenovo Yoga Mode Control driver
>>> + *
>>> + * Copyright © 2022 Gergo Koteles <soyer@irl.hu>
>>> + */
>>> +
>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>> +
>>> +#include <linux/acpi.h>
>>> +#include <linux/dmi.h>
>>> +#include <linux/input.h>
>>> +#include <linux/input/sparse-keymap.h>
>>> +#include <linux/wmi.h>
>>> +#include "ideapad-laptop.h"
>>> +
>>> +#define LENOVO_YMC_EVENT_GUID    "06129D99-6083-4164-81AD-F092F9D773A6"
>>> +#define LENOVO_YMC_QUERY_GUID    "09B0EE6E-C3FD-4243-8DA1-7911FF80BB8C"
>>> +
>>> +#define LENOVO_YMC_QUERY_INSTANCE 0
>>> +#define LENOVO_YMC_QUERY_METHOD 0xAB
>>> +
>>> +static bool ec_trigger __read_mostly;
>>> +module_param(ec_trigger, bool, 0644);
>>> +MODULE_PARM_DESC(ec_trigger, "Enable EC triggering to emit YMC events");
>>> +
>>> +static const struct dmi_system_id ec_trigger_quirk_dmi_table[] = {
>>> +    {
>>> +        // Lenovo Yoga 7 14ARB7
>>> +        .matches = {
>>> +            DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>>> +            DMI_MATCH(DMI_PRODUCT_NAME, "82QF"),
>>> +        },
>>> +    },
>>> +    { },
>>> +};
>>> +
>>> +struct lenovo_ymc_private {
>>> +    struct input_dev *input_dev;
>>> +    struct acpi_device *ec_acpi_dev;
>>> +};
>>> +
>>> +static void lenovo_ymc_trigger_ec(struct wmi_device *wdev, struct lenovo_ymc_private *priv)
>>> +{
>>> +    int err;
>>> +
>>> +    if (!ec_trigger || !priv || !priv->ec_acpi_dev)
>>> +        return;
>>> +    err = write_ec_cmd(priv->ec_acpi_dev->handle, VPCCMD_W_YMC, 1);
>>> +    if (err)
>>> +        dev_warn(&wdev->dev, "Could not write YMC: %d\n", err);
>>> +}
>>> +
>>> +static const struct key_entry lenovo_ymc_keymap[] = {
>>> +    // Laptop
>>> +    { KE_SW, 0x01, { .sw = { SW_TABLET_MODE, 0 } } },
>>> +    // Tablet
>>> +    { KE_SW, 0x02, { .sw = { SW_TABLET_MODE, 1 } } },
>>> +    // Drawing Board
>>> +    { KE_SW, 0x03, { .sw = { SW_TABLET_MODE, 1 } } },
>>> +    // Tent
>>> +    { KE_SW, 0x04, { .sw = { SW_TABLET_MODE, 1 } } },
>>> +    { KE_END },
>>> +};
>>> +
>>> +static void lenovo_ymc_notify(struct wmi_device *wdev, union acpi_object *data)
>>> +{
>>> +    struct lenovo_ymc_private *priv = dev_get_drvdata(&wdev->dev);
>>> +
>>> +    u32 input_val = 0;
>>> +    struct acpi_buffer input = {sizeof(input_val), &input_val};
>>> +    struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
>>> +    acpi_status status;
>>> +    union acpi_object *obj;
>>> +    int code;
>>> +
>>> +    status = wmi_evaluate_method(LENOVO_YMC_QUERY_GUID,
>>> +                LENOVO_YMC_QUERY_INSTANCE,
>>> +                LENOVO_YMC_QUERY_METHOD,
>>> +                &input, &output);
>>> +
>>> +    if (ACPI_FAILURE(status)) {
>>> +        dev_warn(&wdev->dev,
>>> +            "Failed to evaluate query method: %s\n",
>>> +            acpi_format_exception(status));
>>> +        return;
>>> +    }
>>> +
>>> +    obj = output.pointer;
>>> +
>>> +    if (obj->type != ACPI_TYPE_INTEGER) {
>>> +        dev_warn(&wdev->dev,
>>> +            "WMI event data is not an integer\n");
>>> +        goto free_obj;
>>> +    }
>>> +    code = obj->integer.value;
>>> +
>>> +    if (!sparse_keymap_report_event(priv->input_dev, code, 1, true))
>>> +        dev_warn(&wdev->dev, "Unknown key %d pressed\n", code);
>>> +
>>> +free_obj:
>>> +    kfree(obj);
>>> +    lenovo_ymc_trigger_ec(wdev, priv);
>>> +}
>>> +
>>> +static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
>>> +{
>>> +    struct input_dev *input_dev;
>>> +    struct lenovo_ymc_private *priv;
>>> +    int err;
>>> +
>>> +    ec_trigger |= dmi_check_system(ec_trigger_quirk_dmi_table);
>>> +
>>> +    priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
>>> +    if (!priv)
>>> +        return -ENOMEM;
>>> +
>>> +    if (ec_trigger) {
>>> +        pr_debug("Lenovo YMC enable EC triggering.\n");
>>> +        priv->ec_acpi_dev = acpi_dev_get_first_match_dev("VCP2004", NULL, -1);
>>> +        if (!priv->ec_acpi_dev) {
>>> +            dev_err(&wdev->dev, "Could not find EC ACPI device.\n");
>>> +            return -ENODEV;
>>> +        }
>>> +    }
>>> +
>>> +    input_dev = devm_input_allocate_device(&wdev->dev);
>>> +    if (!input_dev)
>>> +        return -ENOMEM;
>>> +
>>> +    input_dev->name = "Lenovo Yoga Tablet Mode Control switch";
>>> +    input_dev->phys = LENOVO_YMC_EVENT_GUID "/input0";
>>> +    input_dev->id.bustype = BUS_HOST;
>>> +    input_dev->dev.parent = &wdev->dev;
>>> +
>>> +    input_set_capability(input_dev, EV_SW, SW_TABLET_MODE);
>>> +
>>> +    err = sparse_keymap_setup(input_dev, lenovo_ymc_keymap, NULL);
>>> +    if (err) {
>>> +        dev_err(&wdev->dev,
>>> +            "Could not set up input device keymap: %d\n", err);
>>> +        return err;
>>> +    }
>>> +
>>> +    err = input_register_device(input_dev);
>>> +    if (err) {
>>> +        dev_err(&wdev->dev,
>>> +            "Could not register input device: %d\n", err);
>>> +        return err;
>>> +    }
>>> +
>>> +    priv->input_dev = input_dev;
>>> +    dev_set_drvdata(&wdev->dev, priv);
>>> +    lenovo_ymc_trigger_ec(wdev, priv);
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct wmi_device_id lenovo_ymc_wmi_id_table[] = {
>>> +    { .guid_string = LENOVO_YMC_EVENT_GUID },
>>> +    { }
>>> +};
>>> +MODULE_DEVICE_TABLE(wmi, lenovo_ymc_wmi_id_table);
>>> +
>>> +static struct wmi_driver lenovo_ymc_driver = {
>>> +    .driver = {
>>> +        .name = "lenovo-ymc",
>>> +    },
>>> +    .id_table = lenovo_ymc_wmi_id_table,
>>> +    .probe = lenovo_ymc_probe,
>>> +    .notify = lenovo_ymc_notify,
>>> +};
>>> +
>>> +module_wmi_driver(lenovo_ymc_driver);
>>> +
>>> +MODULE_AUTHOR("Gergo Koteles <soyer@irl.hu>");
>>> +MODULE_DESCRIPTION("Lenovo Yoga Mode Control driver");
>>> +MODULE_LICENSE("GPL");
>>
> 

Re: [PATCH 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Andrew Kallmeyer]

On Wed, Mar 22, 2023 at 1:39 PM Gergo Koteles <soyer@irl.hu> wrote:
>
> According to Hans, it's unrealistic that Lenovo will change this
> triggering behavior, so it can remain.

In that case I will leave you as the module and commit author but add
your signed-off, unless you'd still prefer not to be the module
author. You wrote most of this code, I only did a few code review
changes :)

Re: [RFC PATCH] Add Lenovo Yoga Mode Control driver

[Gergo Koteles]

On 2022. 10. 04. 23:43, Gergo Koteles wrote:
> This is a WMI driver for the Mode Control switch for Lenovo Yoga
> notebooks.
> The mode is mapped to a SW_TABLET_MODE switch capable input device.
> 
> It should work with  models like the Yoga C940, Ideapad flex 14API,
> Yoga 9 14IAP7, Yoga 7 14ARB7.
> The 14ARB7 model must trigger the EC to send mode change events.
> This might be a firmware bug.
> 
> Introduces a global variable in the ideapad-laptop driver.
> I would like some advice on how to do it without the variable,
> or how to do it better.
> ---
>   drivers/platform/x86/Kconfig          |  10 ++
>   drivers/platform/x86/Makefile         |   1 +
>   drivers/platform/x86/ideapad-laptop.c |  18 +++
>   drivers/platform/x86/lenovo-ymc.c     | 185 ++++++++++++++++++++++++++
>   4 files changed, 214 insertions(+)
>   create mode 100644 drivers/platform/x86/lenovo-ymc.c
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index f2f98e942cf2..72ff6713e447 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -510,6 +510,16 @@ config IDEAPAD_LAPTOP
>   	  This is a driver for Lenovo IdeaPad netbooks contains drivers for
>   	  rfkill switch, hotkey, fan control and backlight control.
>   
> +config LENOVO_YMC
> +	tristate "Lenovo Yoga Mode Control"
> +	depends on ACPI_WMI
> +	depends on INPUT
> +	depends on IDEAPAD_LAPTOP
> +	select ACPI_PLATFORM_PROFILE
> +	select INPUT_SPARSEKMAP
> +	help
> +	  This is a driver for the Mode Control switch for Lenovo Yoga notebooks.
> +
>   config SENSORS_HDAPS
>   	tristate "Thinkpad Hard Drive Active Protection System (hdaps)"
>   	depends on INPUT
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 5a428caa654a..f8f0d6a0b43b 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -65,6 +65,7 @@ obj-$(CONFIG_UV_SYSFS)       += uv_sysfs.o
>   # IBM Thinkpad and Lenovo
>   obj-$(CONFIG_IBM_RTL)		+= ibm_rtl.o
>   obj-$(CONFIG_IDEAPAD_LAPTOP)	+= ideapad-laptop.o
> +obj-$(CONFIG_LENOVO_YMC)	+= lenovo-ymc.o
>   obj-$(CONFIG_SENSORS_HDAPS)	+= hdaps.o
>   obj-$(CONFIG_THINKPAD_ACPI)	+= thinkpad_acpi.o
>   obj-$(CONFIG_THINKPAD_LMI)	+= think-lmi.o
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index abd0c81d62c4..d88a4ee72bf6 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -103,6 +103,7 @@ enum {
>   	VPCCMD_W_FAN,
>   	VPCCMD_R_RF,
>   	VPCCMD_W_RF,
> +	VPCCMD_W_YMC = 0x2A,
>   	VPCCMD_R_FAN = 0x2B,
>   	VPCCMD_R_SPECIAL_BUTTONS = 0x31,
>   	VPCCMD_W_BL_POWER = 0x33,
> @@ -156,6 +157,8 @@ 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.");
>   
> +static struct ideapad_private *ideapad_priv;
> +
>   /*
>    * ACPI Helpers
>    */
> @@ -1421,6 +1424,19 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv)
>   	}
>   }
>   
> +void ideapad_trigger_ymc_next_read(void)
> +{
> +	int err;
> +	if (ideapad_priv != NULL) {
> +		err = write_ec_cmd(ideapad_priv->adev->handle,
> +			VPCCMD_W_YMC, 1);
> +		if (err)
> +			dev_warn(&ideapad_priv->platform_device->dev,
> +				"Could not write YMC: %d\n", err);
> +	}
> +}
> +EXPORT_SYMBOL(ideapad_trigger_ymc_next_read);
> +
>   static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
>   {
>   	struct ideapad_private *priv = data;
> @@ -1663,6 +1679,7 @@ static int ideapad_acpi_add(struct platform_device *pdev)
>   	}
>   #endif
>   
> +	ideapad_priv = priv;
>   	return 0;
>   
>   #if IS_ENABLED(CONFIG_ACPI_WMI)
> @@ -1696,6 +1713,7 @@ static int ideapad_acpi_remove(struct platform_device *pdev)
>   	struct ideapad_private *priv = dev_get_drvdata(&pdev->dev);
>   	int i;
>   
> +	ideapad_priv = NULL;
>   #if IS_ENABLED(CONFIG_ACPI_WMI)
>   	if (priv->fnesc_guid)
>   		wmi_remove_notify_handler(priv->fnesc_guid);
> diff --git a/drivers/platform/x86/lenovo-ymc.c b/drivers/platform/x86/lenovo-ymc.c
> new file mode 100644
> index 000000000000..0b899b02e12f
> --- /dev/null
> +++ b/drivers/platform/x86/lenovo-ymc.c
> @@ -0,0 +1,185 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * lenovo-ymc.c - Lenovo Yoga Mode Control driver
> + *
> + * Copyright © 2022 Gergo Koteles <soyer@irl.hu>
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/dmi.h>
> +#include <linux/input.h>
> +#include <linux/input/sparse-keymap.h>
> +#include <linux/wmi.h>
> +
> +#define LENOVO_YMC_EVENT_GUID	"06129D99-6083-4164-81AD-F092F9D773A6"
> +#define LENOVO_YMC_QUERY_GUID	"09B0EE6E-C3FD-4243-8DA1-7911FF80BB8C"
> +
> +#define LENOVO_YMC_QUERY_INSTANCE 0
> +#define LENOVO_YMC_QUERY_METHOD 0xAB
> +
> +extern void ideapad_trigger_ymc_next_read(void);
> +
> +static bool ec_trigger __read_mostly;
> +module_param(ec_trigger, bool, 0644);
> +MODULE_PARM_DESC(ec_trigger, "Enable EC triggering to emit YMC events");
> +
> +static int enable_ec_trigger(const struct dmi_system_id *id)
> +{
> +       pr_debug("Lenovo YMC enable EC triggering.\n");
> +       ec_trigger = true;
> +       return 0;
> +}
> +
> +static const struct dmi_system_id ec_trigger_quirk_dmi_table[] = {
> +	{
> +		// Lenovo Yoga 7 14ARB7
> +		.callback = enable_ec_trigger,
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "82QF"),
> +		},
> +	},
> +	{ },
> +};
> +
> +static void lenovo_ymc_trigger_ec(void) {
> +	if (ec_trigger) {
> +		ideapad_trigger_ymc_next_read();
> +	}
> +}
> +
> +
> +struct lenovo_ymc_private {
> +	struct input_dev *input_dev;
> +};
> +
> +
> +static const struct key_entry lenovo_ymc_keymap[] = {
> +	// Laptop
> +	{ KE_SW, 0x01, { .sw = { SW_TABLET_MODE, 0 } } },
> +	// Tablet
> +	{ KE_SW, 0x02, { .sw = { SW_TABLET_MODE, 1 } } },
> +	// Drawing Board
> +	{ KE_SW, 0x03, { .sw = { SW_TABLET_MODE, 1 } } },
> +	// Tent
> +	{ KE_SW, 0x04, { .sw = { SW_TABLET_MODE, 1 } } },
> +	{ KE_END },
> +};
> +
> +static void lenovo_ymc_notify(struct wmi_device *wdev, union acpi_object *data)
> +{
> +	struct lenovo_ymc_private *priv = dev_get_drvdata(&wdev->dev);
> +
> +	u32 input_val = 0;
> +	struct acpi_buffer input = {sizeof(input), &input_val};
> +	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
> +	acpi_status status;
> +	union acpi_object *obj;
> +	int code;
> +
> +	status = wmi_evaluate_method(LENOVO_YMC_QUERY_GUID,
> +				LENOVO_YMC_QUERY_INSTANCE,
> +				LENOVO_YMC_QUERY_METHOD,
> +				&input, &output);
> +
> +	if (ACPI_FAILURE(status)) {
> +		dev_warn(&wdev->dev,
> +			"Failed to evaluate query method %ud\n", status);
> +		return;
> +	}
> +
> +	obj = (union acpi_object *)output.pointer;
> +
> +	if (obj->type != ACPI_TYPE_INTEGER) {
> +		dev_warn(&wdev->dev,
> +			"WMI event data is not an integer\n");
> +		goto free_obj;
> +	}
> +	code = obj->integer.value;
> +
> +	if (!sparse_keymap_report_event(priv->input_dev, code, 1, true))
> +		dev_warn(&wdev->dev, "Unknown key %d pressed\n", code);
> +
> +free_obj:
> +	kfree(obj);
> +	lenovo_ymc_trigger_ec();
> +}
> +
> +static void lenovo_ymc_remove(struct wmi_device *wdev)
> +{
> +	struct lenovo_ymc_private *priv = dev_get_drvdata(&wdev->dev);
> +
> +	input_unregister_device(priv->input_dev);
> +}
> +
> +static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
> +{
> +	struct input_dev *input_dev;
> +	struct lenovo_ymc_private *priv;
> +	int err;
> +
> +	dmi_check_system(ec_trigger_quirk_dmi_table);
> +
> +	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	input_dev = input_allocate_device();
> +	if (!input_dev) {
> +		return -ENOMEM;
> +	}
> +
> +	input_dev->name = "Lenovo Yoga Mode Control switch";
> +	input_dev->phys = LENOVO_YMC_EVENT_GUID "/input0";
> +	input_dev->id.bustype = BUS_HOST;
> +	input_dev->dev.parent = &wdev->dev;
> +
> +	input_set_capability(input_dev, EV_SW, SW_TABLET_MODE);
> +
> +	err = sparse_keymap_setup(input_dev, lenovo_ymc_keymap, NULL);
> +	if (err) {
> +		dev_err(&wdev->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(&wdev->dev,
> +			"Could not register input device: %d\n", err);
> +		goto err_free_dev;
> +	}
> +
> +	priv->input_dev = input_dev;
> +	dev_set_drvdata(&wdev->dev, priv);
> +	lenovo_ymc_trigger_ec();
> +	return 0;
> +
> +err_free_dev:
> +	input_free_device(input_dev);
> +	return err;
> +}
> +
> +static const struct wmi_device_id lenovo_ymc_wmi_id_table[] = {
> +	{ .guid_string = LENOVO_YMC_EVENT_GUID },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(wmi, lenovo_ymc_wmi_id_table);
> +
> +static struct wmi_driver lenovo_ymc_driver = {
> +	.driver = {
> +		.name = "lenovo-ymc",
> +	},
> +	.id_table = lenovo_ymc_wmi_id_table,
> +	.probe = lenovo_ymc_probe,
> +	.remove = lenovo_ymc_remove,
> +	.notify = lenovo_ymc_notify,
> +};
> +
> +module_wmi_driver(lenovo_ymc_driver);
> +
> +MODULE_AUTHOR("Gergo Koteles <soyer@irl.hu>");
> +MODULE_DESCRIPTION("Lenovo Yoga Mode Control driver");
> +MODULE_LICENSE("GPL");

I missed Sob from this patch. I add it here.

Signed-off-by: Gergo Koteles <soyer@irl.hu>

Re: [PATCH 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Gergo Koteles]

On 2023. 03. 22. 22:03, Andrew Kallmeyer wrote:
> On Wed, Mar 22, 2023 at 1:39 PM Gergo Koteles <soyer@irl.hu> wrote:
>>
>> According to Hans, it's unrealistic that Lenovo will change this
>> triggering behavior, so it can remain.
> 
> In that case I will leave you as the module and commit author but add
> your signed-off, unless you'd still prefer not to be the module
> author. You wrote most of this code, I only did a few code review
> changes :)

Alright.