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

[PATCH v2 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.

Gergo and I have tested this on the Yoga 7 14ARB7, and the Yoga 7 14AIL7
respectively. Additionally, according to reports at
https://github.com/lukas-w/yoga-usage-mode, which uses the same WMI devices,
this driver should work with:
Yoga C940, Ideapad flex 14API, Yoga 9 14IAP7, Yoga 7 14ARB7, etc.

The diff since v1 is as follows:
v1: https://lore.kernel.org/platform-driver-x86/20230310041726.217447-1-kallmeyeras@gmail.com/

--- a/drivers/platform/x86/lenovo-ymc.c
+++ b/drivers/platform/x86/lenovo-ymc.c
@@ -18,7 +18,7 @@
 #define LENOVO_YMC_QUERY_GUID	"09B0EE6E-C3FD-4243-8DA1-7911FF80BB8C"
 
 #define LENOVO_YMC_QUERY_INSTANCE 0
-#define LENOVO_YMC_QUERY_METHOD 0xAB
+#define LENOVO_YMC_QUERY_METHOD 0x01
 
 static bool ec_trigger __read_mostly;
 module_param(ec_trigger, bool, 0644);
@@ -103,6 +103,12 @@ static void lenovo_ymc_notify(struct wmi_device *wdev, union acpi_object *data)
 	lenovo_ymc_trigger_ec(wdev, priv);
 }
 
