LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/4] New Lenovos 2015 touchpads: party time!
@ 2015-01-28 20:10 Benjamin Tissoires
  2015-01-28 20:10 ` [PATCH 1/4] Input - synaptics: fix middle button on Lenovo 2015 products Benjamin Tissoires
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Benjamin Tissoires @ 2015-01-28 20:10 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Hans de Goede, Peter Hutterer, linux-input, linux-kernel

Hi Dmitry,

Lenovo released their new models, and believe it or not, some kernel
work is required to support them!

We learned the lesson and we will try to maximize the effort in user space
through udev rules instead of polluting the kernel with exceptions.

Patches 1 & 2 are general fixes for synaptics that have been unnoticed
until this new models. They are marked as stable.

Patch 3 is a generic way to fix the PnpId list that was hardcoded in the
kernel. This one is also marked as stable so that distributions will not
have the top software buttons on devices that do not require it.

Patch 4 is a nice to have. It is not mandatory, so not CC-ed to stable,
it just reduced the PnpId list.

The good thing is that the reported min/max are correct, so the problems
with the first generation of the t440 touchpad have been fixed now (and
synaptics assured us that they will keep checking the quality of the PS/2
handling of their touchpads).

One last thing, I still did not manage to talk to the device over SMBus,
so we are still stuck with the PS/2 protocol and its limitations for this
series.

Cheers,
Benjamin

Benjamin Tissoires (4):
  Input - synaptics: fix middle button on Lenovo 2015 products
  Input - synaptics: do not release extra buttons once they are pressed
  Input - synaptics: remove TOPBUTTONPAD property for Lenovos 2015
  Input - synaptics: Remove X1 Carbon 3rd gen from the topbuttonpad list

 drivers/input/mouse/synaptics.c | 20 ++++++++++++++------
 drivers/input/mouse/synaptics.h |  3 +++
 2 files changed, 17 insertions(+), 6 deletions(-)

-- 
2.1.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/4] Input - synaptics: fix middle button on Lenovo 2015 products
  2015-01-28 20:10 [PATCH 0/4] New Lenovos 2015 touchpads: party time! Benjamin Tissoires
@ 2015-01-28 20:10 ` Benjamin Tissoires
  2015-02-02 22:54   ` Dmitry Torokhov
  2015-01-28 20:10 ` [PATCH 2/4] Input - synaptics: do not release extra buttons once they are pressed Benjamin Tissoires
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Benjamin Tissoires @ 2015-01-28 20:10 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Hans de Goede, Peter Hutterer, linux-input, linux-kernel

On the X1 Carbon 3rd gen (with a 2015 broadwell cpu), the physical middle
button of the trackstick (attached to the touchpad serio device, of course)
seems to get lost.

Actually, the touchpads reports 3 extra buttons, which falls in the switch
below to the '2' case. Let's handle the case of odd numbers also, so that
the middle button finds its way back.

Cc: stable@vger.kernel.org
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/input/mouse/synaptics.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index f89de89..0d12664 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -689,7 +689,7 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
 
 		if (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) &&
 		    ((buf[0] ^ buf[3]) & 0x02)) {
-			switch (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) & ~0x01) {
+			switch (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap)) {
 			default:
 				/*
 				 * if nExtBtn is greater than 8 it should be
@@ -698,15 +698,19 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
 				break;
 			case 8:
 				hw->ext_buttons |= ((buf[5] & 0x08)) ? 0x80 : 0;
+			case 7:
 				hw->ext_buttons |= ((buf[4] & 0x08)) ? 0x40 : 0;
 			case 6:
 				hw->ext_buttons |= ((buf[5] & 0x04)) ? 0x20 : 0;
+			case 5:
 				hw->ext_buttons |= ((buf[4] & 0x04)) ? 0x10 : 0;
 			case 4:
 				hw->ext_buttons |= ((buf[5] & 0x02)) ? 0x08 : 0;
+			case 3:
 				hw->ext_buttons |= ((buf[4] & 0x02)) ? 0x04 : 0;
 			case 2:
 				hw->ext_buttons |= ((buf[5] & 0x01)) ? 0x02 : 0;
+			case 1:
 				hw->ext_buttons |= ((buf[4] & 0x01)) ? 0x01 : 0;
 			}
 		}
-- 
2.1.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 2/4] Input - synaptics: do not release extra buttons once they are pressed
  2015-01-28 20:10 [PATCH 0/4] New Lenovos 2015 touchpads: party time! Benjamin Tissoires
  2015-01-28 20:10 ` [PATCH 1/4] Input - synaptics: fix middle button on Lenovo 2015 products Benjamin Tissoires
@ 2015-01-28 20:10 ` Benjamin Tissoires
  2015-02-02 21:46   ` Dmitry Torokhov
  2015-01-28 20:10 ` [PATCH 3/4] Input - synaptics: remove TOPBUTTONPAD property for Lenovos 2015 Benjamin Tissoires
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Benjamin Tissoires @ 2015-01-28 20:10 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Hans de Goede, Peter Hutterer, linux-input, linux-kernel

The current code releases the extra buttons right after they are pressed.
As soon as a new serio report comes in, the hw state is reset to 0
and so the buttons are released.

Check for the report type before acting on the current extra buttons
state.

Cc: stable@vger.kernel.org
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/input/mouse/synaptics.c | 10 +++++++---
 drivers/input/mouse/synaptics.h |  3 +++
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 0d12664..d58863b 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -688,7 +688,7 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
 		}
 
 		if (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) &&
-		    ((buf[0] ^ buf[3]) & 0x02)) {
+		    SYN_REPORT_EXT_BUTTONS(buf)) {
 			switch (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap)) {
 			default:
 				/*
@@ -792,8 +792,12 @@ static void synaptics_report_buttons(struct psmouse *psmouse,
 		input_report_key(dev, BTN_BACK, hw->down);
 	}
 
-	for (i = 0; i < SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap); i++)
-		input_report_key(dev, BTN_0 + i, hw->ext_buttons & (1 << i));
+	if (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) &&
+	    SYN_REPORT_EXT_BUTTONS(psmouse->packet)) {
+		for (i = 0; i < SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap); i++)
+			input_report_key(dev, BTN_0 + i,
+					 hw->ext_buttons & (1 << i));
+	}
 }
 
 static void synaptics_report_mt_data(struct psmouse *psmouse,
diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
index 6faf9bb..0b2b711 100644
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -115,6 +115,9 @@
 #define SYN_NEWABS_RELAXED		2
 #define SYN_OLDABS			3
 
+/* synaptics extended button packet */
+#define SYN_REPORT_EXT_BUTTONS(buf)	(((buf)[0] ^ (buf)[3]) & 0x02)
+
 /* amount to fuzz position data when touchpad reports reduced filtering */
 #define SYN_REDUCED_FILTER_FUZZ		8
 
-- 
2.1.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 3/4] Input - synaptics: remove TOPBUTTONPAD property for Lenovos 2015
  2015-01-28 20:10 [PATCH 0/4] New Lenovos 2015 touchpads: party time! Benjamin Tissoires
  2015-01-28 20:10 ` [PATCH 1/4] Input - synaptics: fix middle button on Lenovo 2015 products Benjamin Tissoires
  2015-01-28 20:10 ` [PATCH 2/4] Input - synaptics: do not release extra buttons once they are pressed Benjamin Tissoires
