[PATCH 0/4] New Lenovos 2015 touchpads: party time!

[PATCH 0/4] New Lenovos 2015 touchpads: party time!

[Benjamin Tissoires]

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

[PATCH 1/4] Input - synaptics: fix middle button on Lenovo 2015 products

[Benjamin Tissoires]

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

[PATCH 2/4] Input - synaptics: do not release extra buttons once they are pressed

[Benjamin Tissoires]

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

[PATCH 3/4] Input - synaptics: remove TOPBUTTONPAD property for Lenovos 2015

[Benjamin Tissoires]

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

[PATCH 4/4] Input - synaptics: Remove X1 Carbon 3rd gen from the topbuttonpad list

[Benjamin Tissoires]

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

Re: [PATCH 0/4] New Lenovos 2015 touchpads: party time!

[Hans de Goede]

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

Re: [PATCH 2/4] Input - synaptics: do not release extra buttons once they are pressed

[Dmitry Torokhov]

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

Re: [PATCH 3/4] Input - synaptics: remove TOPBUTTONPAD property for Lenovos 2015

[Dmitry Torokhov]

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

Re: [PATCH 2/4] Input - synaptics: do not release extra buttons once they are pressed

[Benjamin Tissoires]

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

Re: [PATCH 1/4] Input - synaptics: fix middle button on Lenovo 2015 products

[Dmitry Torokhov]

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

Re: [PATCH 1/4] Input - synaptics: fix middle button on Lenovo 2015 products

[Benjamin Tissoires]

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

Re: [PATCH 1/4] Input - synaptics: fix middle button on Lenovo 2015 products

[Dmitry Torokhov]

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

Re: [PATCH 2/4] Input - synaptics: do not release extra buttons once they are pressed

[Benjamin Tissoires]

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

Re: [PATCH 2/4] Input - synaptics: do not release extra buttons once they are pressed

[Andrew Duggan]

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

Re: [PATCH 2/4] Input - synaptics: do not release extra buttons once they are pressed

[Andrew Duggan]

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