[PATCH 0/4] platform/x86: ideapad-laptop: synchronize VPC commands

[PATCH 0/4] platform/x86: ideapad-laptop: synchronize VPC commands

[Gergo Koteles]

Hi,

Sometimes the Yoga mode control switch did not work properly on my 
laptop, and sometimes even caused a platform profile switch. It turned 
out that it was caused by a race situation, the lenovo-ymc wmi notify 
handler was running at the same time as the ideapad-laptop acpi notify 
handler, and the partial results of the VPC calls in the two were mixed 
up.

This series introduces a mutex and reorganizes the code a bit to 
eliminate this.

Best regards,
Gergo Koteles

Gergo Koteles (4):
  platform/x86: ideapad-laptop: use cleanup.h
  platform/x86: ideapad-laptop: add a mutex to synchronize VPC commands
  platform/x86: ideapad-laptop: move ymc_trigger_ec from lenovo-ymc
  platform/x86: ideapad-laptop: move ACPI helpers from header to source
    file

 drivers/platform/x86/Kconfig          |   1 +
 drivers/platform/x86/ideapad-laptop.c | 284 +++++++++++++++++++++-----
 drivers/platform/x86/ideapad-laptop.h | 140 +------------
 drivers/platform/x86/lenovo-ymc.c     |  34 +--
 4 files changed, 235 insertions(+), 224 deletions(-)

-- 
2.45.2

[PATCH 1/4] platform/x86: ideapad-laptop: use cleanup.h

[Gergo Koteles]

Use cleanup.h helpers to simplify some code paths.

Signed-off-by: Gergo Koteles <soyer@irl.hu>
---
 drivers/platform/x86/ideapad-laptop.c | 71 ++++++++++++---------------
 1 file changed, 31 insertions(+), 40 deletions(-)

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index fcf13d88fd6e..1ace711f7442 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -13,6 +13,7 @@
 #include <linux/bitfield.h>
 #include <linux/bitops.h>
 #include <linux/bug.h>
+#include <linux/cleanup.h>
 #include <linux/debugfs.h>
 #include <linux/device.h>
 #include <linux/dmi.h>
@@ -204,7 +205,7 @@ static int ideapad_shared_init(struct ideapad_private *priv)
 {
 	int ret;
 
-	mutex_lock(&ideapad_shared_mutex);
+	guard(mutex)(&ideapad_shared_mutex);
 
 	if (!ideapad_shared) {
 		ideapad_shared = priv;
@@ -214,19 +215,15 @@ static int ideapad_shared_init(struct ideapad_private *priv)
 		ret = -EINVAL;
 	}
 
-	mutex_unlock(&ideapad_shared_mutex);
-
 	return ret;
 }
 
 static void ideapad_shared_exit(struct ideapad_private *priv)
 {
-	mutex_lock(&ideapad_shared_mutex);
+	guard(mutex)(&ideapad_shared_mutex);
 
 	if (ideapad_shared == priv)
 		ideapad_shared = NULL;
-
-	mutex_unlock(&ideapad_shared_mutex);
 }
 
 /*
@@ -840,36 +837,33 @@ static int dytc_profile_set(struct platform_profile_handler *pprof,
 	unsigned long output;
 	int err;
 
-	err = mutex_lock_interruptible(&dytc->mutex);
-	if (err)
-		return err;
-
-	if (profile == PLATFORM_PROFILE_BALANCED) {
-		/* To get back to balanced mode we just issue a reset command */
-		err = eval_dytc(priv->adev->handle, DYTC_CMD_RESET, NULL);
-		if (err)
-			goto unlock;
-	} else {
-		int perfmode;
-
-		err = convert_profile_to_dytc(profile, &perfmode);
-		if (err)
-			goto unlock;
+	scoped_guard(mutex_intr, &dytc->mutex) {
+		if (profile == PLATFORM_PROFILE_BALANCED) {
+			/* To get back to balanced mode we just issue a reset command */
+			err = eval_dytc(priv->adev->handle, DYTC_CMD_RESET, NULL);
+			if (err)
+				return err;
+		} else {
+			int perfmode;
+
+			err = convert_profile_to_dytc(profile, &perfmode);
+			if (err)
+				return err;
+
+			/* Determine if we are in CQL mode. This alters the commands we do */
+			err = dytc_cql_command(priv,
+					       DYTC_SET_COMMAND(DYTC_FUNCTION_MMC, perfmode, 1),
+					       &output);
+			if (err)
+				return err;
+		}
 
-		/* Determine if we are in CQL mode. This alters the commands we do */
-		err = dytc_cql_command(priv, DYTC_SET_COMMAND(DYTC_FUNCTION_MMC, perfmode, 1),
-				       &output);
-		if (err)
-			goto unlock;
+		/* Success - update current profile */
+		dytc->current_profile = profile;
+		return 0;
 	}
 
-	/* Success - update current profile */
-	dytc->current_profile = profile;
-
-unlock:
-	mutex_unlock(&dytc->mutex);
-
-	return err;
+	return -EINTR;
 }
 
 static void dytc_profile_refresh(struct ideapad_private *priv)