@ 2015-01-28 20:10 ` Benjamin Tissoires
  2015-02-02 21:59   ` Dmitry Torokhov
  2015-01-28 20:10 ` [PATCH 4/4] Input - synaptics: Remove X1 Carbon 3rd gen from the topbuttonpad list Benjamin Tissoires
  2015-01-29  7:52 ` [PATCH 0/4] New Lenovos 2015 touchpads: party time! Hans de Goede
  4 siblings, 1 reply; 15+ messages in thread
From: Benjamin Tissoires @ 2015-01-28 20:10 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Hans de Goede, Peter Hutterer, linux-input, linux-kernel

The 2015 series of the Lenovo thinkpads added back the hardware buttons
on top of the touchpad for the trackstick.

Unfortunately, they are wired to the touchpad, and not the trackstick.
Thus, they are seen as extra buttons from the kernel point of view.

This leads to a problem in user space because extra buttons on synaptics
devices used to be used as scroll up/down buttons. So in the end, the
experience for the user is scroll events for buttons left and right
when using the trackstick. Yay!
Still, this user space problem will be fixed in the user space, and
the kernel will only handle the TOPBUTTONPAD property - which was not
a good idea to introduce in the kernel in the first place, I concede.

Furthermore, Lenovo kept using the PNPIDs that are supposed to be
"5 buttons" touchpads, so the new laptops also have the
INPUT_PROP_TOPBUTTONPAD. Yay again!

Insteead of manually removing each of the new ones, or hoping that we
know all the current ones, we can consider that the PNPIDs list that
were given contains touchpads that have the trackstick buttons, either
physically wired to them, or emulated with the top software button
property.

Thanks to the extra buttons capability, we can reliably detect the
physical buttons from the software ones, and so we can remove the
TOPBUTTONPAD property even if it was declared as such.

Cc: stable@vger.kernel.org
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/input/mouse/synaptics.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index d58863b..4f05c9f 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -1129,7 +1129,8 @@ static void set_input_params(struct psmouse *psmouse,
 
 	if (SYN_CAP_CLICKPAD(priv->ext_cap_0c)) {
 		__set_bit(INPUT_PROP_BUTTONPAD, dev->propbit);
-		if (psmouse_matches_pnp_id(psmouse, topbuttonpad_pnp_ids))
+		if (psmouse_matches_pnp_id(psmouse, topbuttonpad_pnp_ids) &&
+		    !test_bit(BTN_0, dev->keybit))
 			__set_bit(INPUT_PROP_TOPBUTTONPAD, dev->propbit);
 		/* Clickpads report only left button */
 		__clear_bit(BTN_RIGHT, dev->keybit);
-- 
2.1.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 4/4] Input - synaptics: Remove X1 Carbon 3rd gen from the topbuttonpad list
  2015-01-28 20:10 [PATCH 0/4] New Lenovos 2015 touchpads: party time! Benjamin Tissoires
                   ` (2 preceding siblings ...)
  2015-01-28 20:10 ` [PATCH 3/4] Input - synaptics: remove TOPBUTTONPAD property for Lenovos 2015 Benjamin Tissoires
@ 2015-01-28 20:10 ` Benjamin Tissoires
  2015-01-29  7:52 ` [PATCH 0/4] New Lenovos 2015 touchpads: party time! Hans de Goede
  4 siblings, 0 replies; 15+ messages in thread
From: Benjamin Tissoires @ 2015-01-28 20:10 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Hans de Goede, Peter Hutterer, linux-input, linux-kernel

Lenovo decided to switch back to physical buttons for the trackstick
on their latest series. The PNPId list was provided before they reverted
back to physical buttons, so it contains the new models too.
We can know from the touchpad capabilities that the touchpad has physical
buttons, so removing the ids from the list is not mandatory. It is still
nicer to remove the wrong ids, so start by removing the X1 Carbon 3rd gen,
with the PNPId of LEN0048.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/input/mouse/synaptics.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 4f05c9f..b2944ee 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -173,7 +173,6 @@ static const char * const topbuttonpad_pnp_ids[] = {
 	"LEN0045",
 	"LEN0046",
 	"LEN0047",
-	"LEN0048",
 	"LEN0049",
 	"LEN2000",
 	"LEN2001", /* Edge E431 */
-- 
2.1.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/4] New Lenovos 2015 touchpads: party time!
  2015-01-28 20:10 [PATCH 0/4] New Lenovos 2015 touchpads: party time! Benjamin Tissoires
                   ` (3 preceding siblings ...)
  2015-01-28 20:10 ` [PATCH 4/4] Input - synaptics: Remove X1 Carbon 3rd gen from the topbuttonpad list Benjamin Tissoires
@ 2015-01-29  7:52 ` Hans de Goede
  4 siblings, 0 replies; 15+ messages in thread
From: Hans de Goede @ 2015-01-29  7:52 UTC (permalink / raw)
  To: Benjamin Tissoires, Dmitry Torokhov
  Cc: Peter Hutterer, linux-input, linux-kernel

Hi Benjamin & et al,

On 28-01-15 21:10, Benjamin Tissoires wrote:
> Hi Dmitry,
>
> Lenovo released their new models, and believe it or not, some kernel
> work is required to support them!
>
> We learned the lesson and we will try to maximize the effort in user space
> through udev rules instead of polluting the kernel with exceptions.
>
> Patches 1 & 2 are general fixes for synaptics that have been unnoticed
> until this new models. They are marked as stable.
>
> Patch 3 is a generic way to fix the PnpId list that was hardcoded in the
> kernel. This one is also marked as stable so that distributions will not
> have the top software buttons on devices that do not require it.
>
> Patch 4 is a nice to have. It is not mandatory, so not CC-ed to stable,
> it just reduced the PnpId list.
>
> The good thing is that the reported min/max are correct, so the problems
> with the first generation of the t440 touchpad have been fixed now (and
> synaptics assured us that they will keep checking the quality of the PS/2
> handling of their touchpads).
>
> One last thing, I still did not manage to talk to the device over SMBus,
> so we are still stuck with the PS/2 protocol and its limitations for this
> series.

Series looks good to me and is:

Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/4] Input - synaptics: do not release extra buttons once they are pressed
  2015-01-28 20:10 ` [PATCH 2/4] Input - synaptics: do not release extra buttons once they are pressed Benjamin Tissoires
@ 2015-02-02 21:46   ` Dmitry Torokhov
  2015-02-02 22:14     ` Benjamin Tissoires
  0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Torokhov @ 2015-02-02 21:46 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Hans de Goede, Peter Hutterer, linux-input, linux-kernel

On Wed, Jan 28, 2015 at 03:10:05PM -0500, Benjamin Tissoires wrote:
> The current code releases the extra buttons right after they are pressed.
> As soon as a new serio report comes in, the hw state is reset to 0
> and so the buttons are released.
> 
> Check for the report type before acting on the current extra buttons
> state.

No:

"If Ext is 0 (if bit 1 of bytes 1 and 4 are the same) then there are no
external buttons being pressed (or all external buttons have been
released)." - Synaptics PS/2 TouchPad Interfacing Guide PN:
511-000275-01 Rev. B

Thanks.

> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/input/mouse/synaptics.c | 10 +++++++---
>  drivers/input/mouse/synaptics.h |  3 +++
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 0d12664..d58863b 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -688,7 +688,7 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
>  		}
>  
>  		if (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) &&
> -		    ((buf[0] ^ buf[3]) & 0x02)) {
> +		    SYN_REPORT_EXT_BUTTONS(buf)) {
>  			switch (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap)) {
>  			default:
>  				/*
> @@ -792,8 +792,12 @@ static void synaptics_report_buttons(struct psmouse *psmouse,
>  		input_report_key(dev, BTN_BACK, hw->down);
>  	}
>  
> -	for (i = 0; i < SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap); i++)
> -		input_report_key(dev, BTN_0 + i, hw->ext_buttons & (1 << i));
> +	if (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) &&
> +	    SYN_REPORT_EXT_BUTTONS(psmouse->packet)) {
> +		for (i = 0; i < SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap); i++)
> +			input_report_key(dev, BTN_0 + i,
> +					 hw->ext_buttons & (1 << i));
> +	}
>  }
>  
>  static void synaptics_report_mt_data(struct psmouse *psmouse,
> diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
> index 6faf9bb..0b2b711 100644
> --- a/drivers/input/mouse/synaptics.h
> +++ b/drivers/input/mouse/synaptics.h
> @@ -115,6 +115,9 @@
>  #define SYN_NEWABS_RELAXED		2
>  #define SYN_OLDABS			3
>  
> +/* synaptics extended button packet */
> +#define SYN_REPORT_EXT_BUTTONS(buf)	(((buf)[0] ^ (buf)[3]) & 0x02)
> +
>  /* amount to fuzz position data when touchpad reports reduced filtering */
>  #define SYN_REDUCED_FILTER_FUZZ		8
>  
> -- 
> 2.1.0
> 

-- 
Dmitry

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/4] Input - synaptics: remove TOPBUTTONPAD property for Lenovos 2015
  2015-01-28 20:10 ` [PATCH 3/4] Input - synaptics: remove TOPBUTTONPAD property for Lenovos 2015 Benjamin Tissoires
@ 2015-02-02 21:59   ` Dmitry Torokhov
  0 siblings, 0 replies; 15+ messages in thread
