Message-ID: <c7eb2d82-a487-1baa-dd89-4276551974ec@linux.intel.com>
Date: Wed, 25 Jun 2025 15:07:22 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Vishnu Sankar <vishnuocv@...il.com>
cc: pali@...nel.org, dmitry.torokhov@...il.com, hmh@....eng.br, 
    hansg@...nel.org, tglx@...utronix.de, mingo@...nel.org, jon_xie@...art.com, 
    jay_lee@...art.com, zhoubinbin@...ngson.cn, linux-input@...r.kernel.org, 
    LKML <linux-kernel@...r.kernel.org>, ibm-acpi-devel@...ts.sourceforge.net, 
    platform-driver-x86@...r.kernel.org, vsankar@...ovo.com, 
    Mark Pearson <mpearson-lenovo@...ebb.ca>
Subject: Re: [PATCH] x86/Mouse: thinkpad_acpi/Trackpoint: Trackpoint Doubletap
 handling

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@...il.com>
> Suggested-by: Mark Pearson <mpearson-lenovo@...ebb.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.

Powered by blists - more mailing lists