From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753108AbaIIWQ0 (ORCPT ); Tue, 9 Sep 2014 18:16:26 -0400 Received: from us-mx2.synaptics.com ([192.147.44.131]:28424 "EHLO us-mx1.synaptics.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752691AbaIIWQJ (ORCPT ); Tue, 9 Sep 2014 18:16:09 -0400 X-PGP-Universal: processed; by securemail.synaptics.com on Tue, 09 Sep 2014 15:30:28 -0700 Message-ID: <540F7A19.3070704@synaptics.com> Date: Tue, 9 Sep 2014 15:07:21 -0700 From: Andrew Duggan User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.0 MIME-Version: 1.0 To: Dmitry Torokhov , Hans de Goede CC: , Benjamin Tissoires , Christopher Heiny , Subject: Re: [PATCH] Input: synaptics - add support for ForcePads References: <20140908165520.GA35051@core.coreip.homeip.net> <540EAFD1.2040307@redhat.com> <20140909170636.GA28774@core.coreip.homeip.net> In-Reply-To: <20140909170636.GA28774@core.coreip.homeip.net> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.2.20.38] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>> --- >>> 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. > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Duggan Subject: Re: [PATCH] Input: synaptics - add support for ForcePads Date: Tue, 9 Sep 2014 15:07:21 -0700 Message-ID: <540F7A19.3070704@synaptics.com> References: <20140908165520.GA35051@core.coreip.homeip.net> <540EAFD1.2040307@redhat.com> <20140909170636.GA28774@core.coreip.homeip.net> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from us-mx2.synaptics.com ([192.147.44.131]:28424 "EHLO us-mx1.synaptics.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752691AbaIIWQJ (ORCPT ); Tue, 9 Sep 2014 18:16:09 -0400 In-Reply-To: <20140909170636.GA28774@core.coreip.homeip.net> Sender: linux-input-owner@vger.kernel.org List-Id: linux-input@vger.kernel.org To: Dmitry Torokhov , Hans de Goede Cc: linux-input@vger.kernel.org, Benjamin Tissoires , Christopher Heiny , linux-kernel@vger.kernel.org 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 >>> --- >>> 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. >