From: Dmitry Torokhov @ 2015-02-02 21:59 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Hans de Goede, Peter Hutterer, linux-input, linux-kernel

On Wed, Jan 28, 2015 at 03:10:06PM -0500, Benjamin Tissoires wrote:
> The 2015 series of the Lenovo thinkpads added back the hardware buttons
> on top of the touchpad for the trackstick.
> 
> Unfortunately, they are wired to the touchpad, and not the trackstick.
> Thus, they are seen as extra buttons from the kernel point of view.
> 
> This leads to a problem in user space because extra buttons on synaptics
> devices used to be used as scroll up/down buttons. So in the end, the
> experience for the user is scroll events for buttons left and right
> when using the trackstick. Yay!
> Still, this user space problem will be fixed in the user space, and
> the kernel will only handle the TOPBUTTONPAD property - which was not
> a good idea to introduce in the kernel in the first place, I concede.
> 
> Furthermore, Lenovo kept using the PNPIDs that are supposed to be
> "5 buttons" touchpads, so the new laptops also have the
> INPUT_PROP_TOPBUTTONPAD. Yay again!
> 
> Insteead of manually removing each of the new ones, or hoping that we
> know all the current ones, we can consider that the PNPIDs list that
> were given contains touchpads that have the trackstick buttons, either
> physically wired to them, or emulated with the top software button
> property.
> 
> Thanks to the extra buttons capability, we can reliably detect the
> physical buttons from the software ones, and so we can remove the
> TOPBUTTONPAD property even if it was declared as such.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/input/mouse/synaptics.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index d58863b..4f05c9f 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -1129,7 +1129,8 @@ static void set_input_params(struct psmouse *psmouse,
>  
>  	if (SYN_CAP_CLICKPAD(priv->ext_cap_0c)) {
>  		__set_bit(INPUT_PROP_BUTTONPAD, dev->propbit);
> -		if (psmouse_matches_pnp_id(psmouse, topbuttonpad_pnp_ids))
> +		if (psmouse_matches_pnp_id(psmouse, topbuttonpad_pnp_ids) &&
> +		    !test_bit(BTN_0, dev->keybit))

I do not really like this kind of heuristic, but all
topbuttonpad_pnp_ids are currently Lenovos so I guess it's OK and I'll
apply the patch.

Regarding the tracksick/touchpad buttons I would not mind if we'd mix
them in into pass-through port data - kernel's task is to make sense of
hardware for userspace and in this case raw picture does not seem to
make much sense...

Thanks.

>  			__set_bit(INPUT_PROP_TOPBUTTONPAD, dev->propbit);
>  		/* Clickpads report only left button */
>  		__clear_bit(BTN_RIGHT, dev->keybit);
> -- 
> 2.1.0
> 

-- 
Dmitry

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/4] Input - synaptics: do not release extra buttons once they are pressed
  2015-02-02 21:46   ` Dmitry Torokhov
@ 2015-02-02 22:14     ` Benjamin Tissoires
  2015-02-04 21:29       ` Benjamin Tissoires
  0 siblings, 1 reply; 15+ messages in thread
From: Benjamin Tissoires @ 2015-02-02 22:14 UTC (permalink / raw)
  To: Dmitry Torokhov, Andrew Duggan
  Cc: Benjamin Tissoires, Hans de Goede, Peter Hutterer, linux-input,
	linux-kernel

On Mon, Feb 2, 2015 at 4:46 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Wed, Jan 28, 2015 at 03:10:05PM -0500, Benjamin Tissoires wrote:
>> The current code releases the extra buttons right after they are pressed.
>> As soon as a new serio report comes in, the hw state is reset to 0
>> and so the buttons are released.
>>
>> Check for the report type before acting on the current extra buttons
>> state.
>
> No:
>
> "If Ext is 0 (if bit 1 of bytes 1 and 4 are the same) then there are no
> external buttons being pressed (or all external buttons have been
> released)." - Synaptics PS/2 TouchPad Interfacing Guide PN:
> 511-000275-01 Rev. B
>

Hmm, indeed, the current code follows the spec. Yeah! a new firmware bug!

So when I dumped the ps/2 reports, I clearly saw the release report
being sent with the Ext bit to 1, and other reports were in between
with no information (because no fingers were on the touchpad).

I may have access to the hardware tomorrow if the snow has been
removed from the roads, but tonight, I won't be able to test anything
:(

Cheers,
Benjamin

>
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>>  drivers/input/mouse/synaptics.c | 10 +++++++---
>>  drivers/input/mouse/synaptics.h |  3 +++
>>  2 files changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>> index 0d12664..d58863b 100644
>> --- a/drivers/input/mouse/synaptics.c
>> +++ b/drivers/input/mouse/synaptics.c
>> @@ -688,7 +688,7 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
>>               }
>>
>>               if (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) &&
>> -                 ((buf[0] ^ buf[3]) & 0x02)) {
>> +                 SYN_REPORT_EXT_BUTTONS(buf)) {
>>                       switch (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap)) {
>>                       default:
>>                               /*
>> @@ -792,8 +792,12 @@ static void synaptics_report_buttons(struct psmouse *psmouse,
>>               input_report_key(dev, BTN_BACK, hw->down);
>>       }
>>
>> -     for (i = 0; i < SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap); i++)
>> -             input_report_key(dev, BTN_0 + i, hw->ext_buttons & (1 << i));
>> +     if (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) &&
>> +         SYN_REPORT_EXT_BUTTONS(psmouse->packet)) {
>> +             for (i = 0; i < SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap); i++)
>> +                     input_report_key(dev, BTN_0 + i,
>> +                                      hw->ext_buttons & (1 << i));
>> +     }
>>  }
>>
>>  static void synaptics_report_mt_data(struct psmouse *psmouse,
>> diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
>> index 6faf9bb..0b2b711 100644
>> --- a/drivers/input/mouse/synaptics.h
>> +++ b/drivers/input/mouse/synaptics.h
>> @@ -115,6 +115,9 @@
>>  #define SYN_NEWABS_RELAXED           2
>>  #define SYN_OLDABS                   3
>>
>> +/* synaptics extended button packet */
>> +#define SYN_REPORT_EXT_BUTTONS(buf)  (((buf)[0] ^ (buf)[3]) & 0x02)
>> +
>>  /* amount to fuzz position data when touchpad reports reduced filtering */
>>  #define SYN_REDUCED_FILTER_FUZZ              8
>>
>> --
>> 2.1.0
>>
>
> --
> Dmitry
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/4] Input - synaptics: fix middle button on Lenovo 2015 products
  2015-01-28 20:10 ` [PATCH 1/4] Input - synaptics: fix middle button on Lenovo 2015 products Benjamin Tissoires
@ 2015-02-02 22:54   ` Dmitry Torokhov
  2015-02-04 19:43     ` Benjamin Tissoires
  0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Torokhov @ 2015-02-02 22:54 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Hans de Goede, Peter Hutterer, linux-input, linux-kernel

On Wed, Jan 28, 2015 at 03:10:04PM -0500, Benjamin Tissoires wrote:
> On the X1 Carbon 3rd gen (with a 2015 broadwell cpu), the physical middle
> button of the trackstick (attached to the touchpad serio device, of course)
> seems to get lost.
> 
> Actually, the touchpads reports 3 extra buttons, which falls in the switch
> below to the '2' case. Let's handle the case of odd numbers also, so that
> the middle button finds its way back.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/input/mouse/synaptics.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index f89de89..0d12664 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -689,7 +689,7 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
>  
>  		if (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) &&
>  		    ((buf[0] ^ buf[3]) & 0x02)) {
> -			switch (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) & ~0x01) {
> +			switch (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap)) {
>  			default:
>  				/*
>  				 * if nExtBtn is greater than 8 it should be
> @@ -698,15 +698,19 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
>  				break;
>  			case 8:
>  				hw->ext_buttons |= ((buf[5] & 0x08)) ? 0x80 : 0;
> +			case 7:
>  				hw->ext_buttons |= ((buf[4] & 0x08)) ? 0x40 : 0;
>  			case 6:
>  				hw->ext_buttons |= ((buf[5] & 0x04)) ? 0x20 : 0;
> +			case 5:
>  				hw->ext_buttons |= ((buf[4] & 0x04)) ? 0x10 : 0;
>  			case 4:
>  				hw->ext_buttons |= ((buf[5] & 0x02)) ? 0x08 : 0;
> +			case 3:
>  				hw->ext_buttons |= ((buf[4] & 0x02)) ? 0x04 : 0;
>  			case 2:
>  				hw->ext_buttons |= ((buf[5] & 0x01)) ? 0x02 : 0;
> +			case 1:
>  				hw->ext_buttons |= ((buf[4] & 0x01)) ? 0x01 : 0;
>  			}
>  		}

Hmm, that switch has gotten unwieldy. Maybe we can do someting like
this:

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index a039cb7..1250a5d 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -608,6 +608,18 @@ static void synaptics_parse_agm(const unsigned char buf[],
 	}
 }
 
+static void synaptics_parse_ext_buttons(const unsigned char buf[],
+					struct synaptics_data *priv,
+					struct synaptics_hw_state *hw)
+{
+	unsigned int ext_bits =
+		(SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) + 1) >> 1;
+	unsigned int ext_mask = GENMASK(ext_bits - 1, 0);
+
+	hw->ext_buttons = buf[4] & ext_mask;
+	hw->ext_buttons |= (buf[4] & ext_mask) << ext_bits;
+}
+
 static int synaptics_parse_hw_state(const unsigned char buf[],
 				    struct synaptics_data *priv,
 				    struct synaptics_hw_state *hw)
@@ -692,28 +704,9 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
 			hw->down = ((buf[0] ^ buf[3]) & 0x02) ? 1 : 0;
 		}
 
-		if (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) &&
+		if (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) > 0 &&
 		    ((buf[0] ^ buf[3]) & 0x02)) {
-			switch (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) & ~0x01) {
-			default:
-				/*
-				 * if nExtBtn is greater than 8 it should be
-				 * considered invalid and treated as 0
-				 */
-				break;
-			case 8:
-				hw->ext_buttons |= ((buf[5] & 0x08)) ? 0x80 : 0;
-				hw->ext_buttons |= ((buf[4] & 0x08)) ? 0x40 : 0;
-			case 6:
-				hw->ext_buttons |= ((buf[5] & 0x04)) ? 0x20 : 0;
-				hw->ext_buttons |= ((buf[4] & 0x04)) ? 0x10 : 0;
-			case 4:
-				hw->ext_buttons |= ((buf[5] & 0x02)) ? 0x08 : 0;
-				hw->ext_buttons |= ((buf[4] & 0x02)) ? 0x04 : 0;
-			case 2:
-				hw->ext_buttons |= ((buf[5] & 0x01)) ? 0x02 : 0;
-				hw->ext_buttons |= ((buf[4] & 0x01)) ? 0x01 : 0;
-			}
+			synaptics_parse_ext_buttons(buf, priv, hw);
 		}
 	} else {
 		hw->x = (((buf[1] & 0x1f) << 8) | buf[2]);
@@ -780,6 +773,7 @@ static void synaptics_report_buttons(struct psmouse *psmouse,
 {
 	struct input_dev *dev = psmouse->dev;
 	struct synaptics_data *priv = psmouse->private;
+	int ext_bits = (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) + 1) >> 1;
 	int i;
 
 	input_report_key(dev, BTN_LEFT, hw->left);
@@ -793,8 +787,12 @@ static void synaptics_report_buttons(struct psmouse *psmouse,
 		input_report_key(dev, BTN_BACK, hw->down);
 	}
 
-	for (i = 0; i < SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap); i++)
-		input_report_key(dev, BTN_0 + i, hw->ext_buttons & (1 << i));
+	for (i = 0; i < ext_bits; i++) {
+		input_report_key(dev, BTN_0 + 2 * i,
+				 hw->ext_buttons & (1 << i));
+		input_report_key(dev, BTN_1 + 2 * i,
+				 hw->ext_buttons & (1 << (i + ext_bits)));
+	}
 }
 
 static void synaptics_report_mt_data(struct psmouse *psmouse,

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/4] Input - synaptics: fix middle button on Lenovo 2015 products
  2015-02-02 22:54   ` Dmitry Torokhov
@ 2015-02-04 19:43     ` Benjamin Tissoires
  2015-02-04 19:53       ` Dmitry Torokhov
  0 siblings, 1 reply; 15+ messages in thread
From: Benjamin Tissoires @ 2015-02-04 19:43 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Hans de Goede, Peter Hutterer, linux-input, linux-kernel

On Feb 02 2015 or thereabouts, Dmitry Torokhov wrote:
> On Wed, Jan 28, 2015 at 03:10:04PM -0500, Benjamin Tissoires wrote:
> > On the X1 Carbon 3rd gen (with a 2015 broadwell cpu), the physical middle
> > button of the trackstick (attached to the touchpad serio device, of course)
> > seems to get lost.
> > 
> > Actually, the touchpads reports 3 extra buttons, which falls in the switch
> > below to the '2' case. Let's handle the case of odd numbers also, so that
> > the middle button finds its way back.
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
> >  drivers/input/mouse/synaptics.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> > index f89de89..0d12664 100644
> > --- a/drivers/input/mouse/synaptics.c
> > +++ b/drivers/input/mouse/synaptics.c
> > @@ -689,7 +689,7 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
> >  
> >  		if (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) &&
> >  		    ((buf[0] ^ buf[3]) & 0x02)) {
> > -			switch (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) & ~0x01) {
> > +			switch (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap)) {
> >  			default:
> >  				/*
> >  				 * if nExtBtn is greater than 8 it should be
> > @@ -698,15 +698,19 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
> >  				break;
> >  			case 8:
> >  				hw->ext_buttons |= ((buf[5] & 0x08)) ? 0x80 : 0;
> > +			case 7:
> >  				hw->ext_buttons |= ((buf[4] & 0x08)) ? 0x40 : 0;
> >  			case 6:
> >  				hw->ext_buttons |= ((buf[5] & 0x04)) ? 0x20 : 0;
> > +			case 5:
> >  				hw->ext_buttons |= ((buf[4] & 0x04)) ? 0x10 : 0;
> >  			case 4:
> >  				hw->ext_buttons |= ((buf[5] & 0x02)) ? 0x08 : 0;
> > +			case 3:
> >  				hw->ext_buttons |= ((buf[4] & 0x02)) ? 0x04 : 0;
> >  			case 2:
> >  				hw->ext_buttons |= ((buf[5] & 0x01)) ? 0x02 : 0;
> > +			case 1:
> >  				hw->ext_buttons |= ((buf[4] & 0x01)) ? 0x01 : 0;
> >  			}
> >  		}
> 
> Hmm, that switch has gotten unwieldy. Maybe we can do someting like
> this:

Minus a typo, it works too.

Though I must say even if the switch is ugly, it's a no brainer to
understand what is this about. Here, it requires a little bit more
skills :)

> 
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index a039cb7..1250a5d 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -608,6 +608,18 @@ static void synaptics_parse_agm(const unsigned char buf[],
>  	}
>  }
>  
> +static void synaptics_parse_ext_buttons(const unsigned char buf[],
> +					struct synaptics_data *priv,
> +					struct synaptics_hw_state *hw)
> +{
> +	unsigned int ext_bits =
> +		(SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) + 1) >> 1;
> +	unsigned int ext_mask = GENMASK(ext_bits - 1, 0);
> +
> +	hw->ext_buttons = buf[4] & ext_mask;
> +	hw->ext_buttons |= (buf[4] & ext_mask) << ext_bits;

typo: this should read (buf[5] & ext_mask) << ext_bits; (not buf[4])

You can add my Tested-by on this one when applying.

Cheers,
Benjamin

> +}
> +
>  static int synaptics_parse_hw_state(const unsigned char buf[],
>  				    struct synaptics_data *priv,
>  				    struct synaptics_hw_state *hw)
> @@ -692,28 +704,9 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
>  			hw->down = ((buf[0] ^ buf[3]) & 0x02) ? 1 : 0;
>  		}
>  
> -		if (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) &&
> +		if (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) > 0 &&
>  		    ((buf[0] ^ buf[3]) & 0x02)) {
> -			switch (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) & ~0x01) {
> -			default:
> -				/*
> -				 * if nExtBtn is greater than 8 it should be
> -				 * considered invalid and treated as 0
> -				 */
> -				break;
> -			case 8:
> -				hw->ext_buttons |= ((buf[5] & 0x08)) ? 0x80 : 0;
> -				hw->ext_buttons |= ((buf[4] & 0x08)) ? 0x40 : 0;
> -			case 6:
> -				hw->ext_buttons |= ((buf[5] & 0x04)) ? 0x20 : 0;
> -				hw->ext_buttons |= ((buf[4] & 0x04)) ? 0x10 : 0;
> -			case 4:
> -				hw->ext_buttons |= ((buf[5] & 0x02)) ? 0x08 : 0;
> -				hw->ext_buttons |= ((buf[4] & 0x02)) ? 0x04 : 0;
> -			case 2:
> -				hw->ext_buttons |= ((buf[5] & 0x01)) ? 0x02 : 0;
> -				hw->ext_buttons |= ((buf[4] & 0x01)) ? 0x01 : 0;
> -			}
> +			synaptics_parse_ext_buttons(buf, priv, hw);
>  		}
>  	} else {
>  		hw->x = (((buf[1] & 0x1f) << 8) | buf[2]);
> @@ -780,6 +773,7 @@ static void synaptics_report_buttons(struct psmouse *psmouse,
>  {
>  	struct input_dev *dev = psmouse->dev;
>  	struct synaptics_data *priv = psmouse->private;
> +	int ext_bits = (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) + 1) >> 1;
>  	int i;
>  
>  	input_report_key(dev, BTN_LEFT, hw->left);
> @@ -793,8 +787,12 @@ static void synaptics_report_buttons(struct psmouse *psmouse,
>  		input_report_key(dev, BTN_BACK, hw->down);
>  	}
>  
> -	for (i = 0; i < SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap); i++)
> -		input_report_key(dev, BTN_0 + i, hw->ext_buttons & (1 << i));
> +	for (i = 0; i < ext_bits; i++) {
> +		input_report_key(dev, BTN_0 + 2 * i,
> +				 hw->ext_buttons & (1 << i));
> +		input_report_key(dev, BTN_1 + 2 * i,
> +				 hw->ext_buttons & (1 << (i + ext_bits)));
> +	}
>  }
>  
>  static void synaptics_report_mt_data(struct psmouse *psmouse,
> 
> Thanks.
> 
> -- 
> Dmitry

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/4] Input - synaptics: fix middle button on Lenovo 2015 products
  2015-02-04 19:43     ` Benjamin Tissoires
