[PATCH 0/3] platform/x86: ideapad-laptop: Rework touchpad control code

[PATCH 0/3] platform/x86: ideapad-laptop: Rework touchpad control code

[Hans de Goede]

Hi All,

Here are my proposed changes from the "ideapad-laptop touchpad handling
problems, request for help" email thread as proper patches:
https://lore.kernel.org/platform-driver-x86/bc1202d1-d85d-4173-5979-237bb1ee9254@redhat.com/

Note this applies on top of my review-hans branch which has seen a bunch
of other ideapad-laptop changes land recently:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

As suggested by Maxim, the third patch now has a module parameter to
allow users to easily re-enable the i8042 aux-port enabling/disabling
on models other then the Z570.

Eray, you mentioned in another email that you have some concerns about
the approach in this series?

Regards,

Hans


Hans de Goede (3):
  platform/x86: ideapad-laptop: Refactor ideapad_sync_touchpad_state()
  platform/x86: ideapad-laptop: Do not send KEY_TOUCHPAD* events on
    probe / resume
  platform/x86: ideapad-laptop: Rework touchpad control code

 drivers/platform/x86/ideapad-laptop.c | 89 +++++++++++++++------------
 1 file changed, 49 insertions(+), 40 deletions(-)

-- 
2.38.1

[PATCH 1/3] platform/x86: ideapad-laptop: Refactor ideapad_sync_touchpad_state()

[Hans de Goede]

Add an error exit for read_ec_data() failing instead of putting the main
body in an if (success) block.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/ideapad-laptop.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 13e3ae731fd8..dcb3a82024da 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -1490,23 +1490,26 @@ static void ideapad_kbd_bl_exit(struct ideapad_private *priv)
 static void ideapad_sync_touchpad_state(struct ideapad_private *priv)
 {
 	unsigned long value;
+	unsigned char param;
+	int ret;
 
 	if (!priv->features.touchpad_ctrl_via_ec)
 		return;
 
 	/* Without reading from EC touchpad LED doesn't switch state */
-	if (!read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &value)) {
-		unsigned char param;
-		/*
-		 * Some IdeaPads don't really turn off touchpad - they only
-		 * switch the LED state. We (de)activate KBC AUX port to turn
-		 * touchpad off and on. We send KEY_TOUCHPAD_OFF and
-		 * KEY_TOUCHPAD_ON to not to get out of sync with LED
-		 */
-		i8042_command(&param, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE);
-		ideapad_input_report(priv, value ? 67 : 66);
-		sysfs_notify(&priv->platform_device->dev.kobj, NULL, "touchpad");
-	}
+	ret = read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &value);
+	if (ret)
+		return;
+
+	/*
+	 * Some IdeaPads don't really turn off touchpad - they only
+	 * switch the LED state. We (de)activate KBC AUX port to turn
+	 * touchpad off and on. We send KEY_TOUCHPAD_OFF and
+	 * KEY_TOUCHPAD_ON to not to get out of sync with LED
+	 */
+	i8042_command(&param, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE);
+	ideapad_input_report(priv, value ? 67 : 66);
+	sysfs_notify(&priv->platform_device->dev.kobj, NULL, "touchpad");
 }
 
 static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
-- 
2.38.1

[PATCH 2/3] platform/x86: ideapad-laptop: Do not send KEY_TOUCHPAD* events on probe / resume

[Hans de Goede]

The sending of KEY_TOUCHPAD* events is causing spurious touchpad OSD
showing on resume.

Disable the sending of events on probe / resume to fix this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/ideapad-laptop.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index dcb3a82024da..eb0b1ec32c13 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -1487,7 +1487,7 @@ static void ideapad_kbd_bl_exit(struct ideapad_private *priv)
 /*
  * module init/exit
  */
-static void ideapad_sync_touchpad_state(struct ideapad_private *priv)
+static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_events)
 {
 	unsigned long value;
 	unsigned char param;
@@ -1508,8 +1508,11 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv)
 	 * KEY_TOUCHPAD_ON to not to get out of sync with LED
 	 */
 	i8042_command(&param, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE);
