[PATCH] x86/Mouse: thinkpad_acpi/Trackpoint: Trackpoint Doubletap handling

[PATCH] x86/Mouse: thinkpad_acpi/Trackpoint: Trackpoint Doubletap handling

[Vishnu Sankar]

Newer ThinkPads have a doubletap feature that needs to be turned
ON/OFF via the trackpoint registers.
Systems released from 2023 have doubletap disabled by default and
need the feature enabling to be useful.

This patch introduces support for exposing and controlling the
trackpoint doubletap feature via a sysfs attribute.
/sys/devices/platform/thinkpad_acpi/tp_doubletap
This can be toggled by an "enable" or a "disable".

With this implemented we can remove the masking of events, and rely on
HW control instead, when the feature is disabled.

Note - Early Thinkpads (pre 2015) used the same register for hysteris
control, Check the FW IDs to make sure these are not affected.

trackpoint.h is moved to linux/input/.

Signed-off-by: Vishnu Sankar <vishnuocv@gmail.com>
Suggested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
 drivers/input/mouse/alps.c                    |   2 +-
 drivers/input/mouse/psmouse-base.c            |   2 +-
 drivers/input/mouse/trackpoint.c              | 115 ++++++++++++-
 drivers/platform/x86/thinkpad_acpi.c          | 153 ++++++++++++++++--
 .../linux/input}/trackpoint.h                 |  16 ++
 5 files changed, 276 insertions(+), 12 deletions(-)
 rename {drivers/input/mouse => include/linux/input}/trackpoint.h (90%)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index be734d65ea72..2abf338e8f1b 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -18,10 +18,10 @@
 #include <linux/serio.h>
 #include <linux/libps2.h>
 #include <linux/dmi.h>
+#include <linux/input/trackpoint.h>
 
 #include "psmouse.h"
 #include "alps.h"
-#include "trackpoint.h"
 
 /*
  * Definitions for ALPS version 3 and 4 command mode protocol
diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
index a2c9f7144864..6bc515254403 100644
--- a/drivers/input/mouse/psmouse-base.c
+++ b/drivers/input/mouse/psmouse-base.c
@@ -21,6 +21,7 @@
 #include <linux/libps2.h>
 #include <linux/mutex.h>
 #include <linux/types.h>
+#include <linux/input/trackpoint.h>
 
 #include "psmouse.h"
 #include "synaptics.h"
@@ -28,7 +29,6 @@
 #include "alps.h"
 #include "hgpk.h"
 #include "lifebook.h"
-#include "trackpoint.h"
 #include "touchkit_ps2.h"
 #include "elantech.h"
 #include "sentelic.h"
diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
index 5f6643b69a2c..53d06d72968a 100644
--- a/drivers/input/mouse/trackpoint.c
+++ b/drivers/input/mouse/trackpoint.c
@@ -13,8 +13,10 @@
 #include <linux/libps2.h>
 #include <linux/proc_fs.h>
 #include <linux/uaccess.h>
+#include <linux/input/trackpoint.h>
 #include "psmouse.h"
-#include "trackpoint.h"
+
+static struct trackpoint_data *trackpoint_dev;
 
 static const char * const trackpoint_variants[] = {
 	[TP_VARIANT_IBM]		= "IBM",
@@ -63,6 +65,21 @@ static int trackpoint_write(struct ps2dev *ps2dev, u8 loc, u8 val)
 	return ps2_command(ps2dev, param, MAKE_PS2_CMD(3, 0, TP_COMMAND));
 }
 
+/* Read function for TrackPoint extended registers */
+static int trackpoint_extended_read(struct ps2dev *ps2dev, u8 loc, u8 *val)
+{
+	u8 ext_param[2] = {TP_READ_MEM, loc};
+	int error;
+
+	error = ps2_command(ps2dev,
+			    ext_param, MAKE_PS2_CMD(2, 1, TP_COMMAND));
+
+	if (!error)
+		*val = ext_param[0];
+
+	return error;
+}
+
 static int trackpoint_toggle_bit(struct ps2dev *ps2dev, u8 loc, u8 mask)
 {
 	u8 param[3] = { TP_TOGGLE, loc, mask };
@@ -393,6 +410,96 @@ static int trackpoint_reconnect(struct psmouse *psmouse)
 	return 0;
 }
 
+/* List of known incapable device PNP IDs */
+static const char * const dt_incompatible_devices[] = {
+	"LEN0304",
+	"LEN0306",
+	"LEN0317",
+	"LEN031A",
+	"LEN031B",
+	"LEN031C",
+	"LEN031D",
+};
+
+/*
+ * checks if it’s a doubletap capable device
+ * The PNP ID format eg: is "PNP: LEN030d PNP0f13".
+ */
+bool is_trackpoint_dt_capable(const char *pnp_id)
+{
+	char id[16];
+
+	/* Make sure string starts with "PNP: " */
+	if (strncmp(pnp_id, "PNP: LEN03", 10) != 0)
+		return false;
+
+	/* Extract the first word after "PNP: " */
+	if (sscanf(pnp_id + 5, "%15s", id) != 1)
+		return false;
+
+	/* Check if it's blacklisted */
+	for (size_t i = 0; i < ARRAY_SIZE(dt_incompatible_devices); ++i) {
+		if (strcmp(pnp_id, dt_incompatible_devices[i]) == 0)
+			return false;
+	}
+
+	return true;
+}
+
+/* Trackpoint doubletap status function */
+int trackpoint_doubletap_status(bool *status)
+{
+	struct trackpoint_data *tp = trackpoint_dev;
+	struct ps2dev *ps2dev = &tp->psmouse->ps2dev;
+	u8 reg_val;
+	int rc;
+
+	/* Reading the Doubletap register using extended read */
+	rc = trackpoint_extended_read(ps2dev, TP_DOUBLETAP, &reg_val);
+	if (!rc)
+		*status = reg_val & BIT(TP_DOUBLETAP_STATUS_BIT) ?
+				true : false;
+
+	return rc;
+}
+EXPORT_SYMBOL(trackpoint_doubletap_status);
+
+/* Trackpoint doubletap enable/disable function */
+int trackpoint_set_doubletap(bool enable)
+{
+	struct trackpoint_data *tp = trackpoint_dev;
+	struct ps2dev *ps2dev = &tp->psmouse->ps2dev;
+	static u8 doubletap_status;
+	u8 new_val;
+
+	if (!tp)
+		return -ENODEV;
+
+	new_val = enable ? TP_DOUBLETAP_ENABLE : TP_DOUBLETAP_DISABLE;
+
+	/* Comparing the new value paased with the existing value */
+	if (doubletap_status == new_val) {
+		pr_info("TrackPoint: Doubletap is already %s\n",
+			enable ? "enabled" : "disabled");
+		return 0;
+	}
+
+	doubletap_status = new_val;
+
+	return trackpoint_write(ps2dev, TP_DOUBLETAP, new_val);
+}
+EXPORT_SYMBOL(trackpoint_set_doubletap);
+
+/*
+ * Doubletap capability check
+ * We use PNP ID to check the capability of the device.
+ */
+bool trackpoint_doubletap_support(void)
+{
+	return trackpoint_dev->doubletap_capable;
+}
+EXPORT_SYMBOL(trackpoint_doubletap_support);
+
 int trackpoint_detect(struct psmouse *psmouse, bool set_properties)
 {
 	struct ps2dev *ps2dev = &psmouse->ps2dev;
@@ -425,6 +532,9 @@ int trackpoint_detect(struct psmouse *psmouse, bool set_properties)
 	psmouse->reconnect = trackpoint_reconnect;
 	psmouse->disconnect = trackpoint_disconnect;
 
+	trackpoint_dev = psmouse->private;
+	trackpoint_dev->psmouse = psmouse;  /* Set parent reference */
+
 	if (variant_id != TP_VARIANT_IBM) {
 		/* Newer variants do not support extended button query. */
 		button_info = 0x33;
@@ -470,6 +580,9 @@ int trackpoint_detect(struct psmouse *psmouse, bool set_properties)
 		     psmouse->vendor, firmware_id,
 		     (button_info & 0xf0) >> 4, button_info & 0x0f);
 
+	/* Checking the doubletap Capability */
+	tp->doubletap_capable = is_trackpoint_dt_capable(ps2dev->serio->firmware_id);
+
 	return 0;
 }
 
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index e7350c9fa3aa..241c1dd5e1f4 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -71,6 +71,7 @@
 #include <linux/uaccess.h>
 #include <linux/units.h>
 #include <linux/workqueue.h>
+#include <linux/input/trackpoint.h>
 
 #include <acpi/battery.h>
 #include <acpi/video.h>
@@ -373,7 +374,8 @@ static struct {
 	u32 hotkey_poll_active:1;
 	u32 has_adaptive_kbd:1;
 	u32 kbd_lang:1;
-	u32 trackpoint_doubletap:1;
+	u32 trackpoint_doubletap_state:1;
+	u32 trackpoint_doubletap_capable:1;
 	struct quirk_entry *quirks;
 } tp_features;
 
@@ -3325,6 +3327,8 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 	bool radiosw_state  = false;
 	bool tabletsw_state = false;
 	int hkeyv, res, status, camera_shutter_state;
+	bool dt_state;
+	int rc;
 
 	vdbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_HKEY,
 			"initializing hotkey subdriver\n");
@@ -3556,8 +3560,19 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 
 	hotkey_poll_setup_safe(true);
 
-	/* Enable doubletap by default */
-	tp_features.trackpoint_doubletap = 1;
+	/* Checking doubletap status by default */
+	tp_features.trackpoint_doubletap_capable = trackpoint_doubletap_support();
+
+	if (tp_features.trackpoint_doubletap_capable) {
+		rc = trackpoint_doubletap_status(&dt_state);
+		if (rc) {
+			/* Disable if access to register fails */
+			tp_features.trackpoint_doubletap_state = false;
+			pr_info("ThinkPad ACPI: Doubletap failed to check status\n");
+		} else {
+			tp_features.trackpoint_doubletap_state = dt_state;
+		}
+	}
 
 	return 0;
 }