@ 2015-02-04 19:53       ` Dmitry Torokhov
  0 siblings, 0 replies; 15+ messages in thread
From: Dmitry Torokhov @ 2015-02-04 19:53 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Hans de Goede, Peter Hutterer, linux-input, linux-kernel

On Wed, Feb 04, 2015 at 02:43:32PM -0500, Benjamin Tissoires wrote:
> On Feb 02 2015 or thereabouts, Dmitry Torokhov wrote:
> > On Wed, Jan 28, 2015 at 03:10:04PM -0500, Benjamin Tissoires wrote:
> > > On the X1 Carbon 3rd gen (with a 2015 broadwell cpu), the physical middle
> > > button of the trackstick (attached to the touchpad serio device, of course)
> > > seems to get lost.
> > > 
> > > Actually, the touchpads reports 3 extra buttons, which falls in the switch
> > > below to the '2' case. Let's handle the case of odd numbers also, so that
> > > the middle button finds its way back.
> > > 
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > ---
> > >  drivers/input/mouse/synaptics.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> > > index f89de89..0d12664 100644
> > > --- a/drivers/input/mouse/synaptics.c
> > > +++ b/drivers/input/mouse/synaptics.c
> > > @@ -689,7 +689,7 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
> > >  
> > >  		if (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) &&
> > >  		    ((buf[0] ^ buf[3]) & 0x02)) {
> > > -			switch (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) & ~0x01) {
> > > +			switch (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap)) {
> > >  			default:
> > >  				/*
> > >  				 * if nExtBtn is greater than 8 it should be
> > > @@ -698,15 +698,19 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
> > >  				break;
> > >  			case 8:
> > >  				hw->ext_buttons |= ((buf[5] & 0x08)) ? 0x80 : 0;
> > > +			case 7:
> > >  				hw->ext_buttons |= ((buf[4] & 0x08)) ? 0x40 : 0;
> > >  			case 6:
> > >  				hw->ext_buttons |= ((buf[5] & 0x04)) ? 0x20 : 0;
> > > +			case 5:
> > >  				hw->ext_buttons |= ((buf[4] & 0x04)) ? 0x10 : 0;
> > >  			case 4:
> > >  				hw->ext_buttons |= ((buf[5] & 0x02)) ? 0x08 : 0;
> > > +			case 3:
> > >  				hw->ext_buttons |= ((buf[4] & 0x02)) ? 0x04 : 0;
> > >  			case 2:
> > >  				hw->ext_buttons |= ((buf[5] & 0x01)) ? 0x02 : 0;
> > > +			case 1:
> > >  				hw->ext_buttons |= ((buf[4] & 0x01)) ? 0x01 : 0;
> > >  			}
> > >  		}
> > 
> > Hmm, that switch has gotten unwieldy. Maybe we can do someting like
> > this:
> 
> Minus a typo, it works too.
> 
> Though I must say even if the switch is ugly, it's a no brainer to
> understand what is this about. Here, it requires a little bit more
> skills :)

Yeah, it really wasn't about making code that much clearer but rather
about number of operations we need to make to figure button state.
Switch plus bazillion of ternary operations results in many many
branches, the new version does not have any.

> 
> > 
> > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> > index a039cb7..1250a5d 100644
> > --- a/drivers/input/mouse/synaptics.c
> > +++ b/drivers/input/mouse/synaptics.c
> > @@ -608,6 +608,18 @@ static void synaptics_parse_agm(const unsigned char buf[],
> >  	}
> >  }
> >  
> > +static void synaptics_parse_ext_buttons(const unsigned char buf[],
> > +					struct synaptics_data *priv,
> > +					struct synaptics_hw_state *hw)
> > +{
> > +	unsigned int ext_bits =
> > +		(SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) + 1) >> 1;
> > +	unsigned int ext_mask = GENMASK(ext_bits - 1, 0);
> > +
> > +	hw->ext_buttons = buf[4] & ext_mask;
> > +	hw->ext_buttons |= (buf[4] & ext_mask) << ext_bits;
> 
> typo: this should read (buf[5] & ext_mask) << ext_bits; (not buf[4])

Ah, yes, indeed! Thanks for spotting this.

> 
> You can add my Tested-by on this one when applying.

Great, I'll queue it then.

> 
> Cheers,
> Benjamin
> 
> > +}
> > +
> >  static int synaptics_parse_hw_state(const unsigned char buf[],
> >  				    struct synaptics_data *priv,
> >  				    struct synaptics_hw_state *hw)
> > @@ -692,28 +704,9 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
> >  			hw->down = ((buf[0] ^ buf[3]) & 0x02) ? 1 : 0;
> >  		}
> >  
> > -		if (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) &&
> > +		if (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) > 0 &&
> >  		    ((buf[0] ^ buf[3]) & 0x02)) {
> > -			switch (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) & ~0x01) {
> > -			default:
> > -				/*
> > -				 * if nExtBtn is greater than 8 it should be
> > -				 * considered invalid and treated as 0
> > -				 */
> > -				break;
> > -			case 8:
> > -				hw->ext_buttons |= ((buf[5] & 0x08)) ? 0x80 : 0;
> > -				hw->ext_buttons |= ((buf[4] & 0x08)) ? 0x40 : 0;
> > -			case 6:
> > -				hw->ext_buttons |= ((buf[5] & 0x04)) ? 0x20 : 0;
> > -				hw->ext_buttons |= ((buf[4] & 0x04)) ? 0x10 : 0;
> > -			case 4:
> > -				hw->ext_buttons |= ((buf[5] & 0x02)) ? 0x08 : 0;
> > -				hw->ext_buttons |= ((buf[4] & 0x02)) ? 0x04 : 0;
> > -			case 2:
> > -				hw->ext_buttons |= ((buf[5] & 0x01)) ? 0x02 : 0;
> > -				hw->ext_buttons |= ((buf[4] & 0x01)) ? 0x01 : 0;
> > -			}
> > +			synaptics_parse_ext_buttons(buf, priv, hw);
> >  		}
> >  	} else {
> >  		hw->x = (((buf[1] & 0x1f) << 8) | buf[2]);
> > @@ -780,6 +773,7 @@ static void synaptics_report_buttons(struct psmouse *psmouse,
> >  {
> >  	struct input_dev *dev = psmouse->dev;
> >  	struct synaptics_data *priv = psmouse->private;
> > +	int ext_bits = (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) + 1) >> 1;
> >  	int i;
> >  
> >  	input_report_key(dev, BTN_LEFT, hw->left);
> > @@ -793,8 +787,12 @@ static void synaptics_report_buttons(struct psmouse *psmouse,
> >  		input_report_key(dev, BTN_BACK, hw->down);
> >  	}
> >  
> > -	for (i = 0; i < SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap); i++)
> > -		input_report_key(dev, BTN_0 + i, hw->ext_buttons & (1 << i));
> > +	for (i = 0; i < ext_bits; i++) {
> > +		input_report_key(dev, BTN_0 + 2 * i,
> > +				 hw->ext_buttons & (1 << i));
> > +		input_report_key(dev, BTN_1 + 2 * i,
> > +				 hw->ext_buttons & (1 << (i + ext_bits)));
> > +	}
> >  }
> >  
> >  static void synaptics_report_mt_data(struct psmouse *psmouse,
> > 
> > Thanks.
> > 
> > -- 
> > Dmitry

-- 
Dmitry

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/4] Input - synaptics: do not release extra buttons once they are pressed
  2015-02-02 22:14     ` Benjamin Tissoires
