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

[Andrew Duggan <aduggan@synaptics.com>]

On 09/09/2014 10:06 AM, Dmitry Torokhov wrote:
> On Tue, Sep 09, 2014 at 09:44:17AM +0200, Hans de Goede wrote:
>> 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.
> Not sure. So far I only know of HP 1040 having it. FWIW:
>
> dtor@dtor-glaptop:~$ cat /sys/bus/pnp/drivers/i8042\ aux/00\:07/id
> SYN300d
> SYN0100
> SYN0002
> PNP0f13
>
> I think if we see generalities we can switch over later. I hope
> Chris/Andrew will come with a capability bit though :)

The ForcePad capabilities bit is 1 << 15.

>
>> 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.
> I guess we can call it a side effect, but it is fine - we parse AGM and
> stash the data, then when 2nd packet comes we parse and act upon it. In
> the old code we'd parse x, y, z, in AGM etc. and then have it
> overwritten with the data from the 2nd packet, here we do not bother.
>
>> If this is intended then this should probably be in its own
>> patch.
> It does not make sense on its own as ForcePad support needs to know
> values of Z and W when deciding how to handle primary click, otherwise
> the old code was just fine.
>
>>> -		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 ?
> I can, any suggestion on what you want me to say? "If single point
> contact held pressure long enough start reporting primary click"?
>
> Thanks.
>