@@ -878,9 +872,8 @@ static void dytc_profile_refresh(struct ideapad_private *priv)
 	unsigned long output;
 	int err, perfmode;
 
-	mutex_lock(&priv->dytc->mutex);
-	err = dytc_cql_command(priv, DYTC_CMD_GET, &output);
-	mutex_unlock(&priv->dytc->mutex);
+	scoped_guard(mutex, &priv->dytc->mutex)
+		err = dytc_cql_command(priv, DYTC_CMD_GET, &output);
 	if (err)
 		return;
 
@@ -1809,11 +1802,11 @@ static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
 	struct ideapad_wmi_private *wpriv = dev_get_drvdata(&wdev->dev);
 	struct ideapad_private *priv;
 
-	mutex_lock(&ideapad_shared_mutex);
+	guard(mutex)(&ideapad_shared_mutex);
 
 	priv = ideapad_shared;
 	if (!priv)
-		goto unlock;
+		return;
 
 	switch (wpriv->event) {
 	case IDEAPAD_WMI_EVENT_ESC:
@@ -1847,8 +1840,6 @@ static void ideapad_wmi_notify(struct wmi_device *wdev, union acpi_object *data)
 
 		break;
 	}
-unlock:
-	mutex_unlock(&ideapad_shared_mutex);
 }
 
 static const struct ideapad_wmi_private ideapad_wmi_context_esc = {
-- 
2.45.2

Re: [PATCH 1/4] platform/x86: ideapad-laptop: use cleanup.h

[Ilpo Järvinen]

On Tue, 9 Jul 2024, Gergo Koteles wrote:

> Use cleanup.h helpers to simplify some code paths.
> 
> Signed-off-by: Gergo Koteles <soyer@irl.hu>

Hi,

I decided to take this v1 of this cleanup one in this cycle into 
review-ilpo/for-next but left the other patches because of the outstanding 
comments on patches 2 and 3.

-- 
 i.

[PATCH 2/4] platform/x86: ideapad-laptop: add a mutex to synchronize VPC commands

[Gergo Koteles]

Calling VPC commands consists of several VPCW and VPCR ACPI calls.
These calls and their results can get mixed up if they are called
simultaneously from different threads, like acpi notify handler,
sysfs, debugfs, lenovo-ymc.

Add a mutex to synchronize VPC commands.

Signed-off-by: Gergo Koteles <soyer@irl.hu>
---
 drivers/platform/x86/ideapad-laptop.c | 59 +++++++++++++++++++--------
 1 file changed, 43 insertions(+), 16 deletions(-)

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 1ace711f7442..464cc5fd0088 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -126,6 +126,7 @@ struct ideapad_rfk_priv {
 
 struct ideapad_private {
 	struct acpi_device *adev;
+	struct mutex vpc_mutex; /* protects the VPC calls */
 	struct rfkill *rfk[IDEAPAD_RFKILL_DEV_NUM];
 	struct ideapad_rfk_priv rfk_priv[IDEAPAD_RFKILL_DEV_NUM];
 	struct platform_device *platform_device;
@@ -294,6 +295,8 @@ static int debugfs_status_show(struct seq_file *s, void *data)
 	struct ideapad_private *priv = s->private;
 	unsigned long value;
 
+	guard(mutex)(&priv->vpc_mutex);
+
 	if (!read_ec_data(priv->adev->handle, VPCCMD_R_BL_MAX, &value))
 		seq_printf(s, "Backlight max:  %lu\n", value);
 	if (!read_ec_data(priv->adev->handle, VPCCMD_R_BL, &value))
@@ -412,7 +415,8 @@ static ssize_t camera_power_show(struct device *dev,
 	unsigned long result;
 	int err;
 
-	err = read_ec_data(priv->adev->handle, VPCCMD_R_CAMERA, &result);
+	scoped_guard(mutex, &priv->vpc_mutex)
+		err = read_ec_data(priv->adev->handle, VPCCMD_R_CAMERA, &result);
 	if (err)
 		return err;
 
@@ -431,7 +435,8 @@ static ssize_t camera_power_store(struct device *dev,
 	if (err)
 		return err;
 
-	err = write_ec_cmd(priv->adev->handle, VPCCMD_W_CAMERA, state);
+	scoped_guard(mutex, &priv->vpc_mutex)
+		err = write_ec_cmd(priv->adev->handle, VPCCMD_W_CAMERA, state);
 	if (err)
 		return err;
 
@@ -484,7 +489,8 @@ static ssize_t fan_mode_show(struct device *dev,
 	unsigned long result;
 	int err;
 
-	err = read_ec_data(priv->adev->handle, VPCCMD_R_FAN, &result);
+	scoped_guard(mutex, &priv->vpc_mutex)
+		err = read_ec_data(priv->adev->handle, VPCCMD_R_FAN, &result);
 	if (err)
 		return err;
 
@@ -506,7 +512,8 @@ static ssize_t fan_mode_store(struct device *dev,
 	if (state > 4 || state == 3)
 		return -EINVAL;
 
-	err = write_ec_cmd(priv->adev->handle, VPCCMD_W_FAN, state);
+	scoped_guard(mutex, &priv->vpc_mutex)
+		err = write_ec_cmd(priv->adev->handle, VPCCMD_W_FAN, state);
 	if (err)
 		return err;
 
@@ -591,7 +598,8 @@ static ssize_t touchpad_show(struct device *dev,
 	unsigned long result;
 	int err;
 
-	err = read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &result);
+	scoped_guard(mutex, &priv->vpc_mutex)
+		err = read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &result);
 	if (err)
 		return err;
 
@@ -612,7 +620,8 @@ static ssize_t touchpad_store(struct device *dev,
 	if (err)
 		return err;
 
-	err = write_ec_cmd(priv->adev->handle, VPCCMD_W_TOUCHPAD, state);
+	scoped_guard(mutex, &priv->vpc_mutex)
+		err = write_ec_cmd(priv->adev->handle, VPCCMD_W_TOUCHPAD, state);
 	if (err)
 		return err;
 
@@ -1005,6 +1014,8 @@ static int ideapad_rfk_set(void *data, bool blocked)
 	struct ideapad_rfk_priv *priv = data;
 	int opcode = ideapad_rfk_data[priv->dev].opcode;
 
+	guard(mutex)(&priv->priv->vpc_mutex);
+
 	return write_ec_cmd(priv->priv->adev->handle, opcode, !blocked);
 }
 
@@ -1018,6 +1029,8 @@ static void ideapad_sync_rfk_state(struct ideapad_private *priv)
 	int i;
 
 	if (priv->features.hw_rfkill_switch) {
+		guard(mutex)(&priv->vpc_mutex);
+
 		if (read_ec_data(priv->adev->handle, VPCCMD_R_RF, &hw_blocked))
 			return;
 		hw_blocked = !hw_blocked;
@@ -1191,8 +1204,9 @@ static void ideapad_input_novokey(struct ideapad_private *priv)
 {
 	unsigned long long_pressed;
 
-	if (read_ec_data(priv->adev->handle, VPCCMD_R_NOVO, &long_pressed))
-		return;
+	scoped_guard(mutex, &priv->vpc_mutex)
+		if (read_ec_data(priv->adev->handle, VPCCMD_R_NOVO, &long_pressed))
+			return;
 
 	if (long_pressed)
 		ideapad_input_report(priv, 17);
@@ -1204,8 +1218,9 @@ static void ideapad_check_special_buttons(struct ideapad_private *priv)
 {
 	unsigned long bit, value;
 
-	if (read_ec_data(priv->adev->handle, VPCCMD_R_SPECIAL_BUTTONS, &value))
-		return;
+	scoped_guard(mutex, &priv->vpc_mutex)
+		if (read_ec_data(priv->adev->handle, VPCCMD_R_SPECIAL_BUTTONS, &value))
+			return;
 
 	for_each_set_bit (bit, &value, 16) {
 		switch (bit) {
@@ -1238,6 +1253,8 @@ static int ideapad_backlight_get_brightness(struct backlight_device *blightdev)
 	unsigned long now;
 	int err;
 
+	guard(mutex)(&priv->vpc_mutex);
+
 	err = read_ec_data(priv->adev->handle, VPCCMD_R_BL, &now);
 	if (err)
 		return err;
@@ -1250,6 +1267,8 @@ static int ideapad_backlight_update_status(struct backlight_device *blightdev)
 	struct ideapad_private *priv = bl_get_data(blightdev);
 	int err;
 
+	guard(mutex)(&priv->vpc_mutex);
+
 	err = write_ec_cmd(priv->adev->handle, VPCCMD_W_BL,
 			   blightdev->props.brightness);
 	if (err)
@@ -1327,6 +1346,8 @@ static void ideapad_backlight_notify_power(struct ideapad_private *priv)
 	if (!blightdev)
 		return;
 
+	guard(mutex)(&priv->vpc_mutex);
+
 	if (read_ec_data(priv->adev->handle, VPCCMD_R_BL_POWER, &power))
 		return;
 
@@ -1339,7 +1360,8 @@ static void ideapad_backlight_notify_brightness(struct ideapad_private *priv)
 
 	/* if we control brightness via acpi video driver */
 	if (!priv->blightdev)
-		read_ec_data(priv->adev->handle, VPCCMD_R_BL, &now);
+		scoped_guard(mutex, &priv->vpc_mutex)
+			read_ec_data(priv->adev->handle, VPCCMD_R_BL, &now);
 	else
 		backlight_force_update(priv->blightdev, BACKLIGHT_UPDATE_HOTKEY);
 }
@@ -1564,7 +1586,8 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_
 	int ret;
 
 	/* Without reading from EC touchpad LED doesn't switch state */
-	ret = read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &value);
+	scoped_guard(mutex, &priv->vpc_mutex)
+		ret = read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &value);
 	if (ret)
 		return;
 
@@ -1597,11 +1620,13 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
 	struct ideapad_private *priv = data;
 	unsigned long vpc1, vpc2, bit;
 
-	if (read_ec_data(handle, VPCCMD_R_VPC1, &vpc1))
-		return;
+	scoped_guard(mutex, &priv->vpc_mutex) {
+		if (read_ec_data(handle, VPCCMD_R_VPC1, &vpc1))
+			return;
 
-	if (read_ec_data(handle, VPCCMD_R_VPC2, &vpc2))
-		return;
+		if (read_ec_data(handle, VPCCMD_R_VPC2, &vpc2))
+			return;
+	}
 
 	vpc1 = (vpc2 << 8) | vpc1;
 
@@ -1906,6 +1931,8 @@ static int ideapad_acpi_add(struct platform_device *pdev)
 	priv->adev = adev;
 	priv->platform_device = pdev;
 
+	mutex_init(&priv->vpc_mutex);
+
 	ideapad_check_features(priv);
 
 	err = ideapad_sysfs_init(priv);
-- 
2.45.2

[PATCH 3/4] platform/x86: ideapad-laptop: move ymc_trigger_ec from lenovo-ymc

[Gergo Koteles]

Some models need to trigger the EC for the yoga mode control to work
properly. EC triggering consists of a VPC call that needs to be
synchronized. The vpc_mutex is in the ideapad-laptop module, so
synchronization is easier there.

Move the ymc_trigger_ec function to the ideapad-laptop module.

Signed-off-by: Gergo Koteles <soyer@irl.hu>
---
 drivers/platform/x86/Kconfig          |  1 +
 drivers/platform/x86/ideapad-laptop.c | 18 ++++++++++++++
 drivers/platform/x86/ideapad-laptop.h |  2 ++
 drivers/platform/x86/lenovo-ymc.c     | 34 ++++-----------------------
 4 files changed, 26 insertions(+), 29 deletions(-)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 665fa9524986..ddfccc226751 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -477,6 +477,7 @@ 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
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 464cc5fd0088..4dcdbb153dda 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -1615,6 +1615,24 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_
 	priv->r_touchpad_val = value;
 }
 
+void ideapad_ymc_trigger_ec(void)
+{
+	struct ideapad_private *priv;
+	int ret;
+
+	guard(mutex)(&ideapad_shared_mutex);
+
+	priv = ideapad_shared;
+	if (!priv)
+		return;
+
+	scoped_guard(mutex, &priv->vpc_mutex)
+		ret = write_ec_cmd(priv->adev->handle, VPCCMD_W_YMC, 1);
+	if (ret)
+		dev_warn(&priv->platform_device->dev, "Could not write YMC: %d\n", ret);
+}
+EXPORT_SYMBOL_GPL(ideapad_ymc_trigger_ec);
+
 static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct ideapad_private *priv = data;
diff --git a/drivers/platform/x86/ideapad-laptop.h b/drivers/platform/x86/ideapad-laptop.h
index 4498a96de597..4dcbed842b0b 100644
--- a/drivers/platform/x86/ideapad-laptop.h
+++ b/drivers/platform/x86/ideapad-laptop.h
@@ -13,6 +13,8 @@
 #include <linux/jiffies.h>
 #include <linux/errno.h>
 
+void ideapad_ymc_trigger_ec(void);
+
 enum {
 	VPCCMD_R_VPC1 = 0x10,
 	VPCCMD_R_BL_MAX,
diff --git a/drivers/platform/x86/lenovo-ymc.c b/drivers/platform/x86/lenovo-ymc.c
index e1fbc35504d4..0819e9823ca9 100644
--- a/drivers/platform/x86/lenovo-ymc.c
+++ b/drivers/platform/x86/lenovo-ymc.c
@@ -62,19 +62,14 @@ static const struct dmi_system_id allowed_chasis_types_dmi_table[] = {
 
 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)
+static void lenovo_ymc_trigger_ec(void)
 {
-	int err;
-
-	if (!priv->ec_acpi_dev)
+	if (!ec_trigger)
 		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);
+	ideapad_ymc_trigger_ec();
 }
 
 static const struct key_entry lenovo_ymc_keymap[] = {
@@ -125,11 +120,9 @@ static void lenovo_ymc_notify(struct wmi_device *wdev, union acpi_object *data)
 
 free_obj:
 	kfree(obj);
-	lenovo_ymc_trigger_ec(wdev, priv);
+	lenovo_ymc_trigger_ec();
 }
 
-static void acpi_dev_put_helper(void *p) { acpi_dev_put(p); }
-
 static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
 {
 	struct lenovo_ymc_private *priv;
@@ -149,23 +142,6 @@ static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
 	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;
-		}
-		err = devm_add_action_or_reset(&wdev->dev,
-				acpi_dev_put_helper, priv->ec_acpi_dev);
-		if (err) {
-			dev_err(&wdev->dev,
-				"Could not clean up EC ACPI device: %d\n", err);
-			return err;
-		}
-	}
-
 	input_dev = devm_input_allocate_device(&wdev->dev);
 	if (!input_dev)
 		return -ENOMEM;
@@ -192,7 +168,7 @@ static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
 	dev_set_drvdata(&wdev->dev, priv);
 
 	/* Report the state for the first time on probe */
-	lenovo_ymc_trigger_ec(wdev, priv);
+	lenovo_ymc_trigger_ec();
 	lenovo_ymc_notify(wdev, NULL);
 	return 0;
 }
-- 
2.45.2

Re: [PATCH 3/4] platform/x86: ideapad-laptop: move ymc_trigger_ec from lenovo-ymc

[Ilpo Järvinen]

On Tue, 9 Jul 2024, Gergo Koteles wrote:

> Some models need to trigger the EC for the yoga mode control to work
> properly. EC triggering consists of a VPC call that needs to be
> synchronized. The vpc_mutex is in the ideapad-laptop module, so
> synchronization is easier there.
> 
> Move the ymc_trigger_ec function to the ideapad-laptop module.
> 
> Signed-off-by: Gergo Koteles <soyer@irl.hu>
> ---
>  drivers/platform/x86/Kconfig          |  1 +
>  drivers/platform/x86/ideapad-laptop.c | 18 ++++++++++++++
>  drivers/platform/x86/ideapad-laptop.h |  2 ++
>  drivers/platform/x86/lenovo-ymc.c     | 34 ++++-----------------------
>  4 files changed, 26 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 665fa9524986..ddfccc226751 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -477,6 +477,7 @@ 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
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index 464cc5fd0088..4dcdbb153dda 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -1615,6 +1615,24 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_
>  	priv->r_touchpad_val = value;
>  }
>  
> +void ideapad_ymc_trigger_ec(void)
> +{
> +	struct ideapad_private *priv;
> +	int ret;
> +
> +	guard(mutex)(&ideapad_shared_mutex);
> +
> +	priv = ideapad_shared;
> +	if (!priv)
> +		return;
> +
> +	scoped_guard(mutex, &priv->vpc_mutex)

It feels the ordering in this series is off because patch 2 add the 
mutex and here you add it into yet another function. If this patch 
would be before the other, the mutex changes could all go into the same 
patch.

> +		ret = write_ec_cmd(priv->adev->handle, VPCCMD_W_YMC, 1);
> +	if (ret)
> +		dev_warn(&priv->platform_device->dev, "Could not write YMC: %d\n", ret);
> +}
> +EXPORT_SYMBOL_GPL(ideapad_ymc_trigger_ec);

This export should be namespaced.

I'll have to say I can accept this approach only grudgingly... it feels 
off even if I understand why you have to do this cross driver thing.

-- 
 i.

>  static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
>  {
>  	struct ideapad_private *priv = data;
> diff --git a/drivers/platform/x86/ideapad-laptop.h b/drivers/platform/x86/ideapad-laptop.h
> index 4498a96de597..4dcbed842b0b 100644
> --- a/drivers/platform/x86/ideapad-laptop.h
> +++ b/drivers/platform/x86/ideapad-laptop.h
> @@ -13,6 +13,8 @@
>  #include <linux/jiffies.h>
>  #include <linux/errno.h>
>  
> +void ideapad_ymc_trigger_ec(void);
> +
>  enum {
>  	VPCCMD_R_VPC1 = 0x10,
>  	VPCCMD_R_BL_MAX,
> diff --git a/drivers/platform/x86/lenovo-ymc.c b/drivers/platform/x86/lenovo-ymc.c
> index e1fbc35504d4..0819e9823ca9 100644
> --- a/drivers/platform/x86/lenovo-ymc.c
> +++ b/drivers/platform/x86/lenovo-ymc.c
> @@ -62,19 +62,14 @@ static const struct dmi_system_id allowed_chasis_types_dmi_table[] = {
>  
>  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)
> +static void lenovo_ymc_trigger_ec(void)
>  {
> -	int err;
> -
> -	if (!priv->ec_acpi_dev)
> +	if (!ec_trigger)
>  		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);
> +	ideapad_ymc_trigger_ec();
>  }
>  
>  static const struct key_entry lenovo_ymc_keymap[] = {
> @@ -125,11 +120,9 @@ static void lenovo_ymc_notify(struct wmi_device *wdev, union acpi_object *data)
>  
>  free_obj:
>  	kfree(obj);
> -	lenovo_ymc_trigger_ec(wdev, priv);
> +	lenovo_ymc_trigger_ec();
>  }
>  
> -static void acpi_dev_put_helper(void *p) { acpi_dev_put(p); }
> -
>  static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
>  {
>  	struct lenovo_ymc_private *priv;
> @@ -149,23 +142,6 @@ static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
>  	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;
> -		}
> -		err = devm_add_action_or_reset(&wdev->dev,
> -				acpi_dev_put_helper, priv->ec_acpi_dev);
> -		if (err) {
> -			dev_err(&wdev->dev,
> -				"Could not clean up EC ACPI device: %d\n", err);
> -			return err;
> -		}
> -	}
> -
>  	input_dev = devm_input_allocate_device(&wdev->dev);
>  	if (!input_dev)
>  		return -ENOMEM;
> @@ -192,7 +168,7 @@ static int lenovo_ymc_probe(struct wmi_device *wdev, const void *ctx)
>  	dev_set_drvdata(&wdev->dev, priv);
>  
>  	/* Report the state for the first time on probe */
> -	lenovo_ymc_trigger_ec(wdev, priv);
> +	lenovo_ymc_trigger_ec();
>  	lenovo_ymc_notify(wdev, NULL);
>  	return 0;
>  }
> 

Re: [PATCH 3/4] platform/x86: ideapad-laptop: move ymc_trigger_ec from lenovo-ymc

[Gergo Koteles]

Hi Ilpo,

Thanks for the review.

On Thu, 2024-07-11 at 11:18 +0300, Ilpo Järvinen wrote:
> On Tue, 9 Jul 2024, Gergo Koteles wrote:
> 
> > Some models need to trigger the EC for the yoga mode control to work
> > properly. EC triggering consists of a VPC call that needs to be
> > synchronized. The vpc_mutex is in the ideapad-laptop module, so
> > synchronization is easier there.
> > 
> > Move the ymc_trigger_ec function to the ideapad-laptop module.
> > 
> > Signed-off-by: Gergo Koteles <soyer@irl.hu>
> > ---
> >  drivers/platform/x86/Kconfig          |  1 +
> >  drivers/platform/x86/ideapad-laptop.c | 18 ++++++++++++++
> >  drivers/platform/x86/ideapad-laptop.h |  2 ++
> >  drivers/platform/x86/lenovo-ymc.c     | 34 ++++-----------------------
> >  4 files changed, 26 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index 665fa9524986..ddfccc226751 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -477,6 +477,7 @@ 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
> > diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> > index 464cc5fd0088..4dcdbb153dda 100644
> > --- a/drivers/platform/x86/ideapad-laptop.c
> > +++ b/drivers/platform/x86/ideapad-laptop.c
> > @@ -1615,6 +1615,24 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_
> >  	priv->r_touchpad_val = value;
> >  }
> >  
> > +void ideapad_ymc_trigger_ec(void)
> > +{
> > +	struct ideapad_private *priv;
> > +	int ret;
> > +
> > +	guard(mutex)(&ideapad_shared_mutex);
> > +
> > +	priv = ideapad_shared;
> > +	if (!priv)
> > +		return;
> > +
> > +	scoped_guard(mutex, &priv->vpc_mutex)
> 
> It feels the ordering in this series is off because patch 2 add the 
> mutex and here you add it into yet another function. If this patch 
> would be before the other, the mutex changes could all go into the same 
> patch.
> 

I (wrongly) thought it would be easier to explain why a new dependency
is needed. 
Mutex changes go into one patch in the v2.


> > +		ret = write_ec_cmd(priv->adev->handle, VPCCMD_W_YMC, 1);
> > +	if (ret)
> > +		dev_warn(&priv->platform_device->dev, "Could not write YMC: %d\n", ret);
> > +}
> > +EXPORT_SYMBOL_GPL(ideapad_ymc_trigger_ec);
> 
> This export should be namespaced.

It will be in the v2.

> 
> I'll have to say I can accept this approach only grudgingly... it feels 
> off even if I understand why you have to do this cross driver thing.
> 

Based on the WMI driver development guide, I will add a notification
chain in v2 soon. Looks better.

Best Regards,
Gergo Koteles

[PATCH 4/4] platform/x86: ideapad-laptop: move ACPI helpers from header to source file

[Gergo Koteles]

Since moving ymc_trigger_ec from lenovo-ymc to ideapad-laptop, only the
latter uses these definitions and functions.

Move them back to source file.

Signed-off-by: Gergo Koteles <soyer@irl.hu>
---
 drivers/platform/x86/ideapad-laptop.c | 136 +++++++++++++++++++++++++
 drivers/platform/x86/ideapad-laptop.h | 140 --------------------------
 2 files changed, 136 insertions(+), 140 deletions(-)

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 4dcdbb153dda..33608e78e653 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -22,6 +22,7 @@
 #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>
@@ -87,6 +88,34 @@ 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_W_YMC = 0x2A,
+	VPCCMD_R_FAN = 0x2B,
+	VPCCMD_R_SPECIAL_BUTTONS = 0x31,
+	VPCCMD_W_BL_POWER = 0x33,
+};
+
 /*
  * These correspond to the number of supported states - 1
  * Future keyboard types may need a new system, if there's a collision
@@ -230,6 +259,7 @@ 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)
 {
@@ -245,6 +275,29 @@ static int eval_int(acpi_handle handle, const char *name, unsigned long *res)
 	return 0;
 }
 
+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 exec_simple_method(acpi_handle handle, const char *name, unsigned long arg)
 {
 	acpi_status status = acpi_execute_simple_method(handle, (char *)name, arg);
@@ -287,6 +340,89 @@ 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
index 4dcbed842b0b..38233896ad2b 100644
--- a/drivers/platform/x86/ideapad-laptop.h
+++ b/drivers/platform/x86/ideapad-laptop.h
@@ -9,146 +9,6 @@
 #ifndef _IDEAPAD_LAPTOP_H_
 #define _IDEAPAD_LAPTOP_H_
 
-#include <linux/acpi.h>
-#include <linux/jiffies.h>
-#include <linux/errno.h>
-
 void ideapad_ymc_trigger_ec(void);
 
-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_W_YMC = 0x2A,
-	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.45.2