@ 2015-02-04 21:29       ` Benjamin Tissoires
  2015-02-05  1:53         ` Andrew Duggan
  2015-02-06  1:54         ` Andrew Duggan
  0 siblings, 2 replies; 15+ messages in thread
From: Benjamin Tissoires @ 2015-02-04 21:29 UTC (permalink / raw)
  To: Dmitry Torokhov, Andrew Duggan
  Cc: Hans de Goede, Peter Hutterer, linux-input, linux-kernel

On Feb 02 2015 or thereabouts, Benjamin Tissoires wrote:
> On Mon, Feb 2, 2015 at 4:46 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Wed, Jan 28, 2015 at 03:10:05PM -0500, Benjamin Tissoires wrote:
> >> The current code releases the extra buttons right after they are pressed.
> >> As soon as a new serio report comes in, the hw state is reset to 0
> >> and so the buttons are released.
> >>
> >> Check for the report type before acting on the current extra buttons
> >> state.
> >
> > No:
> >
> > "If Ext is 0 (if bit 1 of bytes 1 and 4 are the same) then there are no
> > external buttons being pressed (or all external buttons have been
> > released)." - Synaptics PS/2 TouchPad Interfacing Guide PN:
> > 511-000275-01 Rev. B
> >
> 
> Hmm, indeed, the current code follows the spec. Yeah! a new firmware bug!
> 
> So when I dumped the ps/2 reports, I clearly saw the release report
> being sent with the Ext bit to 1, and other reports were in between
> with no information (because no fingers were on the touchpad).
> 

OK, so with the hardware, here are the results:

After each incoming event (being a button or touch), when there is no
more information to send (i.e. touch released or button pressed or
released), I receive 80 times the following buffer:
80 00 00 c0 00 00

The number 80 seems quite consistant, though I got once 96.
Not sure that this repeated buffer might be of interest however.

When a finger is touched on the sensor, I receive the following:

b0 ba 35 c0 8d ed <- first finger down on the sensor
....
b0 ba 3b c0 6f d0
b0 ba 3b c2 6d d0 <- button 1 pressed
b0 ba 3b c0 6f d0
....
b0 ba 3b c0 6f d0
b0 ba 3c c2 6c d0 <- button 1 released
b0 ba 3c c0 6f d0
....
90 ba 26 c0 6f d0
80 00 00 c0 00 00 <- first finger released from the sensor
80 00 00 c0 00 00 <- repeated 80 times

So here, either the spec is wrong, either the Synaptics with FW 8.1 in
the Lenovos are not following it. But I clearly see that the extended
buttons are reported only when the Ext bit is 1.

Andrew, could you help us determine which way to go?
Ideally, could you point out at a firmware version where we could be
sure that the spec has not been followed so we can add a quirk in the
driver?

Cheers,
Benjamin

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/4] Input - synaptics: do not release extra buttons once they are pressed
  2015-02-04 21:29       ` Benjamin Tissoires