@@ -3862,9 +3877,7 @@ static bool hotkey_notify_8xxx(const u32 hkey, bool *send_acpi_ev)
 {
 	switch (hkey) {
 	case TP_HKEY_EV_TRACK_DOUBLETAP:
-		if (tp_features.trackpoint_doubletap)
-			tpacpi_input_send_key(hkey, send_acpi_ev);
-
+		*send_acpi_ev = true;
 		return true;
 	default:
 		return false;
@@ -10738,6 +10751,101 @@ static struct ibm_struct  dytc_profile_driver_data = {
 	.exit = dytc_profile_exit,
 };
 
+/************************************************************************
+ * Trackpoint Doubletap Interface
+ *
+ * Control/Monitoring of Trackpoint Doubletap from
+ * /sys/devices/platform/thinkpad_acpi/tp_doubletap
+ */
+
+static ssize_t tp_doubletap_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	bool status;
+
+	if (!trackpoint_doubletap_status(&status))
+		return sysfs_emit(buf, "access error\n");
+
+	return sysfs_emit(buf, "%s\n", status ? "enabled" : "disabled");
+}
+
+static ssize_t tp_doubletap_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count)
+{
+	if (sysfs_streq(buf, "enable")) {
+		/* enabling the doubletap here */
+		if (!trackpoint_set_doubletap(true))
+			tp_features.trackpoint_doubletap_state = true;
+	} else if (sysfs_streq(buf, "disable")) {
+		/* disabling the doubletap here */
+		if (!trackpoint_set_doubletap(false))
+			tp_features.trackpoint_doubletap_state = false;
+	} else {
+		pr_err("ThinkPad ACPI: thinkpad_acpi: Invalid value '%s' for tp_doubletap\n", buf);
+		return -EINVAL;
+	}
+
+	return count;
+}
+
+static umode_t tp_doubletap_is_visible(struct kobject *kobj, struct attribute *attr, int index);
+
+static DEVICE_ATTR_RW(tp_doubletap);
+
+static struct attribute *tp_doubletap_attrs[] = {
+	&dev_attr_tp_doubletap.attr,
+	NULL
+};
+
+static const struct attribute_group tp_doubletap_attr_group = {
+	.attrs = tp_doubletap_attrs,
+	.is_visible = tp_doubletap_is_visible,
+};
+
+static umode_t tp_doubletap_is_visible(struct kobject *kobj, struct attribute *attr, int index)
+{
+	/* Only show the attribute if the TrackPoint doubletap is supported */
+	tp_features.trackpoint_doubletap_capable = trackpoint_doubletap_support();
+	if (!tp_features.trackpoint_doubletap_capable)
+		return 0;
+
+	pr_info("ThinkPad ACPI: TrackPoint doubletap sysfs is visible\n");
+
+	return attr->mode;
+}
+
+static struct delayed_work tp_doubletap_work;
+
+static void tp_doubletap_work_func(struct work_struct *work)
+{
+	if (!trackpoint_doubletap_support()) {
+		pr_info("TrackPoint doubletap not supported yet, rechecking later\n");
+		schedule_delayed_work(&tp_doubletap_work, msecs_to_jiffies(2000));
+		return;
+	}
+
+	if (sysfs_create_group(&tpacpi_pdev->dev.kobj, &tp_doubletap_attr_group) == 0)
+		pr_info("TrackPoint doubletap sysfs group created\n");
+	else
+		pr_err("Failed to create TrackPoint doubletap sysfs group\n");
+}
+
+static int __init tp_doubletap_init(struct ibm_init_struct *iibm)
+{
+	INIT_DELAYED_WORK(&tp_doubletap_work, tp_doubletap_work_func);
+	schedule_delayed_work(&tp_doubletap_work, msecs_to_jiffies(1000));
+
+	return 0;
+}
+
+static void tp_doubletap_exit(void)
+{
+	device_remove_file(&tpacpi_pdev->dev, &dev_attr_tp_doubletap);
+}
+
+static struct ibm_struct tp_doubletap_driver_data = {
+	.name = "tp_doubletap",
+	.exit =  tp_doubletap_exit,
+};
+
 /*************************************************************************
  * Keyboard language interface
  */
