Re: [PATCH v4 2/3] platform/x86: thinkpad_acpi - Add doubletap_filter sysfs interface

[Hans de Goede <hansg@kernel.org>]

Hi Vishnu,

On 29-Nov-25 1:25 AM, Vishnu Sankar wrote:
> Add sysfs interface for controlling TrackPoint doubletap event filtering.
> This allows userspace to enable/disable doubletap functionality and
> query the current state.
> 
> The attribute is available at:
> /sys/devices/platform/thinkpad_acpi/doubletap_filter
> 
> When set to 1, doubletap events are filtered out (ignored).
> When set to 0, doubletap events are processed (default).
> 
> This complements the automatic hardware enablement in the trackpoint
> driver by providing user control over event processing at the kernel level.
> 
> Signed-off-by: Vishnu Sankar <vishnuocv@gmail.com>
> Suggested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> ---
> Changes in v2:
> - Updated commit message to clarify dependency on trackpoint driver
> - Now handling sysfs read/write of trackpoint driver using file read/write
> - Removed sysfs attribute creation of trackpoint double tap here
> - Reversed the logic and return false right away
> - Dropped unnecessary debug messages
> - Using dev_dbg() instead of pr_xxxx()
> 
> Changes in v3:
> - No changes
> 
> Changes in v4:
> - Simplified approach: single sysfs attribute for user control
> - Clear naming: doubletap_filter instead of doubletap_enabled
> - Intuitive behavior: 0=process events, 1=filter events
> - No cross-driver dependencies or complex interactions
> - Minimal code changes using existing thinkpad_acpi infrastructure
> ---
> ---
>  drivers/platform/x86/lenovo/thinkpad_acpi.c | 54 +++++++++++++++++++--
>  1 file changed, 49 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> index cc19fe520ea9..9b646ecff452 100644
> --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
> +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> @@ -373,7 +373,7 @@ static struct {
>  	u32 hotkey_poll_active:1;
>  	u32 has_adaptive_kbd:1;
>  	u32 kbd_lang:1;
> -	u32 trackpoint_doubletap:1;
> +	u32 trackpoint_doubletap_filter:1;

Why the rename? The new name suggests that events will be filtered
when trackpoint_doubletap_filter = 1, so inverted of the old name
but when processing events events are let through when
trackpoint_doubletap_filter == 1 so 1 means _not_ filtering which
makes the name weird:

> +		/* Only send the event if kernel-level filtering allows it */
> +		if (tp_features.trackpoint_doubletap_filter)
>  			tpacpi_input_send_key(hkey, send_acpi_ev);

Maybe you forgot to add an '!' here, iow this should become:

		/* Only send the event if kernel-level filtering allows it */
		if (!tp_features.trackpoint_doubletap_filter)
			tpacpi_input_send_key(hkey, send_acpi_ev);

?

>  	struct quirk_entry *quirks;
>  } tp_features;
>  
> @@ -3104,8 +3104,35 @@ static void tpacpi_send_radiosw_update(void)
>  	hotkey_radio_sw_notify_change();
>  }
>  
> +static ssize_t doubletap_filter_show(struct device *dev,
> +				      struct device_attribute *attr,
> +				      char *buf)
> +{
> +	return sysfs_emit(buf, "%d\n", tp_features.trackpoint_doubletap_filter);
> +}
> +
> +static ssize_t doubletap_filter_store(struct device *dev,
> +				       struct device_attribute *attr,
> +				       const char *buf, size_t count)
> +{
> +	bool filter;
> +	int err;
> +
> +	err = kstrtobool(buf, &filter);
> +	if (err)
> +		return err;
> +
> +	tp_features.trackpoint_doubletap_filter = filter;
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(doubletap_filter);
> +
>  static void hotkey_exit(void)
>  {
> +	if (tpacpi_pdev)
> +		device_remove_file(&tpacpi_pdev->dev, &dev_attr_doubletap_filter);
> +

New code should not use device_create_file() / device_remove_file() instead
the attribute should be added to hotkey_attributes[] and then the driver
core will automatically add/remove it.

>  	mutex_lock(&hotkey_mutex);
>  	hotkey_poll_stop_sync();
>  	dbg_printk(TPACPI_DBG_EXIT | TPACPI_DBG_HKEY,
> @@ -3557,8 +3584,22 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
>  
>  	hotkey_poll_setup_safe(true);
>  
> -	/* Enable doubletap by default */
> -	tp_features.trackpoint_doubletap = 1;
> +	/*
> +	 * Enable kernel-level doubletap event filtering by default.
> +	 * This allows doubletap events to reach userspace.
> +	 */
> +	tp_features.trackpoint_doubletap_filter = 1;

Again this seems to be inverted, or if you really want
trackpoint_doubletap_filter to work as an enable flag
then please rename it to trackpoint_doubletap_enable.


> +
> +	/* Create doubletap_filter sysfs attribute */
> +	if (tpacpi_pdev) {
> +		int err = device_create_file(&tpacpi_pdev->dev, &dev_attr_doubletap_filter);
> +		if (err) {
> +			pr_warn("Unable to create doubletap_filter sysfs attribute\n");
> +			/* Continue even if sysfs creation fails */
> +		} else {
> +			pr_info("ThinkPad ACPI doubletap_filter sysfs attribute created\n");
> +		}
> +	}

This can be dropped (replaced by adding the attr to hotkey_attributes[].

Regards,

Hans