-	ideapad_input_report(priv, value ? 67 : 66);
-	sysfs_notify(&priv->platform_device->dev.kobj, NULL, "touchpad");
+
+	if (send_events) {
+		ideapad_input_report(priv, value ? 67 : 66);
+		sysfs_notify(&priv->platform_device->dev.kobj, NULL, "touchpad");
+	}
 }
 
 static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
@@ -1550,7 +1553,7 @@ static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
 			ideapad_sync_rfk_state(priv);
 			break;
 		case 5:
-			ideapad_sync_touchpad_state(priv);
+			ideapad_sync_touchpad_state(priv, true);
 			break;
 		case 4:
 			ideapad_backlight_notify_brightness(priv);
@@ -1840,7 +1843,7 @@ static int ideapad_acpi_add(struct platform_device *pdev)
 			ideapad_register_rfkill(priv, i);
 
 	ideapad_sync_rfk_state(priv);
-	ideapad_sync_touchpad_state(priv);
+	ideapad_sync_touchpad_state(priv, false);
 
 	err = ideapad_dytc_profile_init(priv);
 	if (err) {
@@ -1925,7 +1928,7 @@ static int ideapad_acpi_resume(struct device *dev)
 	struct ideapad_private *priv = dev_get_drvdata(dev);
 
 	ideapad_sync_rfk_state(priv);
-	ideapad_sync_touchpad_state(priv);
+	ideapad_sync_touchpad_state(priv, false);
 
 	if (priv->dytc)
 		dytc_profile_refresh(priv);
-- 
2.38.1

[PATCH 3/3] platform/x86: ideapad-laptop: Rework touchpad control code

[Hans de Goede]

Recently there have been multiple patches to disable the ideapad-laptop's
touchpad control code, because it is causing issues on various laptops,
see the Fixes tags for this commit.

The turning on/off of the ps2 aux port was added specifically for
the IdeaPad Z570, where the EC does toggle the touchpad on/off LED and
toggles the value returned by reading VPCCMD_R_TOUCHPAD, but it does not
actually turn on/off the touchpad.

The ideapad-laptop code really should not be messing with the i8042
controller on all devices just for this special case.

Drop the touchpad_ctrl_via_ec flag and add a DMI based allow-list for
devices which need this workaround, populating it with just the
Ideapad Z570 for now.

A related problem is that on recent Ideapad models the EC does not control
the touchpad at all. Check for this by checking if the value read from
VPCCMD_R_TOUCHPAD actually changes when receiving a touchpad-toggle hotkey
event; and if it does not change send KEY_TOUCHPAD_TOGGLE to userspace to
let userspace enable/disable the touchpad in software.

Fixes: d69cd7eea93e ("platform/x86: ideapad-laptop: Disable touchpad_switch for ELAN0634")
Fixes: a231224a601c ("platform/x86: ideapad-laptop: Disable touchpad_switch")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/ideapad-laptop.c | 69 ++++++++++++++-------------
 1 file changed, 36 insertions(+), 33 deletions(-)

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index eb0b1ec32c13..2654e3668c1a 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -134,6 +134,7 @@ struct ideapad_private {
 	struct ideapad_dytc_priv *dytc;
 	struct dentry *debug;
 	unsigned long cfg;
+	unsigned long r_touchpad_val;
 	struct {
 		bool conservation_mode    : 1;
 		bool dytc                 : 1;
@@ -142,8 +143,8 @@ struct ideapad_private {
 		bool set_fn_lock_led      : 1;
 		bool hw_rfkill_switch     : 1;
 		bool kbd_bl               : 1;
-		bool touchpad_ctrl_via_ec : 1;
 		bool usb_charging         : 1;
+		bool ctrl_ps2_aux_port    : 1;
 	} features;
 	struct {
 		bool initialized;
@@ -174,6 +175,12 @@ MODULE_PARM_DESC(set_fn_lock_led,
 	"Enable driver based updates of the fn-lock LED on fn-lock changes. "
 	"If you need this please report this to: platform-driver-x86@vger.kernel.org");
 
+static bool ctrl_ps2_aux_port;
+module_param(ctrl_ps2_aux_port, bool, 0444);
+MODULE_PARM_DESC(ctrl_ps2_aux_port,
+	"Enable driver based PS/2 aux port en-/dis-abling on touchpad on/off toggle. "
+	"If you need this please report this to: platform-driver-x86@vger.kernel.org");
+
 /*
  * shared data
  */
@@ -729,8 +736,6 @@ static umode_t ideapad_is_visible(struct kobject *kobj,
 		supported = priv->features.fan_mode;
 	else if (attr == &dev_attr_fn_lock.attr)
 		supported = priv->features.fn_lock;
-	else if (attr == &dev_attr_touchpad.attr)
-		supported = priv->features.touchpad_ctrl_via_ec;
 	else if (attr == &dev_attr_usb_charging.attr)
 		supported = priv->features.usb_charging;
 
@@ -1152,6 +1157,7 @@ static const struct key_entry ideapad_keymap[] = {
 	{ KE_KEY,  65, { KEY_PROG4 } },
 	{ KE_KEY,  66, { KEY_TOUCHPAD_OFF } },
 	{ KE_KEY,  67, { KEY_TOUCHPAD_ON } },
+	{ KE_KEY,  68, { KEY_TOUCHPAD_TOGGLE } },
 	{ KE_KEY, 128, { KEY_ESC } },
 
 	/*
@@ -1493,9 +1499,6 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_
 	unsigned char param;
 	int ret;
 
-	if (!priv->features.touchpad_ctrl_via_ec)
-		return;
-
 	/* Without reading from EC touchpad LED doesn't switch state */
 	ret = read_ec_data(priv->adev->handle, VPCCMD_R_TOUCHPAD, &value);
 	if (ret)
@@ -1504,15 +1507,26 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_
 	/*
 	 * Some IdeaPads don't really turn off touchpad - they only
 	 * switch the LED state. We (de)activate KBC AUX port to turn
-	 * touchpad off and on. We send KEY_TOUCHPAD_OFF and
-	 * KEY_TOUCHPAD_ON to not to get out of sync with LED
+	 * the touchpad on and off.
 	 */
-	i8042_command(&param, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE);
+	if (priv->features.ctrl_ps2_aux_port)
+		i8042_command(&param, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE);
 
 	if (send_events) {
-		ideapad_input_report(priv, value ? 67 : 66);
-		sysfs_notify(&priv->platform_device->dev.kobj, NULL, "touchpad");
+		/*
+		 * On older models the EC controls the touchpad and toggles it
+		 * on/off itself, in this case we report KEY_TOUCHPAD_ON/_OFF.
+		 * If the EC did not toggle, report KEY_TOUCHPAD_TOGGLE.
+		 */
+		if (value != priv->r_touchpad_val) {
+			ideapad_input_report(priv, value ? 67 : 66);
+			sysfs_notify(&priv->platform_device->dev.kobj, NULL, "touchpad");
+		} else {
+			ideapad_input_report(priv, 68);
+		}
 	}
+
+	priv->r_touchpad_val = value;
 }
 
 static void ideapad_acpi_notify(acpi_handle handle, u32 event, void *data)
@@ -1615,19 +1629,18 @@ static const struct dmi_system_id hw_rfkill_list[] = {
 	{}
 };
 
-static const struct dmi_system_id no_touchpad_switch_list[] = {
-	{
-	.ident = "Lenovo Yoga 3 Pro 1370",
-	.matches = {
-		DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
-		DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo YOGA 3"),
-		},
-	},
+/*
+ * On some models the EC toggles the touchpad muted LED on touchpad toggle
+ * hotkey presses, but the EC does not actually disable the touchpad itself.
+ * On these models the driver needs to explicitly enable/disable the i8042
+ * (PS/2) aux port.
+ */
+static const struct dmi_system_id ctrl_ps2_aux_port_list[] = {
 	{
-	.ident = "ZhaoYang K4e-IML",
+	/* Lenovo Ideapad Z570 */
 	.matches = {
 		DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
-		DMI_MATCH(DMI_PRODUCT_VERSION, "ZhaoYang K4e-IML"),
+		DMI_MATCH(DMI_PRODUCT_VERSION, "Ideapad Z570"),
 		},
 	},
 	{}
@@ -1642,14 +1655,8 @@ static void ideapad_check_features(struct ideapad_private *priv)
 		set_fn_lock_led || dmi_check_system(set_fn_lock_led_list);
 	priv->features.hw_rfkill_switch =
 		hw_rfkill_switch || dmi_check_system(hw_rfkill_list);
-
-	/* Most ideapads with ELAN0634 touchpad don't use EC touchpad switch */
-	if (acpi_dev_present("ELAN0634", NULL, -1))
-		priv->features.touchpad_ctrl_via_ec = 0;
-	else if (dmi_check_system(no_touchpad_switch_list))
-		priv->features.touchpad_ctrl_via_ec = 0;
-	else
-		priv->features.touchpad_ctrl_via_ec = 1;
+	priv->features.ctrl_ps2_aux_port =
+		ctrl_ps2_aux_port || dmi_check_system(ctrl_ps2_aux_port_list);
 
 	if (!read_ec_data(handle, VPCCMD_R_FAN, &val))
 		priv->features.fan_mode = true;
@@ -1834,10 +1841,6 @@ static int ideapad_acpi_add(struct platform_device *pdev)
 	if (!priv->features.hw_rfkill_switch)
 		write_ec_cmd(priv->adev->handle, VPCCMD_W_RF, 1);
 
-	/* The same for Touchpad */
-	if (!priv->features.touchpad_ctrl_via_ec)
-		write_ec_cmd(priv->adev->handle, VPCCMD_W_TOUCHPAD, 1);
-
 	for (i = 0; i < IDEAPAD_RFKILL_DEV_NUM; i++)
 		if (test_bit(ideapad_rfk_data[i].cfgbit, &priv->cfg))
 			ideapad_register_rfkill(priv, i);
-- 
2.38.1

Re: [PATCH 0/3] platform/x86: ideapad-laptop: Rework touchpad control code

[Eray Orçunus]

Hi,

On Thu, 16 Nov 2022 at 18:25, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi All,
> 
> Here are my proposed changes from the "ideapad-laptop touchpad handling
> problems, request for help" email thread as proper patches:
> https://lore.kernel.org/platform-driver-x86/bc1202d1-d85d-4173-5979-237bb1ee9254@redhat.com/
> 
> Note this applies on top of my review-hans branch which has seen a bunch
> of other ideapad-laptop changes land recently:
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
> 
> As suggested by Maxim, the third patch now has a module parameter to
> allow users to easily re-enable the i8042 aux-port enabling/disabling
> on models other then the Z570.
> 
> Eray, you mentioned in another email that you have some concerns about
> the approach in this series?

Yes, thanks for mentioning. My concerns are these:

- Users of laptops with ELAN0634 (like Yoga 14s and 720s), Lenovo
  Yoga 3 Pro 1370 and ZhaoYang K4e-IML will start to see non-working
  "touchpad" sysfs attribute on their ideapad-laptop driver. I see this
  as a regression. Also it's easy to fix; we can just test for if
  VPCCMD_W_TOUCHPAD works at the boot - with sending 0 first and verify the
  VPCCMD_R_TOUCHPAD result, and then sending 1 and again verify the
  VPCCMD_R_TOUCHPAD result. Later we can remove "touchpad" attribute if it
  doesn't work, with the exception of devices with ctrl_ps2_aux_port,
  since these laptops have working VPCCMD_R_TOUCHPAD.

- You removed sending 1 to VPCCMD_W_TOUCHPAD at the boot, are we sure
  there are no laptops needing that? I don't think we talked about that
  in previous e-mail thread.

- There is no i8042 cmd on touchpad_store, which may make the function
  ineffective on laptops with ctrl_ps2_aux_port.

- r_touchpad_val isn't set at touchpad_store and/or touchpad_read, which
  can make it out of sync when "touchpad" attr is accessed.

Best,
Eray

Re: [PATCH 0/3] platform/x86: ideapad-laptop: Rework touchpad control code

[Hans de Goede]

Hi Eray,

On 11/16/22 17:08, Eray Orçunus wrote:
> Hi,
> 
> On Thu, 16 Nov 2022 at 18:25, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi All,
>>
>> Here are my proposed changes from the "ideapad-laptop touchpad handling
>> problems, request for help" email thread as proper patches:
>> https://lore.kernel.org/platform-driver-x86/bc1202d1-d85d-4173-5979-237bb1ee9254@redhat.com/
>>
>> Note this applies on top of my review-hans branch which has seen a bunch
>> of other ideapad-laptop changes land recently:
>> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
>>
>> As suggested by Maxim, the third patch now has a module parameter to
>> allow users to easily re-enable the i8042 aux-port enabling/disabling
>> on models other then the Z570.
>>
>> Eray, you mentioned in another email that you have some concerns about
>> the approach in this series?
> 
> Yes, thanks for mentioning. My concerns are these:
> 
> - Users of laptops with ELAN0634 (like Yoga 14s and 720s), Lenovo
>   Yoga 3 Pro 1370 and ZhaoYang K4e-IML will start to see non-working
>   "touchpad" sysfs attribute on their ideapad-laptop driver. I see this
>   as a regression. Also it's easy to fix; we can just test for if
>   VPCCMD_W_TOUCHPAD works at the boot - with sending 0 first and verify the
>   VPCCMD_R_TOUCHPAD result, and then sending 1 and again verify the
>   VPCCMD_R_TOUCHPAD result. Later we can remove "touchpad" attribute if it
>   doesn't work,

I'm not 100% sure if this will work everywhere and I'm a bit worried the
VPCCMD_W_TOUCHPAD calls might cause problems on some devices. You did
mention that the ideapad windows software seems to always make
VPCCMD_W_TOUCHPAD calls.

But still, one of my goals with this series is to have the ideapad-laptop
driver poke less at the hw, to avoid the poking potentially causes issues.

Where as your suggested auto-detection makes the driver poke the hw more,
not less.

Thinking more about this makes me wonder: "why not just
entirely remove the whole touchpad sysfs attribute?"

Just removing the entire touchpad sysfs attribute will greatly simplify
things and normal userspace does not depend on it at all. Its only
potential users are custom user scripts and ideapad specific control
panels or some such.

And the custom control panels already need to deal with the touchpad
sysfs attribute not always being there.

I do realize that removing it might be a bit too big of a hammer,
so instead I plan to have it hidden by default and allow enabling
it through a module parameter.

Serr the next version of this patch-set (coming up soon).

Gating this with a module parameter will reduce all the maintenance
burden of having allow-lists are fragile auto-detect code for
a feature which I believe hardly anyone uses at all.

>   with the exception of devices with ctrl_ps2_aux_port,
>   since these laptops have working VPCCMD_R_TOUCHPAD.

I don't agree with having the touchpad sysfs attr visible on
the ctrl_ps2_aux_port devices, writing it will do nothing and as you
mentioned yourself some ideapad specific tools will show a touchpad
on/off option when this file is visible, which then will not work.

Note that just hiding the touchpad sysfs attr be default makes
this whole discussion mute though.

> - You removed sending 1 to VPCCMD_W_TOUCHPAD at the boot, are we sure
>   there are no laptops needing that? I don't think we talked about that
>   in previous e-mail thread.

This was only added recently (until recently there was no touchpad_ctrl_via_ec
flag at all) and seems to be copied and pasted from the rfkill code, where
we know that manually disabling the hw-rfkill bit is actually needed on
some models without an actual hw-rfkill switch...

> - There is no i8042 cmd on touchpad_store, which may make the function
>   ineffective on laptops with ctrl_ps2_aux_port.

This is not a problem introduced by this patch-set, touchpad-store never
controlled the ps2-aux port.

As mentioned above IMHO just hiding the touchpad sysfs attr here is better
(avoids the need to make touch-store have a special case for this).

These devices have a working touchpad-toggle hotkey + a LED to indicate
the status, so there really is no need for the sysfs attr.

(which makes me realize there is really little need for the sysfs attr
at all, so just always hiding it is easier, avoiding all sorts of possible
issues, see above).

> - r_touchpad_val isn't set at touchpad_store and/or touchpad_read, which
>   can make it out of sync when "touchpad" attr is accessed.

I've fixed this in the next version of my patch set.

Regards,

Hans