+static void lenovo_ymc_remove(struct wmi_device *wdev)
+{
+	struct lenovo_ymc_private *priv = dev_get_drvdata(&wdev->dev);
+	acpi_dev_put(priv->ec_acpi_dev);
+}
+
 static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
 {
 	struct input_dev *input_dev;
@@ -117,7 +123,7 @@ static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
 
 	if (ec_trigger) {
 		pr_debug("Lenovo YMC enable EC triggering.\n");
-		priv->ec_acpi_dev = acpi_dev_get_first_match_dev("VCP2004", NULL, -1);
+		priv->ec_acpi_dev = acpi_dev_get_first_match_dev("VPC2004", NULL, -1);
 		if (!priv->ec_acpi_dev) {
 			dev_err(&wdev->dev, "Could not find EC ACPI device.\n");
 			return -ENODEV;
@@ -151,7 +157,10 @@ static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
 
 	priv->input_dev = input_dev;
 	dev_set_drvdata(&wdev->dev, priv);
+
+	// Report the state for the first time on probe
 	lenovo_ymc_trigger_ec(wdev, priv);
+	lenovo_ymc_notify(wdev, NULL);
 	return 0;
 }
 
@@ -168,6 +177,7 @@ static struct wmi_driver lenovo_ymc_driver = {
 	.id_table = lenovo_ymc_wmi_id_table,
 	.probe = lenovo_ymc_probe,
 	.notify = lenovo_ymc_notify,
+	.remove = lenovo_ymc_remove,
 };
 
 module_wmi_driver(lenovo_ymc_driver);

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     | 187 ++++++++++++++++++++++++++
 5 files changed, 351 insertions(+), 134 deletions(-)
 create mode 100644 drivers/platform/x86/ideapad-laptop.h
 create mode 100644 drivers/platform/x86/lenovo-ymc.c

-- 
2.40.0

[PATCH v2 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.

Signed-off-by: Andrew Kallmeyer <kallmeyeras@gmail.com>
Link: https://lore.kernel.org/r/20221004214332.35934-1-soyer@irl.hu/
---
 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.40.0

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

[Andrew Kallmeyer]

From: Gergo Koteles <soyer@irl.hu>

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.

Andrew followed the suggestions that were posted in reply to Gergo's RFC
patch, and on the v1 version of this patch to follow-up and get it
merged.

Changes from Gergo's RFC:

 - Refactored obtaining a reference to the EC ACPI device needed for the
   quirk implementation as suggested by Hans de Goede
 - Applied small fixes and switched to devm_input_allocate_device() and
   removing input_unregister_device() as suggested by Barnabás Pőcze.
 - Merged the lenovo_ymc_trigger_ec function with the
   ideapad_trigger_ymc_next_read function since it was no longer
   external.
 - Added the word "Tablet" to the driver description to hopefully make
   it more clear.
 - Fixed the LENOVO_YMC_QUERY_METHOD ID and the name string for the EC
   APCI device trigged for the quirk
 - Triggered the input event on probe so that the initial tablet mode
   state when the driver is loaded is reported to userspace as suggested
   by Armin Wolf.

We have tested this on the Yoga 7 14AIL7 for the non-quirk path and on
the Yoga 7 14ARB7 which has the firmware bug that requires triggering
the embedded controller to send the mode change events. This workaround
is also used by the Windows drivers.

According to reports at https://github.com/lukas-w/yoga-usage-mode,
which uses the same WMI devices, the following models should also work:
Yoga C940, Ideapad flex 14API, Yoga 9 14IAP7, Yoga 7 14ARB7, etc.

Signed-off-by: Gergo Koteles <soyer@irl.hu>
Co-developed-by: Andrew Kallmeyer <kallmeyeras@gmail.com>
Signed-off-by: Andrew Kallmeyer <kallmeyeras@gmail.com>
Link: https://lore.kernel.org/r/20221004214332.35934-1-soyer@irl.hu/
Link: https://lore.kernel.org/platform-driver-x86/20230310041726.217447-1-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     | 187 ++++++++++++++++++++++++++
 4 files changed, 199 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..c64f7ec85
--- /dev/null
+++ b/drivers/platform/x86/lenovo-ymc.c
@@ -0,0 +1,187 @@
+// 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 0x01
+
+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 void lenovo_ymc_remove(struct wmi_device *wdev)
+{
+	struct lenovo_ymc_private *priv = dev_get_drvdata(&wdev->dev);
+	acpi_dev_put(priv->ec_acpi_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;
+
+	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("VPC2004", 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);
+
+	// Report the state for the first time on probe
+	lenovo_ymc_trigger_ec(wdev, priv);
+	lenovo_ymc_notify(wdev, NULL);
+	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,
+	.remove = lenovo_ymc_remove,
+};
+
+module_wmi_driver(lenovo_ymc_driver);
+
+MODULE_AUTHOR("Gergo Koteles <soyer@irl.hu>");
+MODULE_DESCRIPTION("Lenovo Yoga Mode Control driver");
+MODULE_LICENSE("GPL");
-- 
2.40.0

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

[Barnabás Pőcze]

Hi


2023. március 23., csütörtök 3:52 keltezéssel, Andrew Kallmeyer <kallmeyeras@gmail.com> írta:

> From: Gergo Koteles <soyer@irl.hu>
> 
> 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.
> 
> Andrew followed the suggestions that were posted in reply to Gergo's RFC
> patch, and on the v1 version of this patch to follow-up and get it
> merged.
> 
> Changes from Gergo's RFC:
> 
>  - Refactored obtaining a reference to the EC ACPI device needed for the
>    quirk implementation as suggested by Hans de Goede
>  - Applied small fixes and switched to devm_input_allocate_device() and
>    removing input_unregister_device() as suggested by Barnabás Pőcze.
>  - Merged the lenovo_ymc_trigger_ec function with the
>    ideapad_trigger_ymc_next_read function since it was no longer
>    external.
>  - Added the word "Tablet" to the driver description to hopefully make
>    it more clear.
>  - Fixed the LENOVO_YMC_QUERY_METHOD ID and the name string for the EC
>    APCI device trigged for the quirk
>  - Triggered the input event on probe so that the initial tablet mode
>    state when the driver is loaded is reported to userspace as suggested
>    by Armin Wolf.
> 
> We have tested this on the Yoga 7 14AIL7 for the non-quirk path and on
> the Yoga 7 14ARB7 which has the firmware bug that requires triggering
> the embedded controller to send the mode change events. This workaround
> is also used by the Windows drivers.
> 
> According to reports at https://github.com/lukas-w/yoga-usage-mode,
> which uses the same WMI devices, the following models should also work:
> Yoga C940, Ideapad flex 14API, Yoga 9 14IAP7, Yoga 7 14ARB7, etc.
> 
> Signed-off-by: Gergo Koteles <soyer@irl.hu>
> Co-developed-by: Andrew Kallmeyer <kallmeyeras@gmail.com>
> Signed-off-by: Andrew Kallmeyer <kallmeyeras@gmail.com>
> Link: https://lore.kernel.org/r/20221004214332.35934-1-soyer@irl.hu/
> Link: https://lore.kernel.org/platform-driver-x86/20230310041726.217447-1-kallmeyeras@gmail.com/
> ---
> [...]
> +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)

I think just the `!priv->ec_acpi_dev` check is sufficient.


> +		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 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("VPC2004", NULL, -1);

The reference is leaked in case of an error. Use `devm_add_action_or_reset()`:

  static void acpi_dev_put_helper(void *p) { acpi_dev_put(p); }
  [...]
  priv->ec_acpi_dev = acpi_dev_get_first_match_dev("VPC2004", NULL, -1);
  if (!priv->ec_acpi_dev)
    ...
  err = devm_add_action_or_reset(&wdev->dev, acpi_dev_put_helper, priv->ec_acpi_dev)
  if (err)
    ...

And then you can remove `lenovo_ymc_remove()` altogether.


> +		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);

As far as I can tell `sparse_keymap_setup()` already sets the above.


> +
> +	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);
> +
> +	// Report the state for the first time on probe
> +	lenovo_ymc_trigger_ec(wdev, priv);
> +	lenovo_ymc_notify(wdev, NULL);
> +	return 0;
> +}
> [...]