@@ -11192,7 +11300,7 @@ static struct platform_driver tpacpi_hwmon_pdriver = {
  */
 static bool tpacpi_driver_event(const unsigned int hkey_event)
 {
-	int camera_shutter_state;
+	int camera_shutter_state, rc;
 
 	switch (hkey_event) {
 	case TP_HKEY_EV_BRGHT_UP:
@@ -11284,8 +11392,30 @@ static bool tpacpi_driver_event(const unsigned int hkey_event)
 		mutex_unlock(&tpacpi_inputdev_send_mutex);
 		return true;
 	case TP_HKEY_EV_DOUBLETAP_TOGGLE:
-		tp_features.trackpoint_doubletap = !tp_features.trackpoint_doubletap;
-		return true;
+		if (tp_features.trackpoint_doubletap_capable) {
+			/* Togging the register value */
+			rc = trackpoint_set_doubletap(!tp_features.trackpoint_doubletap_state);
+
+			if (rc) {
+				pr_err("ThinkPad ACPI: Trackpoint doubletap toggle failed\n");
+			} else {
+				/* Toggling the Doubletap Enable/Disable */
+				tp_features.trackpoint_doubletap_state =
+					!tp_features.trackpoint_doubletap_state;
+				pr_info("ThinkPad ACPI: Trackpoint doubletap is %s\n",
+					tp_features.trackpoint_doubletap_state ?
+					"enabled" : "disabled");
+
+				return true;
+			}
+		}
+
+		/*
+		 * Suppress the event if Doubletap is not supported
+		 * or if the trackpoint_set_doubletap() is failing
+		 */
+		return false;
+
 	case TP_HKEY_EV_PROFILE_TOGGLE:
 	case TP_HKEY_EV_PROFILE_TOGGLE2:
 		platform_profile_cycle();
@@ -11751,6 +11881,11 @@ static struct ibm_init_struct ibms_init[] __initdata = {
 		.init = auxmac_init,
 		.data = &auxmac_data,
 	},
+	{
+		.init = tp_doubletap_init,
+		.data = &tp_doubletap_driver_data
+	},
+
 };
 
 static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
diff --git a/drivers/input/mouse/trackpoint.h b/include/linux/input/trackpoint.h
similarity index 90%
rename from drivers/input/mouse/trackpoint.h
rename to include/linux/input/trackpoint.h
index eb5412904fe0..a8165becabe6 100644
--- a/drivers/input/mouse/trackpoint.h
+++ b/include/linux/input/trackpoint.h
@@ -69,6 +69,8 @@
 					/* (how hard it is to drag */
 					/* with Z-axis pressed) */
 
+#define TP_DOUBLETAP		0x58	/* TrackPoint doubletap register */
+
 #define TP_MINDRAG		0x59	/* Minimum amount of force needed */
 					/* to trigger dragging */
 
@@ -139,6 +141,12 @@
 #define TP_DEF_TWOHAND		0x00
 #define TP_DEF_SOURCE_TAG	0x00
 
+/* Doubletap register values */
+#define TP_DOUBLETAP_ENABLE	0xFF	/* Enable value */
+#define TP_DOUBLETAP_DISABLE	0xFE	/* Disable value */
+
+#define TP_DOUBLETAP_STATUS_BIT 0	/* 0th bit defines enable/disable */
+
 #define MAKE_PS2_CMD(params, results, cmd) ((params<<12) | (results<<8) | (cmd))
 
 struct trackpoint_data {
@@ -150,13 +158,21 @@ struct trackpoint_data {
 	u8 thresh, upthresh;
 	u8 ztime, jenks;
 	u8 drift_time;
+	bool doubletap_capable;
 
 	/* toggles */
 	bool press_to_select;
 	bool skipback;
 	bool ext_dev;
+
+	struct psmouse *psmouse;  /* Parent device */
 };
 
 int trackpoint_detect(struct psmouse *psmouse, bool set_properties);
 
+int trackpoint_doubletap_status(bool *status);
+int trackpoint_set_doubletap(bool enable);
+bool trackpoint_doubletap_support(void);
+bool is_trackpoint_dt_capable(const char *device_id);
+
 #endif /* _TRACKPOINT_H */
-- 
2.48.1

Re: [PATCH] x86/Mouse: thinkpad_acpi/Trackpoint: Trackpoint Doubletap handling

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 18978 bytes --]

On Fri, 20 Jun 2025, Vishnu Sankar wrote:

I don't like the shortlog prefixes (in the subject), please don't make 
confusing prefixes up like that.

> Newer ThinkPads have a doubletap feature that needs to be turned
> ON/OFF via the trackpoint registers.

Don't leave lines short mid-paragraph. Either reflow the paragraph or add 
an empty line in between paragraphs.

> Systems released from 2023 have doubletap disabled by default and
> need the feature enabling to be useful.
> 
> This patch introduces support for exposing and controlling the
> trackpoint doubletap feature via a sysfs attribute.
> /sys/devices/platform/thinkpad_acpi/tp_doubletap
> This can be toggled by an "enable" or a "disable".

This too has too short lines.

> 
> With this implemented we can remove the masking of events, and rely on

"With this implemented" is way too vague wording.

> HW control instead, when the feature is disabled.
> 
> Note - Early Thinkpads (pre 2015) used the same register for hysteris

hysteresis ?

> control, Check the FW IDs to make sure these are not affected.

This Note feels very much disconnected from the rest. Should be properly 
described and perhaps in own patch (I don't know)?

> trackpoint.h is moved to linux/input/.

This patch doesn't look minimal and seems to be combining many changes 
into one. Please make a patch series out of changes that can be separated 
instead of putting all together.

> Signed-off-by: Vishnu Sankar <vishnuocv@gmail.com>
> Suggested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> ---
>  drivers/input/mouse/alps.c                    |   2 +-
>  drivers/input/mouse/psmouse-base.c            |   2 +-
>  drivers/input/mouse/trackpoint.c              | 115 ++++++++++++-
>  drivers/platform/x86/thinkpad_acpi.c          | 153 ++++++++++++++++--
>  .../linux/input}/trackpoint.h                 |  16 ++
>  5 files changed, 276 insertions(+), 12 deletions(-)
>  rename {drivers/input/mouse => include/linux/input}/trackpoint.h (90%)
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index be734d65ea72..2abf338e8f1b 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -18,10 +18,10 @@
>  #include <linux/serio.h>
>  #include <linux/libps2.h>
>  #include <linux/dmi.h>
> +#include <linux/input/trackpoint.h>
>  
>  #include "psmouse.h"
>  #include "alps.h"
> -#include "trackpoint.h"
>  
>  /*
>   * Definitions for ALPS version 3 and 4 command mode protocol
> diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
> index a2c9f7144864..6bc515254403 100644
> --- a/drivers/input/mouse/psmouse-base.c
> +++ b/drivers/input/mouse/psmouse-base.c
> @@ -21,6 +21,7 @@
>  #include <linux/libps2.h>
>  #include <linux/mutex.h>
>  #include <linux/types.h>
> +#include <linux/input/trackpoint.h>
>  
>  #include "psmouse.h"
>  #include "synaptics.h"
> @@ -28,7 +29,6 @@
>  #include "alps.h"
>  #include "hgpk.h"
>  #include "lifebook.h"
> -#include "trackpoint.h"
>  #include "touchkit_ps2.h"
>  #include "elantech.h"
>  #include "sentelic.h"
> diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
> index 5f6643b69a2c..53d06d72968a 100644
> --- a/drivers/input/mouse/trackpoint.c
> +++ b/drivers/input/mouse/trackpoint.c
> @@ -13,8 +13,10 @@
>  #include <linux/libps2.h>
>  #include <linux/proc_fs.h>
>  #include <linux/uaccess.h>
> +#include <linux/input/trackpoint.h>
>  #include "psmouse.h"
> -#include "trackpoint.h"
> +
> +static struct trackpoint_data *trackpoint_dev;
>  
>  static const char * const trackpoint_variants[] = {
>  	[TP_VARIANT_IBM]		= "IBM",
> @@ -63,6 +65,21 @@ static int trackpoint_write(struct ps2dev *ps2dev, u8 loc, u8 val)
>  	return ps2_command(ps2dev, param, MAKE_PS2_CMD(3, 0, TP_COMMAND));
>  }
>  
> +/* Read function for TrackPoint extended registers */
> +static int trackpoint_extended_read(struct ps2dev *ps2dev, u8 loc, u8 *val)
> +{
> +	u8 ext_param[2] = {TP_READ_MEM, loc};
> +	int error;
> +
> +	error = ps2_command(ps2dev,
> +			    ext_param, MAKE_PS2_CMD(2, 1, TP_COMMAND));
> +
> +	if (!error)
> +		*val = ext_param[0];
> +
> +	return error;
> +}
> +
>  static int trackpoint_toggle_bit(struct ps2dev *ps2dev, u8 loc, u8 mask)
>  {
>  	u8 param[3] = { TP_TOGGLE, loc, mask };
> @@ -393,6 +410,96 @@ static int trackpoint_reconnect(struct psmouse *psmouse)
>  	return 0;
>  }
>  
> +/* List of known incapable device PNP IDs */
> +static const char * const dt_incompatible_devices[] = {
> +	"LEN0304",
> +	"LEN0306",
> +	"LEN0317",
> +	"LEN031A",
> +	"LEN031B",
> +	"LEN031C",
> +	"LEN031D",
> +};
> +
> +/*
> + * checks if it’s a doubletap capable device
> + * The PNP ID format eg: is "PNP: LEN030d PNP0f13".
> + */
> +bool is_trackpoint_dt_capable(const char *pnp_id)
> +{
> +	char id[16];
> +
> +	/* Make sure string starts with "PNP: " */
> +	if (strncmp(pnp_id, "PNP: LEN03", 10) != 0)

We seem to have strstarts().

> +		return false;
> +
> +	/* Extract the first word after "PNP: " */
> +	if (sscanf(pnp_id + 5, "%15s", id) != 1)
> +		return false;
> +
> +	/* Check if it's blacklisted */
> +	for (size_t i = 0; i < ARRAY_SIZE(dt_incompatible_devices); ++i) {
> +		if (strcmp(pnp_id, dt_incompatible_devices[i]) == 0)

Isn't this buggy wrt. the PNP: prefix??

Perhaps it would have been better to just do pnp_id += 5 before sscanf() 
as you don't care about the prefix after that.

> +			return false;
> +	}
> +
> +	return true;
> +}
> +
> +/* Trackpoint doubletap status function */
> +int trackpoint_doubletap_status(bool *status)
> +{
> +	struct trackpoint_data *tp = trackpoint_dev;
> +	struct ps2dev *ps2dev = &tp->psmouse->ps2dev;
> +	u8 reg_val;
> +	int rc;
> +
> +	/* Reading the Doubletap register using extended read */
> +	rc = trackpoint_extended_read(ps2dev, TP_DOUBLETAP, &reg_val);
> +	if (!rc)

Reverse the logic by returning the error first.

> +		*status = reg_val & BIT(TP_DOUBLETAP_STATUS_BIT) ?

Please remove the _BIT suffix, move BIT() into the define + make sure 
you've the necessary #include for BIT().

> +				true : false;
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL(trackpoint_doubletap_status);
> +
> +/* Trackpoint doubletap enable/disable function */
> +int trackpoint_set_doubletap(bool enable)
> +{
> +	struct trackpoint_data *tp = trackpoint_dev;
> +	struct ps2dev *ps2dev = &tp->psmouse->ps2dev;
> +	static u8 doubletap_status;
> +	u8 new_val;
> +
> +	if (!tp)
> +		return -ENODEV;
> +
> +	new_val = enable ? TP_DOUBLETAP_ENABLE : TP_DOUBLETAP_DISABLE;
> +
> +	/* Comparing the new value paased with the existing value */
> +	if (doubletap_status == new_val) {
> +		pr_info("TrackPoint: Doubletap is already %s\n",
> +			enable ? "enabled" : "disabled");

Why this needs to be logged on info level?

> +		return 0;
> +	}
> +
> +	doubletap_status = new_val;
> +
> +	return trackpoint_write(ps2dev, TP_DOUBLETAP, new_val);
> +}
> +EXPORT_SYMBOL(trackpoint_set_doubletap);
> +
> +/*
> + * Doubletap capability check
> + * We use PNP ID to check the capability of the device.
> + */
> +bool trackpoint_doubletap_support(void)
> +{
> +	return trackpoint_dev->doubletap_capable;
> +}
> +EXPORT_SYMBOL(trackpoint_doubletap_support);

Please write proper kerneldoc documentation for anything you EXPORT.

> +
>  int trackpoint_detect(struct psmouse *psmouse, bool set_properties)
>  {
>  	struct ps2dev *ps2dev = &psmouse->ps2dev;
> @@ -425,6 +532,9 @@ int trackpoint_detect(struct psmouse *psmouse, bool set_properties)
>  	psmouse->reconnect = trackpoint_reconnect;
>  	psmouse->disconnect = trackpoint_disconnect;
>  
> +	trackpoint_dev = psmouse->private;
> +	trackpoint_dev->psmouse = psmouse;  /* Set parent reference */

So why the member variable is not called parent then if you need a 
comment to tell that?

> +
>  	if (variant_id != TP_VARIANT_IBM) {
>  		/* Newer variants do not support extended button query. */
>  		button_info = 0x33;
> @@ -470,6 +580,9 @@ int trackpoint_detect(struct psmouse *psmouse, bool set_properties)
>  		     psmouse->vendor, firmware_id,
>  		     (button_info & 0xf0) >> 4, button_info & 0x0f);
>  
> +	/* Checking the doubletap Capability */

Drop too obvious comments, this is readily readable from the code itself.

> +	tp->doubletap_capable = is_trackpoint_dt_capable(ps2dev->serio->firmware_id);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index e7350c9fa3aa..241c1dd5e1f4 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c

What's you plan for merging this as we've moved this file under lenovo/ 
subdir in this cycle?

> @@ -71,6 +71,7 @@
>  #include <linux/uaccess.h>
>  #include <linux/units.h>
>  #include <linux/workqueue.h>
> +#include <linux/input/trackpoint.h>
>  
>  #include <acpi/battery.h>
>  #include <acpi/video.h>
> @@ -373,7 +374,8 @@ static struct {
>  	u32 hotkey_poll_active:1;
>  	u32 has_adaptive_kbd:1;
>  	u32 kbd_lang:1;
> -	u32 trackpoint_doubletap:1;
> +	u32 trackpoint_doubletap_state:1;
> +	u32 trackpoint_doubletap_capable:1;
>  	struct quirk_entry *quirks;
>  } tp_features;
>  
> @@ -3325,6 +3327,8 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
>  	bool radiosw_state  = false;
>  	bool tabletsw_state = false;
>  	int hkeyv, res, status, camera_shutter_state;
> +	bool dt_state;
> +	int rc;
>  
>  	vdbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_HKEY,
>  			"initializing hotkey subdriver\n");
> @@ -3556,8 +3560,19 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
>  
>  	hotkey_poll_setup_safe(true);
>  
> -	/* Enable doubletap by default */
> -	tp_features.trackpoint_doubletap = 1;
> +	/* Checking doubletap status by default */
> +	tp_features.trackpoint_doubletap_capable = trackpoint_doubletap_support();
> +
> +	if (tp_features.trackpoint_doubletap_capable) {
> +		rc = trackpoint_doubletap_status(&dt_state);
> +		if (rc) {
> +			/* Disable if access to register fails */
> +			tp_features.trackpoint_doubletap_state = false;
> +			pr_info("ThinkPad ACPI: Doubletap failed to check status\n");
> +		} else {
> +			tp_features.trackpoint_doubletap_state = dt_state;
> +		}
> +	}
>  
>  	return 0;
>  }
> @@ -3862,9 +3877,7 @@ static bool hotkey_notify_8xxx(const u32 hkey, bool *send_acpi_ev)
>  {
>  	switch (hkey) {
>  	case TP_HKEY_EV_TRACK_DOUBLETAP:
> -		if (tp_features.trackpoint_doubletap)
> -			tpacpi_input_send_key(hkey, send_acpi_ev);
> -
> +		*send_acpi_ev = true;
>  		return true;
>  	default:
>  		return false;
> @@ -10738,6 +10751,101 @@ static struct ibm_struct  dytc_profile_driver_data = {
>  	.exit = dytc_profile_exit,
>  };
>  
> +/************************************************************************
> + * Trackpoint Doubletap Interface
> + *
> + * Control/Monitoring of Trackpoint Doubletap from
> + * /sys/devices/platform/thinkpad_acpi/tp_doubletap
> + */
> +
> +static ssize_t tp_doubletap_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	bool status;
> +
> +	if (!trackpoint_doubletap_status(&status))
> +		return sysfs_emit(buf, "access error\n");

This should return error code instead I think.

> +
> +	return sysfs_emit(buf, "%s\n", status ? "enabled" : "disabled");

I'm sure there's an existing helper for this bool -> "enabled"/"disabled"
conversion (make sure you add the #include if not yet there when you use 
the helper).

> +}
> +
> +static ssize_t tp_doubletap_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	if (sysfs_streq(buf, "enable")) {

Hmm, should this attribute be named doubletap_enabled and follow the usual 
boolean convention instead?

> +		/* enabling the doubletap here */
> +		if (!trackpoint_set_doubletap(true))
> +			tp_features.trackpoint_doubletap_state = true;
> +	} else if (sysfs_streq(buf, "disable")) {
> +		/* disabling the doubletap here */
> +		if (!trackpoint_set_doubletap(false))
> +			tp_features.trackpoint_doubletap_state = false;
> +	} else {
> +		pr_err("ThinkPad ACPI: thinkpad_acpi: Invalid value '%s' for tp_doubletap\n", buf);

No, sysfs store function should not spam log like this, returning -EINVAL 
tells the very same thing already.

> +		return -EINVAL;
> +	}
> +
> +	return count;
> +}
> +
> +static umode_t tp_doubletap_is_visible(struct kobject *kobj, struct attribute *attr, int index);
> +
> +static DEVICE_ATTR_RW(tp_doubletap);
> +
> +static struct attribute *tp_doubletap_attrs[] = {
> +	&dev_attr_tp_doubletap.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group tp_doubletap_attr_group = {
> +	.attrs = tp_doubletap_attrs,
> +	.is_visible = tp_doubletap_is_visible,
> +};
> +
> +static umode_t tp_doubletap_is_visible(struct kobject *kobj, struct attribute *attr, int index)
> +{
> +	/* Only show the attribute if the TrackPoint doubletap is supported */
> +	tp_features.trackpoint_doubletap_capable = trackpoint_doubletap_support();
> +	if (!tp_features.trackpoint_doubletap_capable)
> +		return 0;
> +
> +	pr_info("ThinkPad ACPI: TrackPoint doubletap sysfs is visible\n");

???

In case any pr_*() printing remains after you've addressed my comments 
(this one shouldn't remain). You should use dev_*() functions for 
printing.

> +	return attr->mode;
> +}
> +
> +static struct delayed_work tp_doubletap_work;
> +
> +static void tp_doubletap_work_func(struct work_struct *work)
> +{
> +	if (!trackpoint_doubletap_support()) {
> +		pr_info("TrackPoint doubletap not supported yet, rechecking later\n");

dev_dbg()

> +		schedule_delayed_work(&tp_doubletap_work, msecs_to_jiffies(2000));
> +		return;
> +	}
> +
> +	if (sysfs_create_group(&tpacpi_pdev->dev.kobj, &tp_doubletap_attr_group) == 0)
> +		pr_info("TrackPoint doubletap sysfs group created\n");

dev_dbg()

> +	else
> +		pr_err("Failed to create TrackPoint doubletap sysfs group\n");

So this is effectively doing probe/init related code in a work function??
Should be very well justified in the changelog if there's no better way to 
do this.

> +}
> +
> +static int __init tp_doubletap_init(struct ibm_init_struct *iibm)
> +{
> +	INIT_DELAYED_WORK(&tp_doubletap_work, tp_doubletap_work_func);
> +	schedule_delayed_work(&tp_doubletap_work, msecs_to_jiffies(1000));
> +
> +	return 0;
> +}
> +
> +static void tp_doubletap_exit(void)
> +{
> +	device_remove_file(&tpacpi_pdev->dev, &dev_attr_tp_doubletap);
> +}
> +
> +static struct ibm_struct tp_doubletap_driver_data = {
> +	.name = "tp_doubletap",
> +	.exit =  tp_doubletap_exit,
> +};
> +
>  /*************************************************************************
>   * Keyboard language interface
>   */
> @@ -11192,7 +11300,7 @@ static struct platform_driver tpacpi_hwmon_pdriver = {
>   */
>  static bool tpacpi_driver_event(const unsigned int hkey_event)
>  {
> -	int camera_shutter_state;
> +	int camera_shutter_state, rc;
>  
>  	switch (hkey_event) {
>  	case TP_HKEY_EV_BRGHT_UP:
> @@ -11284,8 +11392,30 @@ static bool tpacpi_driver_event(const unsigned int hkey_event)
>  		mutex_unlock(&tpacpi_inputdev_send_mutex);
>  		return true;
>  	case TP_HKEY_EV_DOUBLETAP_TOGGLE:
> -		tp_features.trackpoint_doubletap = !tp_features.trackpoint_doubletap;
> -		return true;
> +		if (tp_features.trackpoint_doubletap_capable) {

Can't you reverse the logic and return false right away?

> +			/* Togging the register value */
> +			rc = trackpoint_set_doubletap(!tp_features.trackpoint_doubletap_state);
> +
> +			if (rc) {
> +				pr_err("ThinkPad ACPI: Trackpoint doubletap toggle failed\n");

return false

> +			} else {
> +				/* Toggling the Doubletap Enable/Disable */
> +				tp_features.trackpoint_doubletap_state =
> +					!tp_features.trackpoint_doubletap_state;
> +				pr_info("ThinkPad ACPI: Trackpoint doubletap is %s\n",
> +					tp_features.trackpoint_doubletap_state ?
> +					"enabled" : "disabled");

Use a string helper.

Looks another case for dev_dbg().

> +
> +				return true;
> +			}

This block becomes much lower in indentation after you reverse all the 
logic above. :-)

> +		}
> +
> +		/*
> +		 * Suppress the event if Doubletap is not supported
> +		 * or if the trackpoint_set_doubletap() is failing
> +		 */
> +		return false;
> +
>  	case TP_HKEY_EV_PROFILE_TOGGLE:
>  	case TP_HKEY_EV_PROFILE_TOGGLE2:
>  		platform_profile_cycle();
> @@ -11751,6 +11881,11 @@ static struct ibm_init_struct ibms_init[] __initdata = {
>  		.init = auxmac_init,
>  		.data = &auxmac_data,
>  	},
> +	{
> +		.init = tp_doubletap_init,
> +		.data = &tp_doubletap_driver_data
> +	},
> +
>  };
>  
>  static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
> diff --git a/drivers/input/mouse/trackpoint.h b/include/linux/input/trackpoint.h
> similarity index 90%
> rename from drivers/input/mouse/trackpoint.h
> rename to include/linux/input/trackpoint.h
> index eb5412904fe0..a8165becabe6 100644
> --- a/drivers/input/mouse/trackpoint.h
> +++ b/include/linux/input/trackpoint.h
> @@ -69,6 +69,8 @@
>  					/* (how hard it is to drag */
>  					/* with Z-axis pressed) */
>  
> +#define TP_DOUBLETAP		0x58	/* TrackPoint doubletap register */
> +
>  #define TP_MINDRAG		0x59	/* Minimum amount of force needed */
>  					/* to trigger dragging */
>  
> @@ -139,6 +141,12 @@
>  #define TP_DEF_TWOHAND		0x00
>  #define TP_DEF_SOURCE_TAG	0x00
>  
> +/* Doubletap register values */
> +#define TP_DOUBLETAP_ENABLE	0xFF	/* Enable value */
> +#define TP_DOUBLETAP_DISABLE	0xFE	/* Disable value */

So is the actual enable bit the lowest and we should not touch the other 
bits? Or does the entire value have this meaning?

> +
> +#define TP_DOUBLETAP_STATUS_BIT 0	/* 0th bit defines enable/disable */
> +
>  #define MAKE_PS2_CMD(params, results, cmd) ((params<<12) | (results<<8) | (cmd))
>  
>  struct trackpoint_data {
> @@ -150,13 +158,21 @@ struct trackpoint_data {
>  	u8 thresh, upthresh;
>  	u8 ztime, jenks;
>  	u8 drift_time;
> +	bool doubletap_capable;
>  
>  	/* toggles */
>  	bool press_to_select;
>  	bool skipback;
>  	bool ext_dev;
> +
> +	struct psmouse *psmouse;  /* Parent device */

parent_mouse and drop the comment?

>  };
>  
>  int trackpoint_detect(struct psmouse *psmouse, bool set_properties);
>  
> +int trackpoint_doubletap_status(bool *status);
> +int trackpoint_set_doubletap(bool enable);
> +bool trackpoint_doubletap_support(void);
> +bool is_trackpoint_dt_capable(const char *device_id);
> +
>  #endif /* _TRACKPOINT_H */
> 

-- 
 i.

Re: [PATCH] x86/Mouse: thinkpad_acpi/Trackpoint: Trackpoint Doubletap handling

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 7091 bytes --]

On Thu, 26 Jun 2025, Vishnu Sankar wrote:

> Hi Ilpo,
> 
> Thanks a lot for the comments and guidance.
> Please find my comments inline below.
> 
> On Wed, Jun 25, 2025 at 9:07 PM Ilpo Järvinen < ilpo.jarvinen@linux.intel.com >
> wrote:
>       On Fri, 20 Jun 2025, Vishnu Sankar wrote:
> 
>       I don't like the shortlog prefixes (in the subject), please don't make
>       confusing prefixes up like that.
> 
> Got it.
> I will correct this in V2 (as a patch series). 
> 
>       > Newer ThinkPads have a doubletap feature that needs to be turned
>       > ON/OFF via the trackpoint registers.
> 
>       Don't leave lines short mid-paragraph. Either reflow the paragraph or add
>       an empty line in between paragraphs.
> 
> Acked.
> Will correct this in V2.  
> 
>       > Systems released from 2023 have doubletap disabled by default and
>       > need the feature enabling to be useful.
>       >
>       > This patch introduces support for exposing and controlling the
>       > trackpoint doubletap feature via a sysfs attribute.
>       > /sys/devices/platform/thinkpad_acpi/tp_doubletap
>       > This can be toggled by an "enable" or a "disable".
> 
>       This too has too short lines.
> 
> Sorry! 
> Will do the needful in v2.
> 
>       >
>       > With this implemented we can remove the masking of events, and rely on
> 
>       "With this implemented" is way too vague wording.
> 
> Sorry for this!
> I will make this better. 
> 
>       > HW control instead, when the feature is disabled.
>       >
>       > Note - Early Thinkpads (pre 2015) used the same register for hysteris
> 
>       hysteresis ?
> 
> Sorry for not being clear.
> It's the trackpoint drag hysteris functionality. Pre-2015 ThinkPads used to have a
> user-configurable drag hysterisis register (0x58).
> Drag hysterisis is not user configurable now.
> 
>       > control, Check the FW IDs to make sure these are not affected.
> 
>       This Note feels very much disconnected from the rest. Should be properly
>       described and perhaps in own patch (I don't know)?
> 
> my bad, it's not FW ID, but PnP ID. 
> The older ThinkPad's trackpoint controllers use the same register (0x58) to control
> the drag hysteresis, which is obsolete now.
> Now (on newer systems from 2023) the same register (0x58) is remapped as
> doubletap register.  
> Just to exclude those older systems (with drag hysterisis control), we check the PnP
> ID's.
>
> I will give a better commit message in V2.
>
>       > trackpoint.h is moved to linux/input/.
> 
>       This patch doesn't look minimal and seems to be combining many changes
>       into one. Please make a patch series out of changes that can be separated
>       instead of putting all together.
> 
> Understood.
> Will make a patch series on V2
> 0001: Move trackpoint.h to include/linux/input
> 0002: Add new doubletap set/status/capable logics to trackpoint.c
> 0003: Add new trackpoint api's and trackpoint sysfs in thinkpad_acpi.c

Okay, sounds better.

>       > +/* List of known incapable device PNP IDs */
>       > +static const char * const dt_incompatible_devices[] = {
>       > +     "LEN0304",
>       > +     "LEN0306",
>       > +     "LEN0317",
>       > +     "LEN031A",
>       > +     "LEN031B",
>       > +     "LEN031C",
>       > +     "LEN031D",
>       > +};
>       > +
>       > +/*
>       > + * checks if it’s a doubletap capable device
>       > + * The PNP ID format eg: is "PNP: LEN030d PNP0f13".
>       > + */
>       > +bool is_trackpoint_dt_capable(const char *pnp_id)
>       > +{
>       > +     char id[16];
>       > +
>       > +     /* Make sure string starts with "PNP: " */
>       > +     if (strncmp(pnp_id, "PNP: LEN03", 10) != 0)
> 
>       We seem to have strstarts().
> 
> Thanks a lot for the suggestion.
> Will make use of this. 
> 
>       > +             return false;
>       > +
>       > +     /* Extract the first word after "PNP: " */
>       > +     if (sscanf(pnp_id + 5, "%15s", id) != 1)
>       > +             return false;
>       > +
>       > +     /* Check if it's blacklisted */
>       > +     for (size_t i = 0; i < ARRAY_SIZE(dt_incompatible_devices); ++i)
>       {
>       > +             if (strcmp(pnp_id, dt_incompatible_devices[i]) == 0)
> 
>       Isn't this buggy wrt. the PNP: prefix??
> 
>       Perhaps it would have been better to just do pnp_id += 5 before sscanf()
>       as you don't care about the prefix after that.
> 
> 
> Understood.
> Shall we have something like the following:
>         if (!strstarts(pnp_id, "PNP: LEN03"))
>               return false;
> 
>         id = pnp_id + 5;
> 
>         for (size_t i = 0; i < ARRAY_SIZE(dt_incompatible_devices); ++i) {
>                        if (strncmp(id, dt_incompatible_devices[i],
> strlen(dt_incompatible_devices[i])) == 0)

Why did you change to strncmp()?


>       > +     return sysfs_emit(buf, "%s\n", status ? "enabled" : "disabled");
> 
>       I'm sure there's an existing helper for this bool -> "enabled"/"disabled"
>       conversion (make sure you add the #include if not yet there when you use
>       the helper).
> 
> Is "bool_to_enabled_disabled(status)" the one that you are referring to?

Please see linux/string_choices.h, but after you change the attribute 
name, this won't be required as you can use normal boolean style with 0/1.

>       > +}
>       > +
>       > +static ssize_t tp_doubletap_store(struct device *dev, struct
>       device_attribute *attr, const char *buf, size_t count)
>       > +{
>       > +     if (sysfs_streq(buf, "enable")) {
> 
>       Hmm, should this attribute be named doubletap_enabled and follow the
>       usual
>       boolean convention instead?
> 
> Will change the attribute name as suggested. 

Please also use kstrtobool() then to convert the value.

>       > +             /* enabling the doubletap here */
>       > +             if (!trackpoint_set_doubletap(true))
>       > +                     tp_features.trackpoint_doubletap_state = true;
>       > +     } else if (sysfs_streq(buf, "disable")) {
>       > +             /* disabling the doubletap here */
>       > +             if (!trackpoint_set_doubletap(false))
>       > +                     tp_features.trackpoint_doubletap_state = false;
>       > +     } else {
>       > +             pr_err("ThinkPad ACPI: thinkpad_acpi: Invalid value '%s'
>       for tp_doubletap\n", buf);
> 
>       No, sysfs store function should not spam log like this, returning -EINVAL
>       tells the very same thing already.
> 
> Acked.
> will be removing the log. 
> 
>       > +             return -EINVAL;
>       > +     }
>       > +
>       > +     return count;
>       > +}


-- 
 i.

Re: [PATCH] x86/Mouse: thinkpad_acpi/Trackpoint: Trackpoint Doubletap handling

[Dmitry Torokhov]

Hi Vishnu,

On Fri, Jun 20, 2025 at 09:42:08AM +0900, Vishnu Sankar wrote:
> Newer ThinkPads have a doubletap feature that needs to be turned
> ON/OFF via the trackpoint registers.
> Systems released from 2023 have doubletap disabled by default and
> need the feature enabling to be useful.
> 
> This patch introduces support for exposing and controlling the
> trackpoint doubletap feature via a sysfs attribute.
> /sys/devices/platform/thinkpad_acpi/tp_doubletap
> This can be toggled by an "enable" or a "disable".
> 
> With this implemented we can remove the masking of events, and rely on
> HW control instead, when the feature is disabled.
> 
> Note - Early Thinkpads (pre 2015) used the same register for hysteris
> control, Check the FW IDs to make sure these are not affected.
> 
> trackpoint.h is moved to linux/input/.

No, please keep everything private to trackpoint.c and do not involve
thinkpad_acpi driver. By doing so you are introducing unwanted
dependencies (for both module loading, driver initialization, and
operation) and unsafe use of non-owned pointers/dangling pointers, etc.

Thanks.

-- 
Dmitry

Re: [PATCH] x86/Mouse: thinkpad_acpi/Trackpoint: Trackpoint Doubletap handling

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 7380 bytes --]

On Fri, 27 Jun 2025, Vishnu Sankar wrote:

> Hi Ilpo,
> 
> Thanks a lot for the review.
> 
> On Fri, Jun 27, 2025 at 12:09 AM Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
>       On Thu, 26 Jun 2025, Vishnu Sankar wrote:
> 
>       > Hi Ilpo,
>       >
>       > Thanks a lot for the comments and guidance.
>       > Please find my comments inline below.
>       >
>       > On Wed, Jun 25, 2025 at 9:07 PM Ilpo Järvinen <
>       ilpo.jarvinen@linux.intel.com >
>       > wrote:
>       >       On Fri, 20 Jun 2025, Vishnu Sankar wrote:
>       >
>       >       I don't like the shortlog prefixes (in the subject), please don't
>       make
>       >       confusing prefixes up like that.
>       >
>       > Got it.
>       > I will correct this in V2 (as a patch series). 
>       >
>       >       > Newer ThinkPads have a doubletap feature that needs to be
>       turned
>       >       > ON/OFF via the trackpoint registers.
>       >
>       >       Don't leave lines short mid-paragraph. Either reflow the
>       paragraph or add
>       >       an empty line in between paragraphs.
>       >
>       > Acked.
>       > Will correct this in V2.  
>       >
>       >       > Systems released from 2023 have doubletap disabled by default
>       and
>       >       > need the feature enabling to be useful.
>       >       >
>       >       > This patch introduces support for exposing and controlling the
>       >       > trackpoint doubletap feature via a sysfs attribute.
>       >       > /sys/devices/platform/thinkpad_acpi/tp_doubletap
>       >       > This can be toggled by an "enable" or a "disable".
>       >
>       >       This too has too short lines.
>       >
>       > Sorry! 
>       > Will do the needful in v2.
>       >
>       >       >
>       >       > With this implemented we can remove the masking of events, and
>       rely on
>       >
>       >       "With this implemented" is way too vague wording.
>       >
>       > Sorry for this!
>       > I will make this better. 
>       >
>       >       > HW control instead, when the feature is disabled.
>       >       >
>       >       > Note - Early Thinkpads (pre 2015) used the same register for
>       hysteris
>       >
>       >       hysteresis ?
>       >
>       > Sorry for not being clear.
>       > It's the trackpoint drag hysteris functionality. Pre-2015 ThinkPads
>       used to have a
>       > user-configurable drag hysterisis register (0x58).
>       > Drag hysterisis is not user configurable now.
>       >
>       >       > control, Check the FW IDs to make sure these are not affected.
>       >
>       >       This Note feels very much disconnected from the rest. Should be
>       properly
>       >       described and perhaps in own patch (I don't know)?
>       >
>       > my bad, it's not FW ID, but PnP ID. 
>       > The older ThinkPad's trackpoint controllers use the same register
>       (0x58) to control
>       > the drag hysteresis, which is obsolete now.
>       > Now (on newer systems from 2023) the same register (0x58) is remapped
>       as
>       > doubletap register.  
>       > Just to exclude those older systems (with drag hysterisis control), we
>       check the PnP
>       > ID's.
>       >
>       > I will give a better commit message in V2.
>       >
>       >       > trackpoint.h is moved to linux/input/.
>       >
>       >       This patch doesn't look minimal and seems to be combining many
>       changes
>       >       into one. Please make a patch series out of changes that can be
>       separated
>       >       instead of putting all together.
>       >
>       > Understood.
>       > Will make a patch series on V2
>       > 0001: Move trackpoint.h to include/linux/input
>       > 0002: Add new doubletap set/status/capable logics to trackpoint.c
>       > 0003: Add new trackpoint api's and trackpoint sysfs in thinkpad_acpi.c
> 
>       Okay, sounds better.
> 
>       >       > +/* List of known incapable device PNP IDs */
>       >       > +static const char * const dt_incompatible_devices[] = {
>       >       > +     "LEN0304",
>       >       > +     "LEN0306",
>       >       > +     "LEN0317",
>       >       > +     "LEN031A",
>       >       > +     "LEN031B",
>       >       > +     "LEN031C",
>       >       > +     "LEN031D",
>       >       > +};
>       >       > +
>       >       > +/*
>       >       > + * checks if it’s a doubletap capable device
>       >       > + * The PNP ID format eg: is "PNP: LEN030d PNP0f13".

There's case difference between this comment and the list.

>       >       > + */
>       >       > +bool is_trackpoint_dt_capable(const char *pnp_id)
>       >       > +{
>       >       > +     char id[16];
>       >       > +
>       >       > +     /* Make sure string starts with "PNP: " */
>       >       > +     if (strncmp(pnp_id, "PNP: LEN03", 10) != 0)
>       >
>       >       We seem to have strstarts().
>       >
>       > Thanks a lot for the suggestion.
>       > Will make use of this. 
>       >
>       >       > +             return false;
>       >       > +
>       >       > +     /* Extract the first word after "PNP: " */
>       >       > +     if (sscanf(pnp_id + 5, "%15s", id) != 1)
>       >       > +             return false;
>       >       > +
>       >       > +     /* Check if it's blacklisted */
>       >       > +     for (size_t i = 0; i <
>       ARRAY_SIZE(dt_incompatible_devices); ++i)
>       >       {
>       >       > +             if (strcmp(pnp_id, dt_incompatible_devices[i]) ==
>       0)
>       >
>       >       Isn't this buggy wrt. the PNP: prefix??
>       >
>       >       Perhaps it would have been better to just do pnp_id += 5 before
>       sscanf()
>       >       as you don't care about the prefix after that.
>       >
>       >
>       > Understood.
>       > Shall we have something like the following:
>       >         if (!strstarts(pnp_id, "PNP: LEN03"))
>       >               return false;
>       >
>       >         id = pnp_id + 5;
>       >
>       >         for (size_t i = 0; i < ARRAY_SIZE(dt_incompatible_devices);
>       ++i) {
>       >                        if (strncmp(id, dt_incompatible_devices[i],
>       > strlen(dt_incompatible_devices[i])) == 0)
> 
>       Why did you change to strncmp()?
> 
> I switched to strncmp() to allow matching just the known prefix part in
> dt_incompatible_devices, rather than requiring an exact full string match.
> Will keep the original "if (strcmp(id, dt_incompatible_devices[i]) == 0) " logic as
> it serves the purpose.

I didn't mean to say the change is necessarily incorrect, I was just 
asking for reasonale as it was different from the original.

If you think you it needs to be broader to the match to a prefix only, 
I've no problem with that.

-- 
 i.

Re: [PATCH] x86/Mouse: thinkpad_acpi/Trackpoint: Trackpoint Doubletap handling

[Mark Pearson]

Hi Dmitry,

On Fri, Jun 27, 2025, at 2:14 PM, Dmitry Torokhov wrote:
> Hi Vishnu,
>
> On Fri, Jun 20, 2025 at 09:42:08AM +0900, Vishnu Sankar wrote:
>> Newer ThinkPads have a doubletap feature that needs to be turned
>> ON/OFF via the trackpoint registers.
>> Systems released from 2023 have doubletap disabled by default and
>> need the feature enabling to be useful.
>> 
>> This patch introduces support for exposing and controlling the
>> trackpoint doubletap feature via a sysfs attribute.
>> /sys/devices/platform/thinkpad_acpi/tp_doubletap
>> This can be toggled by an "enable" or a "disable".
>> 
>> With this implemented we can remove the masking of events, and rely on
>> HW control instead, when the feature is disabled.
>> 
>> Note - Early Thinkpads (pre 2015) used the same register for hysteris
>> control, Check the FW IDs to make sure these are not affected.
>> 
>> trackpoint.h is moved to linux/input/.
>
> No, please keep everything private to trackpoint.c and do not involve
> thinkpad_acpi driver. By doing so you are introducing unwanted
> dependencies (for both module loading, driver initialization, and
> operation) and unsafe use of non-owned pointers/dangling pointers, etc.
>

Do you have recommendations on how to handle this case then?

This is a Thinkpad specific feature and hence the logic for involving thinkpad_acpi. There are Thinkpad hotkeys that will enable/disable the trackpoint doubletap feature - so there is some linkage. I'm not sure how to avoid that.

Is there a cleaner way to do this that you'd recommend we look at using? It's a feature (albeit a minor one) on the laptops that we'd like to make available to Linux users.

Mark

Re: [PATCH] x86/Mouse: thinkpad_acpi/Trackpoint: Trackpoint Doubletap handling

[Pali Rohár]

On Monday 30 June 2025 05:42:45 Mark Pearson wrote:
> Hi Dmitry,
> 
> On Fri, Jun 27, 2025, at 2:14 PM, Dmitry Torokhov wrote:
> > Hi Vishnu,
> >
> > On Fri, Jun 20, 2025 at 09:42:08AM +0900, Vishnu Sankar wrote:
> >> Newer ThinkPads have a doubletap feature that needs to be turned
> >> ON/OFF via the trackpoint registers.
> >> Systems released from 2023 have doubletap disabled by default and
> >> need the feature enabling to be useful.
> >> 
> >> This patch introduces support for exposing and controlling the
> >> trackpoint doubletap feature via a sysfs attribute.
> >> /sys/devices/platform/thinkpad_acpi/tp_doubletap
> >> This can be toggled by an "enable" or a "disable".
> >> 
> >> With this implemented we can remove the masking of events, and rely on
> >> HW control instead, when the feature is disabled.
> >> 
> >> Note - Early Thinkpads (pre 2015) used the same register for hysteris
> >> control, Check the FW IDs to make sure these are not affected.
> >> 
> >> trackpoint.h is moved to linux/input/.
> >
> > No, please keep everything private to trackpoint.c and do not involve
> > thinkpad_acpi driver. By doing so you are introducing unwanted
> > dependencies (for both module loading, driver initialization, and
> > operation) and unsafe use of non-owned pointers/dangling pointers, etc.
> >
> 
> Do you have recommendations on how to handle this case then?
> 
> This is a Thinkpad specific feature and hence the logic for involving thinkpad_acpi. There are Thinkpad hotkeys that will enable/disable the trackpoint doubletap feature - so there is some linkage. I'm not sure how to avoid that.
> 
> Is there a cleaner way to do this that you'd recommend we look at using? It's a feature (albeit a minor one) on the laptops that we'd like to make available to Linux users.
> 
> Mark

Hello, I do not know what is doubletap and patch description does not
explain it. But for laptop / mouse interface, I'm just giving example
that dell-laptop.c for some particular laptop can enable/disable
touchpad led and uses PS/2 interface for it. See touchpad_led_init().
I do not know if it is ideal or preferred solution, just writing to let
you know, maybe it can be useful.

Re: [PATCH] x86/Mouse: thinkpad_acpi/Trackpoint: Trackpoint Doubletap handling

[Dmitry Torokhov]

Hi Mark,

On Mon, Jun 30, 2025 at 05:42:45AM +0900, Mark Pearson wrote:
> Hi Dmitry,
> 
> On Fri, Jun 27, 2025, at 2:14 PM, Dmitry Torokhov wrote:
> > Hi Vishnu,
> >
> > On Fri, Jun 20, 2025 at 09:42:08AM +0900, Vishnu Sankar wrote:
> >> Newer ThinkPads have a doubletap feature that needs to be turned
> >> ON/OFF via the trackpoint registers.
> >> Systems released from 2023 have doubletap disabled by default and
> >> need the feature enabling to be useful.
> >> 
> >> This patch introduces support for exposing and controlling the
> >> trackpoint doubletap feature via a sysfs attribute.
> >> /sys/devices/platform/thinkpad_acpi/tp_doubletap
> >> This can be toggled by an "enable" or a "disable".
> >> 
> >> With this implemented we can remove the masking of events, and rely on
> >> HW control instead, when the feature is disabled.
> >> 
> >> Note - Early Thinkpads (pre 2015) used the same register for hysteris
> >> control, Check the FW IDs to make sure these are not affected.
> >> 
> >> trackpoint.h is moved to linux/input/.
> >
> > No, please keep everything private to trackpoint.c and do not involve
> > thinkpad_acpi driver. By doing so you are introducing unwanted
> > dependencies (for both module loading, driver initialization, and
> > operation) and unsafe use of non-owned pointers/dangling pointers, etc.
> >
> 
> Do you have recommendations on how to handle this case then?
> 
> This is a Thinkpad specific feature and hence the logic for involving
> thinkpad_acpi. There are Thinkpad hotkeys that will enable/disable the
> trackpoint doubletap feature - so there is some linkage. I'm not sure
> how to avoid that.
> 
> Is there a cleaner way to do this that you'd recommend we look at
> using? It's a feature (albeit a minor one) on the laptops that we'd
> like to make available to Linux users.

I believe if you define the doubletap as an attribute (see
TRACKPOINT_INT_ATTR or TRACKPOINT_BIT_ATTR in
drivers/input/mouse/trackpoint.c) then whatever process is handling the
hot keys switching this function on or off should be able to toggle the
behavior. The difference is that it will have to locate trackpoint node
in /sys/bus/serio/devices/* (or maybe scan
/sys/devices/platform/i8042/serio*) instead of expecting the attributes
be atached to thinkpad_acpi instance.

You just don't want to have one driver directly peeking into another,
because then it starts breaking if you unbind or force use of a
different protocol, etc.

Thanks.

-- 
Dmitry

Re: [PATCH] x86/Mouse: thinkpad_acpi/Trackpoint: Trackpoint Doubletap handling

[Vishnu Sankar]

Hi Pali,

On Mon, Jun 30, 2025 at 5:51 AM Pali Rohár <pali@kernel.org> wrote:
>
> On Monday 30 June 2025 05:42:45 Mark Pearson wrote:
> > Hi Dmitry,
> >
> > On Fri, Jun 27, 2025, at 2:14 PM, Dmitry Torokhov wrote:
> > > Hi Vishnu,
> > >
> > > On Fri, Jun 20, 2025 at 09:42:08AM +0900, Vishnu Sankar wrote:
> > >> Newer ThinkPads have a doubletap feature that needs to be turned
> > >> ON/OFF via the trackpoint registers.
> > >> Systems released from 2023 have doubletap disabled by default and
> > >> need the feature enabling to be useful.
> > >>
> > >> This patch introduces support for exposing and controlling the
> > >> trackpoint doubletap feature via a sysfs attribute.
> > >> /sys/devices/platform/thinkpad_acpi/tp_doubletap
> > >> This can be toggled by an "enable" or a "disable".
> > >>
> > >> With this implemented we can remove the masking of events, and rely on
> > >> HW control instead, when the feature is disabled.
> > >>
> > >> Note - Early Thinkpads (pre 2015) used the same register for hysteris
> > >> control, Check the FW IDs to make sure these are not affected.
> > >>
> > >> trackpoint.h is moved to linux/input/.
> > >
> > > No, please keep everything private to trackpoint.c and do not involve
> > > thinkpad_acpi driver. By doing so you are introducing unwanted
> > > dependencies (for both module loading, driver initialization, and
> > > operation) and unsafe use of non-owned pointers/dangling pointers, etc.
> > >
> >
> > Do you have recommendations on how to handle this case then?
> >
> > This is a Thinkpad specific feature and hence the logic for involving thinkpad_acpi. There are Thinkpad hotkeys that will enable/disable the trackpoint doubletap feature - so there is some linkage. I'm not sure how to avoid that.
> >
> > Is there a cleaner way to do this that you'd recommend we look at using? It's a feature (albeit a minor one) on the laptops that we'd like to make available to Linux users.
> >
> > Mark
>
> Hello, I do not know what is doubletap and patch description does not
> explain it. But for laptop / mouse interface, I'm just giving example
> that dell-laptop.c for some particular laptop can enable/disable
> touchpad led and uses PS/2 interface for it. See touchpad_led_init().
> I do not know if it is ideal or preferred solution, just writing to let
> you know, maybe it can be useful.

FYR, please find the previous commits regarding Doubletap support for
Trackpoint below:

https://github.com/torvalds/linux/commit/a9b0b1ee59a79d0d3853cba9a4b7376ea15be21f
https://github.com/torvalds/linux/commit/fd1e3344d13f1eedb862804dd1d2d5e184cf8eae

Sorry if details are missing in the commit. I can add those.

To be more precise, Thinkpad_acpi handles hotkeys, including Fn+G for doubletap.
To make that work, we need an integration point.

Thinkpad_acpi.c
Fn + G Hotkey - Enable/Disable the Trackpoint Double Tap.
DoubleTap on Trackpoint - Generates a DoubleTap ACPI Event

trackpoint.c
Enable/Disable the Doubletap functionality

To enable (this is decided based on Fn + G toggle key) the Trackpoint
Doubletap, we need to write to the Trackpoint extended register.
Therefore, the event to enable or disable the double tap will be
handled inside Thinkpad_acpi, but the actual write to the register is
in trackpoint.c.

I will check dell-laptop.c to understand the LED handling.

-- 

Regards,

      Vishnu Sankar
     +817015150407 (Japan)

Re: [PATCH] x86/Mouse: thinkpad_acpi/Trackpoint: Trackpoint Doubletap handling

[Vishnu Sankar]

Hi Ilpo,

Sorry for the late reply.

On Fri, Jun 27, 2025 at 4:28 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Fri, 27 Jun 2025, Vishnu Sankar wrote:
>
> > Hi Ilpo,
> >
> > Thanks a lot for the review.
> >
> > On Fri, Jun 27, 2025 at 12:09 AM Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> >       On Thu, 26 Jun 2025, Vishnu Sankar wrote:
> >
> >       > Hi Ilpo,
> >       >
> >       > Thanks a lot for the comments and guidance.
> >       > Please find my comments inline below.
> >       >
> >       > On Wed, Jun 25, 2025 at 9:07 PM Ilpo Järvinen <
> >       ilpo.jarvinen@linux.intel.com >
> >       > wrote:
> >       >       On Fri, 20 Jun 2025, Vishnu Sankar wrote:
> >       >
> >       >       I don't like the shortlog prefixes (in the subject), please don't
> >       make
> >       >       confusing prefixes up like that.
> >       >
> >       > Got it.
> >       > I will correct this in V2 (as a patch series).
> >       >
> >       >       > Newer ThinkPads have a doubletap feature that needs to be
> >       turned
> >       >       > ON/OFF via the trackpoint registers.
> >       >
> >       >       Don't leave lines short mid-paragraph. Either reflow the
> >       paragraph or add
> >       >       an empty line in between paragraphs.
> >       >
> >       > Acked.
> >       > Will correct this in V2.
> >       >
> >       >       > Systems released from 2023 have doubletap disabled by default
> >       and
> >       >       > need the feature enabling to be useful.
> >       >       >
> >       >       > This patch introduces support for exposing and controlling the
> >       >       > trackpoint doubletap feature via a sysfs attribute.
> >       >       > /sys/devices/platform/thinkpad_acpi/tp_doubletap
> >       >       > This can be toggled by an "enable" or a "disable".
> >       >
> >       >       This too has too short lines.
> >       >
> >       > Sorry!
> >       > Will do the needful in v2.
> >       >
> >       >       >
> >       >       > With this implemented we can remove the masking of events, and
> >       rely on
> >       >
> >       >       "With this implemented" is way too vague wording.
> >       >
> >       > Sorry for this!
> >       > I will make this better.
> >       >
> >       >       > HW control instead, when the feature is disabled.
> >       >       >
> >       >       > Note - Early Thinkpads (pre 2015) used the same register for
> >       hysteris
> >       >
> >       >       hysteresis ?
> >       >
> >       > Sorry for not being clear.
> >       > It's the trackpoint drag hysteris functionality. Pre-2015 ThinkPads
> >       used to have a
> >       > user-configurable drag hysterisis register (0x58).
> >       > Drag hysterisis is not user configurable now.
> >       >
> >       >       > control, Check the FW IDs to make sure these are not affected.
> >       >
> >       >       This Note feels very much disconnected from the rest. Should be
> >       properly
> >       >       described and perhaps in own patch (I don't know)?
> >       >
> >       > my bad, it's not FW ID, but PnP ID.
> >       > The older ThinkPad's trackpoint controllers use the same register
> >       (0x58) to control
> >       > the drag hysteresis, which is obsolete now.
> >       > Now (on newer systems from 2023) the same register (0x58) is remapped
> >       as
> >       > doubletap register.
> >       > Just to exclude those older systems (with drag hysterisis control), we
> >       check the PnP
> >       > ID's.
> >       >
> >       > I will give a better commit message in V2.
> >       >
> >       >       > trackpoint.h is moved to linux/input/.
> >       >
> >       >       This patch doesn't look minimal and seems to be combining many
> >       changes
> >       >       into one. Please make a patch series out of changes that can be
> >       separated
> >       >       instead of putting all together.
> >       >
> >       > Understood.
> >       > Will make a patch series on V2
> >       > 0001: Move trackpoint.h to include/linux/input
> >       > 0002: Add new doubletap set/status/capable logics to trackpoint.c
> >       > 0003: Add new trackpoint api's and trackpoint sysfs in thinkpad_acpi.c
> >
> >       Okay, sounds better.
> >
> >       >       > +/* List of known incapable device PNP IDs */
> >       >       > +static const char * const dt_incompatible_devices[] = {
> >       >       > +     "LEN0304",
> >       >       > +     "LEN0306",
> >       >       > +     "LEN0317",
> >       >       > +     "LEN031A",
> >       >       > +     "LEN031B",
> >       >       > +     "LEN031C",
> >       >       > +     "LEN031D",
> >       >       > +};
> >       >       > +
> >       >       > +/*
> >       >       > + * checks if it’s a doubletap capable device
> >       >       > + * The PNP ID format eg: is "PNP: LEN030d PNP0f13".
>
> There's case difference between this comment and the list.
Thank you for pointing this out.
Will correct this.
>
> >       >       > + */
> >       >       > +bool is_trackpoint_dt_capable(const char *pnp_id)
> >       >       > +{
> >       >       > +     char id[16];
> >       >       > +
> >       >       > +     /* Make sure string starts with "PNP: " */
> >       >       > +     if (strncmp(pnp_id, "PNP: LEN03", 10) != 0)
> >       >
> >       >       We seem to have strstarts().
> >       >
> >       > Thanks a lot for the suggestion.
> >       > Will make use of this.
> >       >
> >       >       > +             return false;
> >       >       > +
> >       >       > +     /* Extract the first word after "PNP: " */
> >       >       > +     if (sscanf(pnp_id + 5, "%15s", id) != 1)
> >       >       > +             return false;
> >       >       > +
> >       >       > +     /* Check if it's blacklisted */
> >       >       > +     for (size_t i = 0; i <
> >       ARRAY_SIZE(dt_incompatible_devices); ++i)
> >       >       {
> >       >       > +             if (strcmp(pnp_id, dt_incompatible_devices[i]) ==
> >       0)
> >       >
> >       >       Isn't this buggy wrt. the PNP: prefix??
> >       >
> >       >       Perhaps it would have been better to just do pnp_id += 5 before
> >       sscanf()
> >       >       as you don't care about the prefix after that.
> >       >
> >       >
> >       > Understood.
> >       > Shall we have something like the following:
> >       >         if (!strstarts(pnp_id, "PNP: LEN03"))
> >       >               return false;
> >       >
> >       >         id = pnp_id + 5;
> >       >
> >       >         for (size_t i = 0; i < ARRAY_SIZE(dt_incompatible_devices);
> >       ++i) {
> >       >                        if (strncmp(id, dt_incompatible_devices[i],
> >       > strlen(dt_incompatible_devices[i])) == 0)
> >
> >       Why did you change to strncmp()?
> >
> > I switched to strncmp() to allow matching just the known prefix part in
> > dt_incompatible_devices, rather than requiring an exact full string match.
> > Will keep the original "if (strcmp(id, dt_incompatible_devices[i]) == 0) " logic as
> > it serves the purpose.
>
> I didn't mean to say the change is necessarily incorrect, I was just
> asking for reasonale as it was different from the original.
Understood.
>
> If you think you it needs to be broader to the match to a prefix only,
> I've no problem with that.
Understood. Will keep the original as it is, without changes.
>
> --
>  i.



-- 

Regards,

      Vishnu Sankar
     +817015150407 (Japan)

Re: [PATCH] x86/Mouse: thinkpad_acpi/Trackpoint: Trackpoint Doubletap handling

[Vishnu Sankar]

Hi Dimitry,


On Mon, Jun 30, 2025 at 2:20 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Mark,
>
> On Mon, Jun 30, 2025 at 05:42:45AM +0900, Mark Pearson wrote:
> > Hi Dmitry,
> >
> > On Fri, Jun 27, 2025, at 2:14 PM, Dmitry Torokhov wrote:
> > > Hi Vishnu,
> > >
> > > On Fri, Jun 20, 2025 at 09:42:08AM +0900, Vishnu Sankar wrote:
> > >> Newer ThinkPads have a doubletap feature that needs to be turned
> > >> ON/OFF via the trackpoint registers.
> > >> Systems released from 2023 have doubletap disabled by default and
> > >> need the feature enabling to be useful.
> > >>
> > >> This patch introduces support for exposing and controlling the
> > >> trackpoint doubletap feature via a sysfs attribute.
> > >> /sys/devices/platform/thinkpad_acpi/tp_doubletap
> > >> This can be toggled by an "enable" or a "disable".
> > >>
> > >> With this implemented we can remove the masking of events, and rely on
> > >> HW control instead, when the feature is disabled.
> > >>
> > >> Note - Early Thinkpads (pre 2015) used the same register for hysteris
> > >> control, Check the FW IDs to make sure these are not affected.
> > >>
> > >> trackpoint.h is moved to linux/input/.
> > >
> > > No, please keep everything private to trackpoint.c and do not involve
> > > thinkpad_acpi driver. By doing so you are introducing unwanted
> > > dependencies (for both module loading, driver initialization, and
> > > operation) and unsafe use of non-owned pointers/dangling pointers, etc.
> > >
> >
> > Do you have recommendations on how to handle this case then?
> >
> > This is a Thinkpad specific feature and hence the logic for involving
> > thinkpad_acpi. There are Thinkpad hotkeys that will enable/disable the
> > trackpoint doubletap feature - so there is some linkage. I'm not sure
> > how to avoid that.
> >
> > Is there a cleaner way to do this that you'd recommend we look at
> > using? It's a feature (albeit a minor one) on the laptops that we'd
> > like to make available to Linux users.
>
> I believe if you define the doubletap as an attribute (see
> TRACKPOINT_INT_ATTR or TRACKPOINT_BIT_ATTR in
> drivers/input/mouse/trackpoint.c) then whatever process is handling the
> hot keys switching this function on or off should be able to toggle the
> behavior. The difference is that it will have to locate trackpoint node
> in /sys/bus/serio/devices/* (or maybe scan
> /sys/devices/platform/i8042/serio*) instead of expecting the attributes
> be atached to thinkpad_acpi instance.
>
> You just don't want to have one driver directly peeking into another,
> because then it starts breaking if you unbind or force use of a
> different protocol, etc.
>
> Thanks.
>
> --
> Dmitry

Thanks for the suggestion. I understand the concern about avoiding
direct driver-to-driver calls and unwanted dependencies.

Just to clarify: if we move the sysfs attribute to the trackpoint
driver itself (under /sys/bus/serio/devices/...), then thinkpad_acpi
would no longer be able to directly enable/disable the doubletap
feature in response to the Fn+G hotkey press. Don't we need userspace
to listen for the hotkey event, find the trackpoint sysfs node, and
toggle the attribute there?
That's possible, of course, but it means the feature won't work
out-of-the-box without extra userspace integration. For example, there
would be no automatic linkage between pressing Fn+G and toggling the
feature unless a udev rule or userspace daemon is configured to do it.
Or is there an approach you'd recommend to preserve the automatic
hotkey integration while avoiding the direct dependency between
thinkpad_acpi and trackpoint?
Sorry, I missed something.

Windows is utilizing this double-tap event as a Quick Launch button.


-- 

Regards,

      Vishnu Sankar
     +817015150407 (Japan)

Re: [PATCH] x86/Mouse: thinkpad_acpi/Trackpoint: Trackpoint Doubletap handling

[Dmitry Torokhov]

On Mon, Jun 30, 2025 at 08:50:27PM +0900, Vishnu Sankar wrote:
> Hi Dimitry,
> 
> 
> On Mon, Jun 30, 2025 at 2:20 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > Hi Mark,
> >
> > On Mon, Jun 30, 2025 at 05:42:45AM +0900, Mark Pearson wrote:
> > > Hi Dmitry,
> > >
> > > On Fri, Jun 27, 2025, at 2:14 PM, Dmitry Torokhov wrote:
> > > > Hi Vishnu,
> > > >
> > > > On Fri, Jun 20, 2025 at 09:42:08AM +0900, Vishnu Sankar wrote:
> > > >> Newer ThinkPads have a doubletap feature that needs to be turned
> > > >> ON/OFF via the trackpoint registers.
> > > >> Systems released from 2023 have doubletap disabled by default and
> > > >> need the feature enabling to be useful.
> > > >>
> > > >> This patch introduces support for exposing and controlling the
> > > >> trackpoint doubletap feature via a sysfs attribute.
> > > >> /sys/devices/platform/thinkpad_acpi/tp_doubletap
> > > >> This can be toggled by an "enable" or a "disable".
> > > >>
> > > >> With this implemented we can remove the masking of events, and rely on
> > > >> HW control instead, when the feature is disabled.
> > > >>
> > > >> Note - Early Thinkpads (pre 2015) used the same register for hysteris
> > > >> control, Check the FW IDs to make sure these are not affected.
> > > >>
> > > >> trackpoint.h is moved to linux/input/.
> > > >
> > > > No, please keep everything private to trackpoint.c and do not involve
> > > > thinkpad_acpi driver. By doing so you are introducing unwanted
> > > > dependencies (for both module loading, driver initialization, and
> > > > operation) and unsafe use of non-owned pointers/dangling pointers, etc.
> > > >
> > >
> > > Do you have recommendations on how to handle this case then?
> > >
> > > This is a Thinkpad specific feature and hence the logic for involving
> > > thinkpad_acpi. There are Thinkpad hotkeys that will enable/disable the
> > > trackpoint doubletap feature - so there is some linkage. I'm not sure
> > > how to avoid that.
> > >
> > > Is there a cleaner way to do this that you'd recommend we look at
> > > using? It's a feature (albeit a minor one) on the laptops that we'd
> > > like to make available to Linux users.
> >
> > I believe if you define the doubletap as an attribute (see
> > TRACKPOINT_INT_ATTR or TRACKPOINT_BIT_ATTR in
> > drivers/input/mouse/trackpoint.c) then whatever process is handling the
> > hot keys switching this function on or off should be able to toggle the
> > behavior. The difference is that it will have to locate trackpoint node
> > in /sys/bus/serio/devices/* (or maybe scan
> > /sys/devices/platform/i8042/serio*) instead of expecting the attributes
> > be atached to thinkpad_acpi instance.
> >
> > You just don't want to have one driver directly peeking into another,
> > because then it starts breaking if you unbind or force use of a
> > different protocol, etc.
> >
> > Thanks.
> >
> > --
> > Dmitry
> 
> Thanks for the suggestion. I understand the concern about avoiding
> direct driver-to-driver calls and unwanted dependencies.
> 
> Just to clarify: if we move the sysfs attribute to the trackpoint
> driver itself (under /sys/bus/serio/devices/...), then thinkpad_acpi
> would no longer be able to directly enable/disable the doubletap
> feature in response to the Fn+G hotkey press. Don't we need userspace
> to listen for the hotkey event, find the trackpoint sysfs node, and
> toggle the attribute there?

Yes.

> That's possible, of course, but it means the feature won't work
> out-of-the-box without extra userspace integration. For example, there
> would be no automatic linkage between pressing Fn+G and toggling the
> feature unless a udev rule or userspace daemon is configured to do it.
> Or is there an approach you'd recommend to preserve the automatic
> hotkey integration while avoiding the direct dependency between
> thinkpad_acpi and trackpoint?
> Sorry, I missed something.

Well, I guess you can look into interacting with sysfs file from
thinkpad_acpi.c... There is kernel_read_file_from_path() and others, you
will need to implement write counterpart of it. Pretty ugly but safer
than following pointers that may go away.

Thanks.

-- 
Dmitry

Re: [PATCH] x86/Mouse: thinkpad_acpi/Trackpoint: Trackpoint Doubletap handling

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4423 bytes --]

On Mon, 30 Jun 2025, Dmitry Torokhov wrote:
> On Mon, Jun 30, 2025 at 08:50:27PM +0900, Vishnu Sankar wrote:
> > On Mon, Jun 30, 2025 at 2:20 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > > On Mon, Jun 30, 2025 at 05:42:45AM +0900, Mark Pearson wrote:
> > > > On Fri, Jun 27, 2025, at 2:14 PM, Dmitry Torokhov wrote:
> > > > > On Fri, Jun 20, 2025 at 09:42:08AM +0900, Vishnu Sankar wrote:
> > > > >> Newer ThinkPads have a doubletap feature that needs to be turned
> > > > >> ON/OFF via the trackpoint registers.
> > > > >> Systems released from 2023 have doubletap disabled by default and
> > > > >> need the feature enabling to be useful.
> > > > >>
> > > > >> This patch introduces support for exposing and controlling the
> > > > >> trackpoint doubletap feature via a sysfs attribute.
> > > > >> /sys/devices/platform/thinkpad_acpi/tp_doubletap
> > > > >> This can be toggled by an "enable" or a "disable".
> > > > >>
> > > > >> With this implemented we can remove the masking of events, and rely on
> > > > >> HW control instead, when the feature is disabled.
> > > > >>
> > > > >> Note - Early Thinkpads (pre 2015) used the same register for hysteris
> > > > >> control, Check the FW IDs to make sure these are not affected.
> > > > >>
> > > > >> trackpoint.h is moved to linux/input/.
> > > > >
> > > > > No, please keep everything private to trackpoint.c and do not involve
> > > > > thinkpad_acpi driver. By doing so you are introducing unwanted
> > > > > dependencies (for both module loading, driver initialization, and
> > > > > operation) and unsafe use of non-owned pointers/dangling pointers, etc.
> > > > >
> > > >
> > > > Do you have recommendations on how to handle this case then?
> > > >
> > > > This is a Thinkpad specific feature and hence the logic for involving
> > > > thinkpad_acpi. There are Thinkpad hotkeys that will enable/disable the
> > > > trackpoint doubletap feature - so there is some linkage. I'm not sure
> > > > how to avoid that.
> > > >
> > > > Is there a cleaner way to do this that you'd recommend we look at
> > > > using? It's a feature (albeit a minor one) on the laptops that we'd
> > > > like to make available to Linux users.
> > >
> > > I believe if you define the doubletap as an attribute (see
> > > TRACKPOINT_INT_ATTR or TRACKPOINT_BIT_ATTR in
> > > drivers/input/mouse/trackpoint.c) then whatever process is handling the
> > > hot keys switching this function on or off should be able to toggle the
> > > behavior. The difference is that it will have to locate trackpoint node
> > > in /sys/bus/serio/devices/* (or maybe scan
> > > /sys/devices/platform/i8042/serio*) instead of expecting the attributes
> > > be atached to thinkpad_acpi instance.
> > >
> > > You just don't want to have one driver directly peeking into another,
> > > because then it starts breaking if you unbind or force use of a
> > > different protocol, etc.
> > >
> > > Thanks.
> > >
> > > --
> > > Dmitry
> > 
> > Thanks for the suggestion. I understand the concern about avoiding
> > direct driver-to-driver calls and unwanted dependencies.
> > 
> > Just to clarify: if we move the sysfs attribute to the trackpoint
> > driver itself (under /sys/bus/serio/devices/...), then thinkpad_acpi
> > would no longer be able to directly enable/disable the doubletap
> > feature in response to the Fn+G hotkey press. Don't we need userspace
> > to listen for the hotkey event, find the trackpoint sysfs node, and
> > toggle the attribute there?
> 
> Yes.
> 
> > That's possible, of course, but it means the feature won't work
> > out-of-the-box without extra userspace integration. For example, there
> > would be no automatic linkage between pressing Fn+G and toggling the
> > feature unless a udev rule or userspace daemon is configured to do it.
> > Or is there an approach you'd recommend to preserve the automatic
> > hotkey integration while avoiding the direct dependency between
> > thinkpad_acpi and trackpoint?
> > Sorry, I missed something.
> 
> Well, I guess you can look into interacting with sysfs file from
> thinkpad_acpi.c... There is kernel_read_file_from_path() and others, you
> will need to implement write counterpart of it. Pretty ugly but safer
> than following pointers that may go away.

Could device links be used here to ensure the correct shutdown order?

-- 
 i.

Re: [PATCH] x86/Mouse: thinkpad_acpi/Trackpoint: Trackpoint Doubletap handling

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 665 bytes --]

On Fri 2025-06-20 09:42:08, Vishnu Sankar wrote:
> Newer ThinkPads have a doubletap feature that needs to be turned
> ON/OFF via the trackpoint registers.
> Systems released from 2023 have doubletap disabled by default and
> need the feature enabling to be useful.
> 
> This patch introduces support for exposing and controlling the
> trackpoint doubletap feature via a sysfs attribute.
> /sys/devices/platform/thinkpad_acpi/tp_doubletap
> This can be toggled by an "enable" or a "disable".

sysfs attributes need documentation.
							Pavel
							
-- 
I don't work for Nazis and criminals, and neither should you.
Boycott Putin, Trump, and Musk!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]