Re: [PATCH] Input: synaptics - add support for ForcePads

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 09/08/2014 06:55 PM, Dmitry Torokhov wrote:
> ForcePads are found on HP EliteBook 1040 laptops. They lack any kind of
> physical buttons, instead they generate primary button click when user
> presses somewhat hard on the surface of the touchpad. Unfortunately they
> also report primary button click whenever there are 2 or more contacts
> on the pad, messing up all multi-finger gestures (2-finger scrolling,
> multi-finger tapping, etc). To cope with this behavior we introduce a
> delay (currently 50 msecs) in reporting primary press in case more
> contacts appear.
> 
> For now we are using DMI matching to detect ForcePads, hopefully we'll
> be able to figure a better way down the road.

What about using the pnp-id, in my experience with the recent lenovo
laptops that tends to be more reliable.

Christopher, Andrew (added to the CC), can one of you tell us if there
is a capability bit to detect this, and if not can you perhaps provide
a list of pnp-ids of devices which behave like this ?

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/mouse/synaptics.c | 80 ++++++++++++++++++++++++++++++++---------
>  drivers/input/mouse/synaptics.h |  5 +++
>  2 files changed, 69 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index e8573c6..5de1bb6 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -618,6 +618,8 @@ static void synaptics_parse_agm(const unsigned char buf[],
>  	priv->agm_pending = true;
>  }
>  
> +static bool is_forcepad;
> +
>  static int synaptics_parse_hw_state(const unsigned char buf[],
>  				    struct synaptics_data *priv,
>  				    struct synaptics_hw_state *hw)
> @@ -629,10 +631,58 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
>  			 ((buf[0] & 0x04) >> 1) |
>  			 ((buf[3] & 0x04) >> 2));
>  
> +		if ((SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) ||
> +			SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c)) &&
> +		    hw->w == 2) {
> +			synaptics_parse_agm(buf, priv, hw);
> +			return 1;
> +		}
> +
> +		hw->x = (((buf[3] & 0x10) << 8) |
> +			 ((buf[1] & 0x0f) << 8) |
> +			 buf[4]);
> +		hw->y = (((buf[3] & 0x20) << 7) |
> +			 ((buf[1] & 0xf0) << 4) |
> +			 buf[5]);
> +		hw->z = buf[2];
> +
>  		hw->left  = (buf[0] & 0x01) ? 1 : 0;
>  		hw->right = (buf[0] & 0x02) ? 1 : 0;
>  

Moving this up means that on clickpads the button will no longer
be checked + set for hw->w == 2 packets. That seems like an unintended
side effect. If this is intended then this should probably be in its own
patch.

> -		if (SYN_CAP_CLICKPAD(priv->ext_cap_0c)) {
> +		if (is_forcepad) {
> +			/* XXX is there a proper capability bit for this? */
> +			/*
> +			 * ForcePads, like Clickpads, use middle button
> +			 * bits to report primary button clicks.
> +			 * Unfortunately they report primary button not only
> +			 * when user presses on the pad above certain threshold,
> +			 * but also when there are more than one finger on the
> +			 * touchpad, which interferes with out multi-finger
> +			 * gestures.
> +			 */
> +			if (hw->z == 0) {
> +				/* No contacts */
> +				priv->press = priv->report_press = false;
> +			} else if (hw->w >= 4 && ((buf[0] ^ buf[3]) & 0x01)) {
> +				/*
> +				 * Single-finger touch with pressure above
> +				 * the threshold.
> +				 */
> +				if  (!priv->press) {
> +					priv->press_start = jiffies;
> +					priv->press = true;
> +				} else if (time_after(jiffies,
> +						priv->press_start +
> +							msecs_to_jiffies(50))) {
> +					priv->report_press = true;
> +				}

You're not setting a timer here, instead relying on there to be further events,
that is probably ok, but maybe put a comment to that extent here ?

> +			} else {
> +				priv->press = false;
> +			}
> +
> +			hw->left = priv->report_press;
> +
> +		} else if (SYN_CAP_CLICKPAD(priv->ext_cap_0c)) {
>  			/*
>  			 * Clickpad's button is transmitted as middle button,
>  			 * however, since it is primary button, we will report
> @@ -651,21 +701,6 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
>  			hw->down = ((buf[0] ^ buf[3]) & 0x02) ? 1 : 0;
>  		}
>  
> -		if ((SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) ||
> -			SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c)) &&
> -		    hw->w == 2) {
> -			synaptics_parse_agm(buf, priv, hw);
> -			return 1;
> -		}
> -
> -		hw->x = (((buf[3] & 0x10) << 8) |
> -			 ((buf[1] & 0x0f) << 8) |
> -			 buf[4]);
> -		hw->y = (((buf[3] & 0x20) << 7) |
> -			 ((buf[1] & 0xf0) << 4) |
> -			 buf[5]);
> -		hw->z = buf[2];
> -
>  		if (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) &&
>  		    ((buf[0] ^ buf[3]) & 0x02)) {
>  			switch (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) & ~0x01) {
> @@ -1642,11 +1677,24 @@ static const struct dmi_system_id __initconst cr48_dmi_table[] = {
>  	{ }
>  };
>  
> +static const struct dmi_system_id forcepad_dmi_table[] __initconst = {
> +#if defined(CONFIG_DMI) && defined(CONFIG_X86)
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "HP EliteBook Folio 1040 G1"),
> +		},
> +	},
> +#endif
> +	{ }
> +};
> +
>  void __init synaptics_module_init(void)
>  {
>  	impaired_toshiba_kbc = dmi_check_system(toshiba_dmi_table);
>  	broken_olpc_ec = dmi_check_system(olpc_dmi_table);
>  	cr48_profile_sensor = dmi_check_system(cr48_dmi_table);
> +	is_forcepad = dmi_check_system(forcepad_dmi_table);
>  }
>  
>  static int __synaptics_init(struct psmouse *psmouse, bool absolute_mode)
> diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
> index e594af0..adc80c9 100644
> --- a/drivers/input/mouse/synaptics.h
> +++ b/drivers/input/mouse/synaptics.h
> @@ -177,6 +177,11 @@ struct synaptics_data {
>  	 */
>  	struct synaptics_hw_state agm;
>  	bool agm_pending;			/* new AGM packet received */
> +
> +	/* ForcePad handling */
> +	unsigned long				press_start;
> +	bool					press;
> +	bool					report_press;
>  };
>  
>  void synaptics_module_init(void);
> 

Regards,

Hans