@ 2015-02-05  1:53         ` Andrew Duggan
  2015-02-06  1:54         ` Andrew Duggan
  1 sibling, 0 replies; 15+ messages in thread
From: Andrew Duggan @ 2015-02-05  1:53 UTC (permalink / raw)
  To: Benjamin Tissoires, Dmitry Torokhov
  Cc: Hans de Goede, Peter Hutterer, linux-input, linux-kernel

On 02/04/2015 01:29 PM, Benjamin Tissoires wrote:
> On Feb 02 2015 or thereabouts, Benjamin Tissoires wrote:
>> On Mon, Feb 2, 2015 at 4:46 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>>> On Wed, Jan 28, 2015 at 03:10:05PM -0500, Benjamin Tissoires wrote:
>>>> The current code releases the extra buttons right after they are pressed.
>>>> As soon as a new serio report comes in, the hw state is reset to 0
>>>> and so the buttons are released.
>>>>
>>>> Check for the report type before acting on the current extra buttons
>>>> state.
>>> No:
>>>
>>> "If Ext is 0 (if bit 1 of bytes 1 and 4 are the same) then there are no
>>> external buttons being pressed (or all external buttons have been
>>> released)." - Synaptics PS/2 TouchPad Interfacing Guide PN:
>>> 511-000275-01 Rev. B
>>>
>> Hmm, indeed, the current code follows the spec. Yeah! a new firmware bug!
>>
>> So when I dumped the ps/2 reports, I clearly saw the release report
>> being sent with the Ext bit to 1, and other reports were in between
>> with no information (because no fingers were on the touchpad).
>>
> OK, so with the hardware, here are the results:
>
> After each incoming event (being a button or touch), when there is no
> more information to send (i.e. touch released or button pressed or
> released), I receive 80 times the following buffer:
> 80 00 00 c0 00 00