Regards,
Barnabás Pőcze

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

[André Apitzsch]

Am Mittwoch, dem 22.03.2023 um 19:51 -0700 schrieb 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.
> 
> Gergo and I have tested this on the Yoga 7 14ARB7, and the Yoga 7 14AIL7
> respectively. Additionally, according to reports at
> https://github.com/lukas-w/yoga-usage-mode, which uses the same WMI devices,
> this driver should work with:
> Yoga C940, Ideapad flex 14API, Yoga 9 14IAP7, Yoga 7 14ARB7, etc.
> 
> [..]
> 
> 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     | 187 ++++++++++++++++++++++++++
>  5 files changed, 351 insertions(+), 134 deletions(-)
>  create mode 100644 drivers/platform/x86/ideapad-laptop.h
>  create mode 100644 drivers/platform/x86/lenovo-ymc.c
> 

Hi Andrew,

thank you for working on this.

Tested on Lenovo ThinkBook 14s Yoga ITL.

Tested-by: André Apitzsch <git@apitzsch.eu>

Best regards,
André

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

[Armin Wolf]

Am 23.03.23 um 03:52 schrieb Andrew Kallmeyer:

> From: Gergo Koteles <soyer@irl.hu>
>
> 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.
>
> Andrew followed the suggestions that were posted in reply to Gergo's RFC
> patch, and on the v1 version of this patch to follow-up and get it
> merged.
>
> Changes from Gergo's RFC:
>
>   - Refactored obtaining a reference to the EC ACPI device needed for the
>     quirk implementation as suggested by Hans de Goede
>   - Applied small fixes and switched to devm_input_allocate_device() and
>     removing input_unregister_device() as suggested by Barnabás Pőcze.
>   - Merged the lenovo_ymc_trigger_ec function with the
>     ideapad_trigger_ymc_next_read function since it was no longer
>     external.
>   - Added the word "Tablet" to the driver description to hopefully make
>     it more clear.
>   - Fixed the LENOVO_YMC_QUERY_METHOD ID and the name string for the EC
>     APCI device trigged for the quirk
>   - Triggered the input event on probe so that the initial tablet mode
>     state when the driver is loaded is reported to userspace as suggested
>     by Armin Wolf.
>
> We have tested this on the Yoga 7 14AIL7 for the non-quirk path and on
> the Yoga 7 14ARB7 which has the firmware bug that requires triggering
> the embedded controller to send the mode change events. This workaround
> is also used by the Windows drivers.
>
> According to reports at https://github.com/lukas-w/yoga-usage-mode,
> which uses the same WMI devices, the following models should also work:
> Yoga C940, Ideapad flex 14API, Yoga 9 14IAP7, Yoga 7 14ARB7, etc.
>
> Signed-off-by: Gergo Koteles <soyer@irl.hu>
> Co-developed-by: Andrew Kallmeyer <kallmeyeras@gmail.com>
> Signed-off-by: Andrew Kallmeyer <kallmeyeras@gmail.com>
> Link: https://lore.kernel.org/r/20221004214332.35934-1-soyer@irl.hu/
> Link: https://lore.kernel.org/platform-driver-x86/20230310041726.217447-1-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     | 187 ++++++++++++++++++++++++++
>   4 files changed, 199 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..c64f7ec85
> --- /dev/null
> +++ b/drivers/platform/x86/lenovo-ymc.c
> @@ -0,0 +1,187 @@
> +// 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 0x01
> +
> +static bool ec_trigger __read_mostly;
> +module_param(ec_trigger, bool, 0644);
> +MODULE_PARM_DESC(ec_trigger, "Enable EC triggering to emit YMC events");