Yeah, this is normal. The firmware reports this for 1 second after a 
finger lift. The last paragraph of 3.2 mentions this behavior.

> The number 80 seems quite consistant, though I got once 96.
> Not sure that this repeated buffer might be of interest however.
>
> When a finger is touched on the sensor, I receive the following:
>
> b0 ba 35 c0 8d ed <- first finger down on the sensor
> ....
> b0 ba 3b c0 6f d0
> b0 ba 3b c2 6d d0 <- button 1 pressed
> b0 ba 3b c0 6f d0
> ....
> b0 ba 3b c0 6f d0
> b0 ba 3c c2 6c d0 <- button 1 released
> b0 ba 3c c0 6f d0
> ....
> 90 ba 26 c0 6f d0
> 80 00 00 c0 00 00 <- first finger released from the sensor
> 80 00 00 c0 00 00 <- repeated 80 times
>
> So here, either the spec is wrong, either the Synaptics with FW 8.1 in
> the Lenovos are not following it. But I clearly see that the extended
> buttons are reported only when the Ext bit is 1.

I will have to clarify with those who are more familiar with PS/2. The 
spec might not be up to date since it looks like it was last updated in 
2011.

Andrew

>
> Andrew, could you help us determine which way to go?
> Ideally, could you point out at a firmware version where we could be
> sure that the spec has not been followed so we can add a quirk in the
> driver?
>
> Cheers,
> Benjamin


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/4] Input - synaptics: do not release extra buttons once they are pressed
  2015-02-04 21:29       ` Benjamin Tissoires
  2015-02-05  1:53         ` Andrew Duggan
@ 2015-02-06  1:54         ` Andrew Duggan
  1 sibling, 0 replies; 15+ messages in thread
From: Andrew Duggan @ 2015-02-06  1:54 UTC (permalink / raw)
  To: Benjamin Tissoires, Dmitry Torokhov
  Cc: Hans de Goede, Peter Hutterer, linux-input, linux-kernel

On 02/04/2015 01:29 PM, Benjamin Tissoires wrote:
> On Feb 02 2015 or thereabouts, Benjamin Tissoires wrote:
>> On Mon, Feb 2, 2015 at 4:46 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>>> On Wed, Jan 28, 2015 at 03:10:05PM -0500, Benjamin Tissoires wrote:
>>>> The current code releases the extra buttons right after they are pressed.
>>>> As soon as a new serio report comes in, the hw state is reset to 0
>>>> and so the buttons are released.
>>>>
>>>> Check for the report type before acting on the current extra buttons
>>>> state.
>>> No:
>>>
>>> "If Ext is 0 (if bit 1 of bytes 1 and 4 are the same) then there are no
>>> external buttons being pressed (or all external buttons have been
>>> released)." - Synaptics PS/2 TouchPad Interfacing Guide PN:
>>> 511-000275-01 Rev. B
>>>
>> Hmm, indeed, the current code follows the spec. Yeah! a new firmware bug!
>>
>> So when I dumped the ps/2 reports, I clearly saw the release report
>> being sent with the Ext bit to 1, and other reports were in between
>> with no information (because no fingers were on the touchpad).
>>
> OK, so with the hardware, here are the results:
>
> After each incoming event (being a button or touch), when there is no
> more information to send (i.e. touch released or button pressed or
> released), I receive 80 times the following buffer:
> 80 00 00 c0 00 00
>
> The number 80 seems quite consistant, though I got once 96.
> Not sure that this repeated buffer might be of interest however.
>
> When a finger is touched on the sensor, I receive the following:
>
> b0 ba 35 c0 8d ed <- first finger down on the sensor
> ....
> b0 ba 3b c0 6f d0
> b0 ba 3b c2 6d d0 <- button 1 pressed
> b0 ba 3b c0 6f d0
> ....
> b0 ba 3b c0 6f d0
> b0 ba 3c c2 6c d0 <- button 1 released
> b0 ba 3c c0 6f d0
> ....
> 90 ba 26 c0 6f d0
> 80 00 00 c0 00 00 <- first finger released from the sensor
> 80 00 00 c0 00 00 <- repeated 80 times
>
> So here, either the spec is wrong, either the Synaptics with FW 8.1 in
> the Lenovos are not following it. But I clearly see that the extended
> buttons are reported only when the Ext bit is 1.
>
> Andrew, could you help us determine which way to go?
> Ideally, could you point out at a firmware version where we could be
> sure that the spec has not been followed so we can add a quirk in the
> driver?

I got confirmation that this is a firmware bug and that it has probably 
been there since 2011. This bug is definitely in 8.1 and probably 
several versions before it. But, we haven't shipped anything with 
extended buttons in a long time, so applying the quirk to firmware 
revision 8.1 should be adequate. When we fix the bug we will bump the 
minor version.

Andrew

> Cheers,
> Benjamin


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2015-02-06  1:57 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-28 20:10 [PATCH 0/4] New Lenovos 2015 touchpads: party time! Benjamin Tissoires
2015-01-28 20:10 ` [PATCH 1/4] Input - synaptics: fix middle button on Lenovo 2015 products Benjamin Tissoires
2015-02-02 22:54   ` Dmitry Torokhov
2015-02-04 19:43     ` Benjamin Tissoires
2015-02-04 19:53       ` Dmitry Torokhov
2015-01-28 20:10 ` [PATCH 2/4] Input - synaptics: do not release extra buttons once they are pressed Benjamin Tissoires
2015-02-02 21:46   ` Dmitry Torokhov
2015-02-02 22:14     ` Benjamin Tissoires
2015-02-04 21:29       ` Benjamin Tissoires
2015-02-05  1:53         ` Andrew Duggan
2015-02-06  1:54         ` Andrew Duggan
2015-01-28 20:10 ` [PATCH 3/4] Input - synaptics: remove TOPBUTTONPAD property for Lenovos 2015 Benjamin Tissoires
2015-02-02 21:59   ` Dmitry Torokhov
2015-01-28 20:10 ` [PATCH 4/4] Input - synaptics: Remove X1 Carbon 3rd gen from the topbuttonpad list Benjamin Tissoires
2015-01-29  7:52 ` [PATCH 0/4] New Lenovos 2015 touchpads: party time! Hans de Goede

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).