Hi,

is it really necessary to allow userspace to read/write ec_trigger? The ACPI device
used for triggering the EC is only initialized during probing, so changing the value
of ec_trigger will make no difference in such cases.

Maybe you could change module_param(ec_trigger, bool, 0644) to module_param(ec_trigger, bool, 0)?

Armin Wolf

> +
> +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 void lenovo_ymc_remove(struct wmi_device *wdev)
> +{
> +	struct lenovo_ymc_private *priv = dev_get_drvdata(&wdev->dev);
> +	acpi_dev_put(priv->ec_acpi_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;
> +
> +	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("VPC2004", 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);
> +
> +	// Report the state for the first time on probe
> +	lenovo_ymc_trigger_ec(wdev, priv);
> +	lenovo_ymc_notify(wdev, NULL);
> +	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,
> +	.remove = lenovo_ymc_remove,
> +};
> +
> +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 v2 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Andrew Kallmeyer]

On Sat, Mar 25, 2023 at 12:10 AM Armin Wolf <W_Armin@gmx.de> wrote:
>
> Hi,
>
> is it really necessary to allow userspace to read/write ec_trigger? The ACPI device
> used for triggering the EC is only initialized during probing, so changing the value
> of ec_trigger will make no difference in such cases.
>
> Maybe you could change module_param(ec_trigger, bool, 0644) to module_param(ec_trigger, bool, 0)?
>
> Armin Wolf

Great point, this is actually a regression from Gergo's original patch
that I didn't realize I caused. I believe the intention was that if
the quirk detection code doesn't have full coverage users can set the
parameter themselves. In Gergo's code it used the acpi_device from
ideapad-laptop.c which is always loaded if it exists. Now I only load
it if ec_trigger is true at probe time, I think I should update it to
load the acpi device always if it exists so that the user can set this
parameter at any time. I suppose I would just remove the if
(ec_trigger) (and the debug print) in the probe code when I load it.

That is, unless you think it is best to just patch in more models to
the quirk detection later and not provide a parameter. Barnabás
actually suggested removing the ec_trigger flag completely because
right now the code isn't relying on it, but I think that is a bug.

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

[Armin Wolf]

Am 25.03.23 um 16:16 schrieb Andrew Kallmeyer:

> On Sat, Mar 25, 2023 at 12:10 AM Armin Wolf <W_Armin@gmx.de> wrote:
>> Hi,
>>
>> is it really necessary to allow userspace to read/write ec_trigger? The ACPI device
>> used for triggering the EC is only initialized during probing, so changing the value
>> of ec_trigger will make no difference in such cases.
>>
>> Maybe you could change module_param(ec_trigger, bool, 0644) to module_param(ec_trigger, bool, 0)?
>>
>> Armin Wolf
> Great point, this is actually a regression from Gergo's original patch
> that I didn't realize I caused. I believe the intention was that if
> the quirk detection code doesn't have full coverage users can set the
> parameter themselves. In Gergo's code it used the acpi_device from
> ideapad-laptop.c which is always loaded if it exists. Now I only load
> it if ec_trigger is true at probe time, I think I should update it to
> load the acpi device always if it exists so that the user can set this
> parameter at any time. I suppose I would just remove the if
> (ec_trigger) (and the debug print) in the probe code when I load it.
>
> That is, unless you think it is best to just patch in more models to
> the quirk detection later and not provide a parameter. Barnabás
> actually suggested removing the ec_trigger flag completely because
> right now the code isn't relying on it, but I think that is a bug.

I think it is best to still provide this module param for people who need
it, but only allow enabling it when loading the module. This way userspace
should not be able to read/write ec_trigger after the module has been loaded.

Because of this, i believe the ACPI device should only be initialized when
the DMI table says its necessary or ec_trigger was set. So the current solution
is fine for me, except for ec_trigger being accessible to userspace after
the module was loaded.

Armin Wolf

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

[Hans de Goede]

Hi,

On 3/25/23 22:49, Armin Wolf wrote:
> Am 25.03.23 um 16:16 schrieb Andrew Kallmeyer:
> 
>> On Sat, Mar 25, 2023 at 12:10 AM Armin Wolf <W_Armin@gmx.de> wrote:
>>> Hi,
>>>
>>> is it really necessary to allow userspace to read/write ec_trigger? The ACPI device
>>> used for triggering the EC is only initialized during probing, so changing the value
>>> of ec_trigger will make no difference in such cases.
>>>
>>> Maybe you could change module_param(ec_trigger, bool, 0644) to module_param(ec_trigger, bool, 0)?
>>>
>>> Armin Wolf
>> Great point, this is actually a regression from Gergo's original patch
>> that I didn't realize I caused. I believe the intention was that if
>> the quirk detection code doesn't have full coverage users can set the
>> parameter themselves. In Gergo's code it used the acpi_device from
>> ideapad-laptop.c which is always loaded if it exists. Now I only load
>> it if ec_trigger is true at probe time, I think I should update it to
>> load the acpi device always if it exists so that the user can set this
>> parameter at any time. I suppose I would just remove the if
>> (ec_trigger) (and the debug print) in the probe code when I load it.
>>
>> That is, unless you think it is best to just patch in more models to
>> the quirk detection later and not provide a parameter. Barnabás
>> actually suggested removing the ec_trigger flag completely because
>> right now the code isn't relying on it, but I think that is a bug.
> 
> I think it is best to still provide this module param for people who need
> it, but only allow enabling it when loading the module. This way userspace
> should not be able to read/write ec_trigger after the module has been loaded.
> 
> Because of this, i believe the ACPI device should only be initialized when
> the DMI table says its necessary or ec_trigger was set. So the current solution
> is fine for me, except for ec_trigger being accessible to userspace after
> the module was loaded.

Right, ack.

For the next version please use 0444 and not 0 for the sysfs file rights on the file, this way users can still read the parameters under /sys/modules/.../parameters/ec_trigger to e.g. check if the parameter was set, which is useful to e.g. see if an /etc/modprobe.d/foo.conf dropin file is working as expected.

Also please address Barnabás' remarks. I have not taken a really close look myself yet, but given the close look others have already given this drivers I don't expect to find any issues.

So please prepare a v3 (when you have time) and then I'll try to merge that right away.

Regards,

Hans