[PATCH 00/12] Synaptics image sensor support

[PATCH 00/12] Synaptics image sensor support

[djkurtz]

From: Daniel Kurtz <djkurtz@chromium.org>

Hello,

This patch set (against next) is intended to add support for synaptics
"image sensor" touchpads.

Patches 1-8 clean up the current driver slightly and prepare for the image
sensor patches which follow.

Patches 9-11 add up to 3 finger support for image sensor touchpads.
Image sensors do not suffer from the finger tracking issues that plagued
the earlier "profile sensors", and which required the invention of "semi-mt"
(which reports a bounding box around two fingers instead of the fingers
themselves).  Instead, the image sensors report the actual positions of two
fingers using the same "Advanced Gesture Mode". Unfortunately, the limitations
of the Synaptics PS/2 protocol make it difficult to keep track of which two
fingers are being reported at any given time.

Patch 12 adds up to 5 finger support.
In fact, the image sensor touchpads actually track 5 fingers while reporting
just 2 finger positions.  This patch tracks which fingers are being reported,
and passes them up to userspace using 5 MT-B slots.

These patches are similar to, and inspired by, a similar patchset recently
submitted by Derek Foreman and Daniel Stone.  However, it is not directly built
upon, nor is it compatible with, those patches.

Thanks,
Daniel

Daniel Kurtz (12):
  Input: synaptics - cleanup 0x0c query documentation
  Input: synaptics - do not invert y if 0
  Input: synaptics - fix minimum reported ABS_TOOL_WIDTH
  Input: synaptics - set resolution for MT_POSITION_X/Y axes
  Input: synaptics - process button bits in AGM packets
  Input: synaptics - fuzz position if touchpad reports reduced
    filtering
  Input: synaptics - rename synaptics_data.mt to agm
  Input: synaptics - rename set_slot to be more descriptive
  Input: synaptics - add image sensor support
  Input: synaptics - decode AGM packet types
  Input: synaptics - process finger (<=3) transitions
  Input: synaptics - process finger (<=5) transitions

 drivers/input/mouse/synaptics.c |  442 +++++++++++++++++++++++++++++++++++----
 drivers/input/mouse/synaptics.h |   41 +++-
 include/linux/input.h           |    1 +
 3 files changed, 440 insertions(+), 44 deletions(-)

-- 
1.7.3.1

[PATCH 01/12] Input: synaptics - cleanup 0x0c query documentation

[djkurtz]

From: Daniel Kurtz <djkurtz@chromium.org>

Convert some spaces to tabs.
Reorder defines to match bit ordering.
Add defines for all bits.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/mouse/synaptics.h |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
index 7453938..34bedde 100644
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -66,18 +66,24 @@
  * 1	0x60	multifinger mode	identifies firmware finger counting
  *					(not reporting!) algorithm.
  *					Not particularly meaningful
- * 1	0x80    covered pad		W clipped to 14, 15 == pad mostly covered
- * 2	0x01    clickpad bit 1		2-button ClickPad
- * 2	0x02    deluxe LED controls	touchpad support LED commands
+ * 1	0x80	covered pad		W clipped to 14, 15 == pad mostly covered
+ * 2	0x01	clickpad bit 1		2-button ClickPad
+ * 2	0x02	deluxe LED controls	touchpad support LED commands
  *					ala multimedia control bar
  * 2	0x04	reduced filtering	firmware does less filtering on
  *					position data, driver should watch
  *					for noise.
  */
-#define SYN_CAP_CLICKPAD(ex0c)		((ex0c) & 0x100000) /* 1-button ClickPad */
-#define SYN_CAP_CLICKPAD2BTN(ex0c)	((ex0c) & 0x000100) /* 2-button ClickPad */
+#define SYN_CAP_ADJ_THRESHOLD(ex0c)	((ex0c) & 0x010000)
 #define SYN_CAP_MAX_DIMENSIONS(ex0c)	((ex0c) & 0x020000)
+#define SYN_CAP_CLEARPAD(ex0c)		((ex0c) & 0x040000)
 #define SYN_CAP_ADV_GESTURE(ex0c)	((ex0c) & 0x080000)
+#define SYN_CAP_CLICKPAD(ex0c)		((ex0c) & 0x100000) /* 1-button ClickPad */
+#define SYN_CAP_MULTIFINGER_MODE(ex0c)	((ex0c) & 0x600000)
+#define SYN_CAP_COVERED_PAD(ex0c)	((ex0c) & 0x800000)
+#define SYN_CAP_CLICKPAD2BTN(ex0c)	((ex0c) & 0x000100) /* 2-button ClickPad */
+#define SYN_CAP_DELUXE_LED(ex0c)	((ex0c) & 0x000200)
+#define SYN_CAP_REDUCED_FILTERING(ex0c)	((ex0c) & 0x000400)
 
 /* synaptics modes query bits */
 #define SYN_MODE_ABSOLUTE(m)		((m) & (1 << 7))
-- 
1.7.3.1

Re: [PATCH 01/12] Input: synaptics - cleanup 0x0c query documentation

[Chase Douglas]

On 06/28/2011 10:07 PM, djkurtz@chromium.org wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
> 
> Convert some spaces to tabs.
> Reorder defines to match bit ordering.
> Add defines for all bits.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/input/mouse/synaptics.h |   16 +++++++++++-----
>  1 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
> index 7453938..34bedde 100644
> --- a/drivers/input/mouse/synaptics.h
> +++ b/drivers/input/mouse/synaptics.h
> @@ -66,18 +66,24 @@
>   * 1	0x60	multifinger mode	identifies firmware finger counting
>   *					(not reporting!) algorithm.
>   *					Not particularly meaningful
> - * 1	0x80    covered pad		W clipped to 14, 15 == pad mostly covered
> - * 2	0x01    clickpad bit 1		2-button ClickPad
> - * 2	0x02    deluxe LED controls	touchpad support LED commands
> + * 1	0x80	covered pad		W clipped to 14, 15 == pad mostly covered
> + * 2	0x01	clickpad bit 1		2-button ClickPad
> + * 2	0x02	deluxe LED controls	touchpad support LED commands
>   *					ala multimedia control bar
>   * 2	0x04	reduced filtering	firmware does less filtering on
>   *					position data, driver should watch
>   *					for noise.
>   */
> -#define SYN_CAP_CLICKPAD(ex0c)		((ex0c) & 0x100000) /* 1-button ClickPad */
> -#define SYN_CAP_CLICKPAD2BTN(ex0c)	((ex0c) & 0x000100) /* 2-button ClickPad */
> +#define SYN_CAP_ADJ_THRESHOLD(ex0c)	((ex0c) & 0x010000)
>  #define SYN_CAP_MAX_DIMENSIONS(ex0c)	((ex0c) & 0x020000)
> +#define SYN_CAP_CLEARPAD(ex0c)		((ex0c) & 0x040000)
>  #define SYN_CAP_ADV_GESTURE(ex0c)	((ex0c) & 0x080000)
> +#define SYN_CAP_CLICKPAD(ex0c)		((ex0c) & 0x100000) /* 1-button ClickPad */
> +#define SYN_CAP_MULTIFINGER_MODE(ex0c)	((ex0c) & 0x600000)
> +#define SYN_CAP_COVERED_PAD(ex0c)	((ex0c) & 0x800000)
> +#define SYN_CAP_CLICKPAD2BTN(ex0c)	((ex0c) & 0x000100) /* 2-button ClickPad */
> +#define SYN_CAP_DELUXE_LED(ex0c)	((ex0c) & 0x000200)
> +#define SYN_CAP_REDUCED_FILTERING(ex0c)	((ex0c) & 0x000400)
>  
>  /* synaptics modes query bits */
>  #define SYN_MODE_ABSOLUTE(m)		((m) & (1 << 7))

I may not be the most knowledgeable developer on Synaptics, but I think
I'm somewhat knowledgeable and I can't figure out what each of the
bitmasks does now. Since we now have many of them defined, could you add
documentation about what each means? For example, I don't know if
SYN_CAP_CLICKPAD is set in conjunction with SYN_CAP_CLICKPAD2BTN, or if
they are mutually exclusive. I also don't know what SYN_CAP_CLICKPAD2BTN
means anyways :).

I'm curious, are you basing this work on documentation or
reverse-engineering? Maybe you can't say :). It would be awesome if
someone at Synaptics would just release the documentation for these
advanced features already...

Thanks!

-- Chase

Re: [PATCH 01/12] Input: synaptics - cleanup 0x0c query documentation

[Daniel Kurtz]

Hi Chase,

On Wed, Jul 6, 2011 at 1:33 AM, Chase Douglas
<chase.douglas@canonical.com> wrote:
> On 06/28/2011 10:07 PM, djkurtz@chromium.org wrote:
>> From: Daniel Kurtz <djkurtz@chromium.org>
>>
>> Convert some spaces to tabs.
>> Reorder defines to match bit ordering.
>> Add defines for all bits.
>>
>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>> ---
>>  drivers/input/mouse/synaptics.h |   16 +++++++++++-----
>>  1 files changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
>> index 7453938..34bedde 100644
>> --- a/drivers/input/mouse/synaptics.h
>> +++ b/drivers/input/mouse/synaptics.h
>> @@ -66,18 +66,24 @@
>>   * 1 0x60    multifinger mode        identifies firmware finger counting
>>   *                                   (not reporting!) algorithm.
>>   *                                   Not particularly meaningful
>> - * 1 0x80    covered pad             W clipped to 14, 15 == pad mostly covered
>> - * 2 0x01    clickpad bit 1          2-button ClickPad
>> - * 2 0x02    deluxe LED controls     touchpad support LED commands
>> + * 1 0x80    covered pad             W clipped to 14, 15 == pad mostly covered
>> + * 2 0x01    clickpad bit 1          2-button ClickPad
>> + * 2 0x02    deluxe LED controls     touchpad support LED commands
>>   *                                   ala multimedia control bar
>>   * 2 0x04    reduced filtering       firmware does less filtering on
>>   *                                   position data, driver should watch
>>   *                                   for noise.
>>   */
>> -#define SYN_CAP_CLICKPAD(ex0c)               ((ex0c) & 0x100000) /* 1-button ClickPad */
>> -#define SYN_CAP_CLICKPAD2BTN(ex0c)   ((ex0c) & 0x000100) /* 2-button ClickPad */
>> +#define SYN_CAP_ADJ_THRESHOLD(ex0c)  ((ex0c) & 0x010000)
>>  #define SYN_CAP_MAX_DIMENSIONS(ex0c) ((ex0c) & 0x020000)
>> +#define SYN_CAP_CLEARPAD(ex0c)               ((ex0c) & 0x040000)
>>  #define SYN_CAP_ADV_GESTURE(ex0c)    ((ex0c) & 0x080000)
>> +#define SYN_CAP_CLICKPAD(ex0c)               ((ex0c) & 0x100000) /* 1-button ClickPad */
>> +#define SYN_CAP_MULTIFINGER_MODE(ex0c)       ((ex0c) & 0x600000)
>> +#define SYN_CAP_COVERED_PAD(ex0c)    ((ex0c) & 0x800000)
>> +#define SYN_CAP_CLICKPAD2BTN(ex0c)   ((ex0c) & 0x000100) /* 2-button ClickPad */
>> +#define SYN_CAP_DELUXE_LED(ex0c)     ((ex0c) & 0x000200)
>> +#define SYN_CAP_REDUCED_FILTERING(ex0c)      ((ex0c) & 0x000400)
>>
>>  /* synaptics modes query bits */
>>  #define SYN_MODE_ABSOLUTE(m)         ((m) & (1 << 7))
>
> I may not be the most knowledgeable developer on Synaptics, but I think
> I'm somewhat knowledgeable and I can't figure out what each of the
> bitmasks does now. Since we now have many of them defined, could you add
> documentation about what each means? For example, I don't know if
> SYN_CAP_CLICKPAD is set in conjunction with SYN_CAP_CLICKPAD2BTN, or if
> they are mutually exclusive. I also don't know what SYN_CAP_CLICKPAD2BTN
> means anyways :).

Me neither.  All this patch does is convert some spaces to tabs, and
reorder the defines so that their order matches that of the
descriptions.  The descriptions are what Dmitry added in a previous
commit.

>
> I'm curious, are you basing this work on documentation or
> reverse-engineering? Maybe you can't say :). It would be awesome if
> someone at Synaptics would just release the documentation for these
> advanced features already...
>
> Thanks!
>
> -- Chase
>

Re: [PATCH 01/12] Input: synaptics - cleanup 0x0c query documentation

[Dmitry Torokhov]

On Wed, Jun 29, 2011 at 01:07:11PM +0800, djkurtz@chromium.org wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
> 
> Convert some spaces to tabs.
> Reorder defines to match bit ordering.
> Add defines for all bits.

I'd prefer if we added defines for bits we are actually using, so I am
going to drop this patch (sans the bits I folded into fuzz patch).

Thanks.

-- 
Dmitry

[PATCH 02/12] Input: synaptics - do not invert y if 0

[djkurtz]

From: Daniel Kurtz <djkurtz@chromium.org>

Synaptics touchpads report increasing y from bottom to top.
This is inverted from normal userspace "top of screen is 0" coordinates.
Thus, the kernel driver reports inverted y coordinates to userspace.

In some cases, however, y = 0 is sent by the touchpad.
In these cases, the kernel driver should not invert, and just report 0.

This patch also refactors the inversion into a macro, and moves it
into packet processing instead of during position reporting.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/mouse/synaptics.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index e06e045..f6d0c04 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -44,6 +44,8 @@
 #define YMIN_NOMINAL 1408
 #define YMAX_NOMINAL 4448
 
+#define INVERT_Y(y) (((y) != 0) ? (YMAX_NOMINAL + YMIN_NOMINAL - (y)) : 0)
+
 
 /*****************************************************************************
  *	Stuff we need even when we do not want native Synaptics support
@@ -409,9 +411,9 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
 		hw->x = (((buf[3] & 0x10) << 8) |
 			 ((buf[1] & 0x0f) << 8) |
 			 buf[4]);
-		hw->y = (((buf[3] & 0x20) << 7) |
+		hw->y = INVERT_Y((((buf[3] & 0x20) << 7) |
 			 ((buf[1] & 0xf0) << 4) |
-			 buf[5]);
+			 buf[5]));
 
 		hw->z = buf[2];
 		hw->w = (((buf[0] & 0x30) >> 2) |
@@ -421,7 +423,8 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
 		if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) && hw->w == 2) {
 			/* Gesture packet: (x, y, z) at half resolution */
 			priv->mt.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
-			priv->mt.y = (((buf[4] & 0xf0) << 4) | buf[2]) << 1;
+			priv->mt.y = INVERT_Y((((buf[4] & 0xf0) << 4)
+					      | buf[2]) << 1);
 			priv->mt.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
 			return 1;
 		}
@@ -473,7 +476,7 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
 		}
 	} else {
 		hw->x = (((buf[1] & 0x1f) << 8) | buf[2]);
-		hw->y = (((buf[4] & 0x1f) << 8) | buf[5]);
+		hw->y = INVERT_Y(((buf[4] & 0x1f) << 8) | buf[5]);
 
 		hw->z = (((buf[0] & 0x30) << 2) | (buf[3] & 0x3F));
 		hw->w = (((buf[1] & 0x80) >> 4) | ((buf[0] & 0x04) >> 1));
@@ -491,8 +494,7 @@ static void set_slot(struct input_dev *dev, int slot, bool active, int x, int y)
 	input_mt_report_slot_state(dev, MT_TOOL_FINGER, active);
 	if (active) {
 		input_report_abs(dev, ABS_MT_POSITION_X, x);
-		input_report_abs(dev, ABS_MT_POSITION_Y,
-				 YMAX_NOMINAL + YMIN_NOMINAL - y);
+		input_report_abs(dev, ABS_MT_POSITION_Y, y);
 	}
 }
 
@@ -584,7 +586,7 @@ static void synaptics_process_packet(struct psmouse *psmouse)
 
 	if (num_fingers > 0) {
 		input_report_abs(dev, ABS_X, hw.x);
-		input_report_abs(dev, ABS_Y, YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
+		input_report_abs(dev, ABS_Y, hw.y);
 	}
 	input_report_abs(dev, ABS_PRESSURE, hw.z);
 
-- 
1.7.3.1

Re: [PATCH 02/12] Input: synaptics - do not invert y if 0

[Henrik Rydberg]

Hi Daniel,

> Synaptics touchpads report increasing y from bottom to top.
> This is inverted from normal userspace "top of screen is 0" coordinates.
> Thus, the kernel driver reports inverted y coordinates to userspace.
> 
> In some cases, however, y = 0 is sent by the touchpad.
> In these cases, the kernel driver should not invert, and just report 0.
> 
> This patch also refactors the inversion into a macro, and moves it
> into packet processing instead of during position reporting.

The patch seems to invert the current output?

> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/input/mouse/synaptics.c |   16 +++++++++-------
>  1 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index e06e045..f6d0c04 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -44,6 +44,8 @@
>  #define YMIN_NOMINAL 1408
>  #define YMAX_NOMINAL 4448
>  
> +#define INVERT_Y(y) (((y) != 0) ? (YMAX_NOMINAL + YMIN_NOMINAL - (y)) : 0)
> +
>  
>  /*****************************************************************************
>   *	Stuff we need even when we do not want native Synaptics support
> @@ -409,9 +411,9 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
>  		hw->x = (((buf[3] & 0x10) << 8) |
>  			 ((buf[1] & 0x0f) << 8) |
>  			 buf[4]);
> -		hw->y = (((buf[3] & 0x20) << 7) |
> +		hw->y = INVERT_Y((((buf[3] & 0x20) << 7) |
>  			 ((buf[1] & 0xf0) << 4) |
> -			 buf[5]);
> +			 buf[5]));
>  
>  		hw->z = buf[2];
>  		hw->w = (((buf[0] & 0x30) >> 2) |
> @@ -421,7 +423,8 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
>  		if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) && hw->w == 2) {
>  			/* Gesture packet: (x, y, z) at half resolution */
>  			priv->mt.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
> -			priv->mt.y = (((buf[4] & 0xf0) << 4) | buf[2]) << 1;
> +			priv->mt.y = INVERT_Y((((buf[4] & 0xf0) << 4)
> +					      | buf[2]) << 1);
>  			priv->mt.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
>  			return 1;
>  		}
> @@ -473,7 +476,7 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
>  		}
>  	} else {
>  		hw->x = (((buf[1] & 0x1f) << 8) | buf[2]);
> -		hw->y = (((buf[4] & 0x1f) << 8) | buf[5]);
> +		hw->y = INVERT_Y(((buf[4] & 0x1f) << 8) | buf[5]);
>  
>  		hw->z = (((buf[0] & 0x30) << 2) | (buf[3] & 0x3F));
>  		hw->w = (((buf[1] & 0x80) >> 4) | ((buf[0] & 0x04) >> 1));
> @@ -491,8 +494,7 @@ static void set_slot(struct input_dev *dev, int slot, bool active, int x, int y)
>  	input_mt_report_slot_state(dev, MT_TOOL_FINGER, active);
>  	if (active) {
>  		input_report_abs(dev, ABS_MT_POSITION_X, x);
> -		input_report_abs(dev, ABS_MT_POSITION_Y,
> -				 YMAX_NOMINAL + YMIN_NOMINAL - y);
> +		input_report_abs(dev, ABS_MT_POSITION_Y, y);
>  	}
>  }
>  
> @@ -584,7 +586,7 @@ static void synaptics_process_packet(struct psmouse *psmouse)
>  
>  	if (num_fingers > 0) {
>  		input_report_abs(dev, ABS_X, hw.x);
> -		input_report_abs(dev, ABS_Y, YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
> +		input_report_abs(dev, ABS_Y, hw.y);
>  	}
>  	input_report_abs(dev, ABS_PRESSURE, hw.z);
>  
> -- 
> 1.7.3.1
> 

Thanks,
Henrik

Re: [PATCH 02/12] Input: synaptics - do not invert y if 0

[Daniel Kurtz]

Hi Henrik
Thanks for the review.

On Tue, Jul 5, 2011 at 5:08 AM, Henrik Rydberg <rydberg@euromail.se> wrote:
>
> Hi Daniel,
>
> > Synaptics touchpads report increasing y from bottom to top.
> > This is inverted from normal userspace "top of screen is 0" coordinates.
> > Thus, the kernel driver reports inverted y coordinates to userspace.
> >
> > In some cases, however, y = 0 is sent by the touchpad.
> > In these cases, the kernel driver should not invert, and just report 0.
> >
> > This patch also refactors the inversion into a macro, and moves it
> > into packet processing instead of during position reporting.
>
> The patch seems to invert the current output?

By 'current' do you mean referenced from the previous implementation?
Or referenced from the raw input.
It does indeed invert the raw input.
This is the same as the previous implementation did.
The difference is that it does not also invert the special 'y=0' into
an arbitrarily large value.
Is this your concern?
-Dan

>
> >
> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> > ---
> >  drivers/input/mouse/synaptics.c |   16 +++++++++-------
> >  1 files changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> > index e06e045..f6d0c04 100644
> > --- a/drivers/input/mouse/synaptics.c
> > +++ b/drivers/input/mouse/synaptics.c
> > @@ -44,6 +44,8 @@
> >  #define YMIN_NOMINAL 1408
> >  #define YMAX_NOMINAL 4448
> >
> > +#define INVERT_Y(y) (((y) != 0) ? (YMAX_NOMINAL + YMIN_NOMINAL - (y)) : 0)
> > +
> >
> >  /*****************************************************************************
> >   *   Stuff we need even when we do not want native Synaptics support
> > @@ -409,9 +411,9 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
> >               hw->x = (((buf[3] & 0x10) << 8) |
> >                        ((buf[1] & 0x0f) << 8) |
> >                        buf[4]);
> > -             hw->y = (((buf[3] & 0x20) << 7) |
> > +             hw->y = INVERT_Y((((buf[3] & 0x20) << 7) |
> >                        ((buf[1] & 0xf0) << 4) |
> > -                      buf[5]);
> > +                      buf[5]));
> >
> >               hw->z = buf[2];
> >               hw->w = (((buf[0] & 0x30) >> 2) |
> > @@ -421,7 +423,8 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
> >               if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) && hw->w == 2) {
> >                       /* Gesture packet: (x, y, z) at half resolution */
> >                       priv->mt.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
> > -                     priv->mt.y = (((buf[4] & 0xf0) << 4) | buf[2]) << 1;
> > +                     priv->mt.y = INVERT_Y((((buf[4] & 0xf0) << 4)
> > +                                           | buf[2]) << 1);
> >                       priv->mt.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
> >                       return 1;
> >               }
> > @@ -473,7 +476,7 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
> >               }
> >       } else {
> >               hw->x = (((buf[1] & 0x1f) << 8) | buf[2]);
> > -             hw->y = (((buf[4] & 0x1f) << 8) | buf[5]);
> > +             hw->y = INVERT_Y(((buf[4] & 0x1f) << 8) | buf[5]);
> >
> >               hw->z = (((buf[0] & 0x30) << 2) | (buf[3] & 0x3F));
> >               hw->w = (((buf[1] & 0x80) >> 4) | ((buf[0] & 0x04) >> 1));
> > @@ -491,8 +494,7 @@ static void set_slot(struct input_dev *dev, int slot, bool active, int x, int y)
> >       input_mt_report_slot_state(dev, MT_TOOL_FINGER, active);
> >       if (active) {
> >               input_report_abs(dev, ABS_MT_POSITION_X, x);
> > -             input_report_abs(dev, ABS_MT_POSITION_Y,
> > -                              YMAX_NOMINAL + YMIN_NOMINAL - y);
> > +             input_report_abs(dev, ABS_MT_POSITION_Y, y);
> >       }
> >  }
> >
> > @@ -584,7 +586,7 @@ static void synaptics_process_packet(struct psmouse *psmouse)
> >
> >       if (num_fingers > 0) {
> >               input_report_abs(dev, ABS_X, hw.x);
> > -             input_report_abs(dev, ABS_Y, YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
> > +             input_report_abs(dev, ABS_Y, hw.y);
> >       }
> >       input_report_abs(dev, ABS_PRESSURE, hw.z);
> >
> > --
> > 1.7.3.1
> >
>
> Thanks,
> Henrik

Re: [PATCH 02/12] Input: synaptics - do not invert y if 0

[Henrik Rydberg]

> > > In some cases, however, y = 0 is sent by the touchpad.
> > > In these cases, the kernel driver should not invert, and just report 0.
> > >
> > > This patch also refactors the inversion into a macro, and moves it
> > > into packet processing instead of during position reporting.
> >
> > The patch seems to invert the current output?
> 
> By 'current' do you mean referenced from the previous implementation?
> Or referenced from the raw input.
> It does indeed invert the raw input.
> This is the same as the previous implementation did.
> The difference is that it does not also invert the special 'y=0' into
> an arbitrarily large value.
> Is this your concern?

It would be clearer to just change the argument of the
input_report_abs() instances, would it not? An explanation why zero,
outside the value range, should be output also needs a rationale. It
would seem such packets should be masked somehow.

Thanks,
Henrik

Re: [PATCH 02/12] Input: synaptics - do not invert y if 0

[Chris Bagwell]

On Tue, Jul 5, 2011 at 1:07 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
>> > > In some cases, however, y = 0 is sent by the touchpad.
>> > > In these cases, the kernel driver should not invert, and just report 0.
>> > >
>> > > This patch also refactors the inversion into a macro, and moves it
>> > > into packet processing instead of during position reporting.
>> >
>> > The patch seems to invert the current output?
>>
>> By 'current' do you mean referenced from the previous implementation?
>> Or referenced from the raw input.
>> It does indeed invert the raw input.
>> This is the same as the previous implementation did.
>> The difference is that it does not also invert the special 'y=0' into
>> an arbitrarily large value.
>> Is this your concern?
>
> It would be clearer to just change the argument of the
> input_report_abs() instances, would it not? An explanation why zero,
> outside the value range, should be output also needs a rationale. It
> would seem such packets should be masked somehow.
>

When I saw this, thats what it looked like to me.  We already know
that hw.x == 1 is invalid and added to a list to filter out but maybe
hw.y == 0 is invalid as well and it wasn't as obvious because of the
inversion.

Sound the following pre-existing line be expanded to its filtered?

       if (hw.z > 0 && hw.x > 1) {

Chris

Re: [PATCH 02/12] Input: synaptics - do not invert y if 0

[Daniel Kurtz]

On Wed, Jul 6, 2011 at 2:07 AM, Henrik Rydberg <rydberg@euromail.se> wrote:
>> > > In some cases, however, y = 0 is sent by the touchpad.
>> > > In these cases, the kernel driver should not invert, and just report 0.
>> > >
>> > > This patch also refactors the inversion into a macro, and moves it
>> > > into packet processing instead of during position reporting.
>> >
>> > The patch seems to invert the current output?
>>
>> By 'current' do you mean referenced from the previous implementation?
>> Or referenced from the raw input.
>> It does indeed invert the raw input.
>> This is the same as the previous implementation did.
>> The difference is that it does not also invert the special 'y=0' into
>> an arbitrarily large value.
>> Is this your concern?
>
> It would be clearer to just change the argument of the
> input_report_abs() instances, would it not? An explanation why zero,
> outside the value range, should be output also needs a rationale. It
> would seem such packets should be masked somehow.

Actually, I think it is clearer to change all the y's in one function,
during parsing, rather than scattered about in various output
functions.

I'm sorry, I think I was misleading in my commit message.  This change
doesn't affect how packets get masked.  The y=0 is not handled
differently now that it is no longer inverted.  If it wasn't sent to
userspace before, it still won't be sent.

The older inverted y=0 was also, by definition, out of range, so
hopefully those packets are masked, and continue to be masked.  Not
inverting y=0 just makes it a bit easier to debug the driver, by
slightly more accurately parsing the incoming raw data.

>
> Thanks,
> Henrik
>

Re: [PATCH 02/12] Input: synaptics - do not invert y if 0

[Chase Douglas]

On 06/28/2011 10:07 PM, djkurtz@chromium.org wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
> 
> Synaptics touchpads report increasing y from bottom to top.
> This is inverted from normal userspace "top of screen is 0" coordinates.
> Thus, the kernel driver reports inverted y coordinates to userspace.
> 
> In some cases, however, y = 0 is sent by the touchpad.
> In these cases, the kernel driver should not invert, and just report 0.

Under what cases is y sent as 0, and why do we want to report it as 0 to
userspace?

> 
> This patch also refactors the inversion into a macro, and moves it
> into packet processing instead of during position reporting.

It's getting really hard to read the bit transformations with a macro.
I'd prefer an approach that splits the computation. Maybe:

hw->y = bit_manipulations;
hw->y = INVERT_Y(hw->y);

(could use a temporary variable too)

Or maybe define a static inline function that takes the hw state as an arg:

hw->y = bit_manipulations;
fix_y(hw);

I'd prefer this implementation if every scenario involved manipulating
the y value of the hw state struct.

Also, it would be great if you could add your explanation of why Y is
inverted as a comment above INVERT_Y (or whatever function/macro it
eventually becomes).

Thanks!

-- Chase

> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/input/mouse/synaptics.c |   16 +++++++++-------
>  1 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index e06e045..f6d0c04 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -44,6 +44,8 @@
>  #define YMIN_NOMINAL 1408
>  #define YMAX_NOMINAL 4448
>  
> +#define INVERT_Y(y) (((y) != 0) ? (YMAX_NOMINAL + YMIN_NOMINAL - (y)) : 0)
> +
>  
>  /*****************************************************************************
>   *	Stuff we need even when we do not want native Synaptics support
> @@ -409,9 +411,9 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
>  		hw->x = (((buf[3] & 0x10) << 8) |
>  			 ((buf[1] & 0x0f) << 8) |
>  			 buf[4]);
> -		hw->y = (((buf[3] & 0x20) << 7) |
> +		hw->y = INVERT_Y((((buf[3] & 0x20) << 7) |
>  			 ((buf[1] & 0xf0) << 4) |
> -			 buf[5]);
> +			 buf[5]));
>  
>  		hw->z = buf[2];
>  		hw->w = (((buf[0] & 0x30) >> 2) |
> @@ -421,7 +423,8 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
>  		if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) && hw->w == 2) {
>  			/* Gesture packet: (x, y, z) at half resolution */
>  			priv->mt.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
> -			priv->mt.y = (((buf[4] & 0xf0) << 4) | buf[2]) << 1;
> +			priv->mt.y = INVERT_Y((((buf[4] & 0xf0) << 4)
> +					      | buf[2]) << 1);
>  			priv->mt.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
>  			return 1;
>  		}
> @@ -473,7 +476,7 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
>  		}
>  	} else {
>  		hw->x = (((buf[1] & 0x1f) << 8) | buf[2]);
> -		hw->y = (((buf[4] & 0x1f) << 8) | buf[5]);
> +		hw->y = INVERT_Y(((buf[4] & 0x1f) << 8) | buf[5]);
>  
>  		hw->z = (((buf[0] & 0x30) << 2) | (buf[3] & 0x3F));
>  		hw->w = (((buf[1] & 0x80) >> 4) | ((buf[0] & 0x04) >> 1));
> @@ -491,8 +494,7 @@ static void set_slot(struct input_dev *dev, int slot, bool active, int x, int y)
>  	input_mt_report_slot_state(dev, MT_TOOL_FINGER, active);
>  	if (active) {
>  		input_report_abs(dev, ABS_MT_POSITION_X, x);
> -		input_report_abs(dev, ABS_MT_POSITION_Y,
> -				 YMAX_NOMINAL + YMIN_NOMINAL - y);
> +		input_report_abs(dev, ABS_MT_POSITION_Y, y);
>  	}
>  }
>  
> @@ -584,7 +586,7 @@ static void synaptics_process_packet(struct psmouse *psmouse)
>  
>  	if (num_fingers > 0) {
>  		input_report_abs(dev, ABS_X, hw.x);
> -		input_report_abs(dev, ABS_Y, YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
> +		input_report_abs(dev, ABS_Y, hw.y);
>  	}
>  	input_report_abs(dev, ABS_PRESSURE, hw.z);
>  

Re: [PATCH 02/12] Input: synaptics - do not invert y if 0

[Daniel Kurtz]

On Wed, Jul 6, 2011 at 1:42 AM, Chase Douglas
<chase.douglas@canonical.com> wrote:
>
> On 06/28/2011 10:07 PM, djkurtz@chromium.org wrote:
> > From: Daniel Kurtz <djkurtz@chromium.org>
> >
> > Synaptics touchpads report increasing y from bottom to top.
> > This is inverted from normal userspace "top of screen is 0" coordinates.
> > Thus, the kernel driver reports inverted y coordinates to userspace.
> >
> > In some cases, however, y = 0 is sent by the touchpad.
> > In these cases, the kernel driver should not invert, and just report 0.
>
> Under what cases is y sent as 0, and why do we want to report it as 0 to
> userspace?

I know of two such cases for the image sensor:
  (1) When all fingers, save the first finger are removed, an AGM
packet with (x=0, y=0, z=0) is sent.
  (2) When all fingers are removed, an SGM packet with (x=0, y=0, z=0) is sent.

After uploading this patch set, I played with the profile sensor
again, and I also saw it sometimes sends y=1 packets.  I don't know
what those are.

This is mostly useful for debugging the kernel driver.
When observing the raw position values, the special 0 (and 1?) cases
are more obvious when not inverted.
I think I am misleading in my commit message.  I don't believe these
are actually ever passed through to userspace.

>
> >
> > This patch also refactors the inversion into a macro, and moves it
> > into packet processing instead of during position reporting.
>
> It's getting really hard to read the bit transformations with a macro.
> I'd prefer an approach that splits the computation. Maybe:
>
> hw->y = bit_manipulations;
> hw->y = INVERT_Y(hw->y);
>
> (could use a temporary variable too)
>
> Or maybe define a static inline function that takes the hw state as an arg:
>
> hw->y = bit_manipulations;
> fix_y(hw);
>
> I'd prefer this implementation if every scenario involved manipulating
> the y value of the hw state struct.

Ok.  This option sounds good to me.  Both the sgm & agm cases
manipulate the same struct.

>
> Also, it would be great if you could add your explanation of why Y is
> inverted as a comment above INVERT_Y (or whatever function/macro it
> eventually becomes).

Ok.  I agree a comment would help.

Thanks for the review!

-Dan

>
> Thanks!
>
> -- Chase
>
> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> > ---
> >  drivers/input/mouse/synaptics.c |   16 +++++++++-------
> >  1 files changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> > index e06e045..f6d0c04 100644
> > --- a/drivers/input/mouse/synaptics.c
> > +++ b/drivers/input/mouse/synaptics.c
> > @@ -44,6 +44,8 @@
> >  #define YMIN_NOMINAL 1408
> >  #define YMAX_NOMINAL 4448
> >
> > +#define INVERT_Y(y) (((y) != 0) ? (YMAX_NOMINAL + YMIN_NOMINAL - (y)) : 0)
> > +
> >
> >  /*****************************************************************************
> >   *   Stuff we need even when we do not want native Synaptics support
> > @@ -409,9 +411,9 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
> >               hw->x = (((buf[3] & 0x10) << 8) |
> >                        ((buf[1] & 0x0f) << 8) |
> >                        buf[4]);
> > -             hw->y = (((buf[3] & 0x20) << 7) |
> > +             hw->y = INVERT_Y((((buf[3] & 0x20) << 7) |
> >                        ((buf[1] & 0xf0) << 4) |
> > -                      buf[5]);
> > +                      buf[5]));
> >
> >               hw->z = buf[2];
> >               hw->w = (((buf[0] & 0x30) >> 2) |
> > @@ -421,7 +423,8 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
> >               if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) && hw->w == 2) {
> >                       /* Gesture packet: (x, y, z) at half resolution */
> >                       priv->mt.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
> > -                     priv->mt.y = (((buf[4] & 0xf0) << 4) | buf[2]) << 1;
> > +                     priv->mt.y = INVERT_Y((((buf[4] & 0xf0) << 4)
> > +                                           | buf[2]) << 1);
> >                       priv->mt.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
> >                       return 1;
> >               }
> > @@ -473,7 +476,7 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
> >               }
> >       } else {
> >               hw->x = (((buf[1] & 0x1f) << 8) | buf[2]);
> > -             hw->y = (((buf[4] & 0x1f) << 8) | buf[5]);
> > +             hw->y = INVERT_Y(((buf[4] & 0x1f) << 8) | buf[5]);
> >
> >               hw->z = (((buf[0] & 0x30) << 2) | (buf[3] & 0x3F));
> >               hw->w = (((buf[1] & 0x80) >> 4) | ((buf[0] & 0x04) >> 1));
> > @@ -491,8 +494,7 @@ static void set_slot(struct input_dev *dev, int slot, bool active, int x, int y)
> >       input_mt_report_slot_state(dev, MT_TOOL_FINGER, active);
> >       if (active) {
> >               input_report_abs(dev, ABS_MT_POSITION_X, x);
> > -             input_report_abs(dev, ABS_MT_POSITION_Y,
> > -                              YMAX_NOMINAL + YMIN_NOMINAL - y);
> > +             input_report_abs(dev, ABS_MT_POSITION_Y, y);
> >       }
> >  }
> >
> > @@ -584,7 +586,7 @@ static void synaptics_process_packet(struct psmouse *psmouse)
> >
> >       if (num_fingers > 0) {
> >               input_report_abs(dev, ABS_X, hw.x);
> > -             input_report_abs(dev, ABS_Y, YMAX_NOMINAL + YMIN_NOMINAL - hw.y);
> > +             input_report_abs(dev, ABS_Y, hw.y);
> >       }
> >       input_report_abs(dev, ABS_PRESSURE, hw.z);
> >
>

Re: [PATCH 02/12] Input: synaptics - do not invert y if 0

[Chase Douglas]

On 07/05/2011 03:50 PM, Daniel Kurtz wrote:
> On Wed, Jul 6, 2011 at 1:42 AM, Chase Douglas
> <chase.douglas@canonical.com> wrote:
>>
>> On 06/28/2011 10:07 PM, djkurtz@chromium.org wrote:
>>> From: Daniel Kurtz <djkurtz@chromium.org>
>>>
>>> Synaptics touchpads report increasing y from bottom to top.
>>> This is inverted from normal userspace "top of screen is 0" coordinates.
>>> Thus, the kernel driver reports inverted y coordinates to userspace.
>>>
>>> In some cases, however, y = 0 is sent by the touchpad.
>>> In these cases, the kernel driver should not invert, and just report 0.
>>
>> Under what cases is y sent as 0, and why do we want to report it as 0 to
>> userspace?
> 
> I know of two such cases for the image sensor:
>   (1) When all fingers, save the first finger are removed, an AGM
> packet with (x=0, y=0, z=0) is sent.
>   (2) When all fingers are removed, an SGM packet with (x=0, y=0, z=0) is sent.
> 
> After uploading this patch set, I played with the profile sensor
> again, and I also saw it sometimes sends y=1 packets.  I don't know
> what those are.
> 
> This is mostly useful for debugging the kernel driver.
> When observing the raw position values, the special 0 (and 1?) cases
> are more obvious when not inverted.
> I think I am misleading in my commit message.  I don't believe these
> are actually ever passed through to userspace.

If they are set, then they are passed through evdev, unless I'm missing
something... Given the cases listed above, it seems like we should be
special casing these scenarios. Maybe that means dropping the event or
handling finger count transitions or something else.

I'm happy with the patch as a cleanup of y inversion, but I don't see
the utility of passing y = 0 through. With all the debugging techniques
we have (printk, systemtap, perf, ftrace, etc.), I don't think we need
this functionality, and it might lead a code reviewer to scratch their
head wondering what the point was :).

Thanks!

-- Chase

Re: [PATCH 02/12] Input: synaptics - do not invert y if 0

[Daniel Kurtz]

On Wed, Jul 6, 2011 at 7:06 AM, Chase Douglas
<chase.douglas@canonical.com> wrote:
> On 07/05/2011 03:50 PM, Daniel Kurtz wrote:
>> On Wed, Jul 6, 2011 at 1:42 AM, Chase Douglas
>> <chase.douglas@canonical.com> wrote:
>>>
>>> On 06/28/2011 10:07 PM, djkurtz@chromium.org wrote:
>>>> From: Daniel Kurtz <djkurtz@chromium.org>
>>>>
>>>> Synaptics touchpads report increasing y from bottom to top.
>>>> This is inverted from normal userspace "top of screen is 0" coordinates.
>>>> Thus, the kernel driver reports inverted y coordinates to userspace.
>>>>
>>>> In some cases, however, y = 0 is sent by the touchpad.
>>>> In these cases, the kernel driver should not invert, and just report 0.
>>>
>>> Under what cases is y sent as 0, and why do we want to report it as 0 to
>>> userspace?
>>
>> I know of two such cases for the image sensor:
>>   (1) When all fingers, save the first finger are removed, an AGM
>> packet with (x=0, y=0, z=0) is sent.
>>   (2) When all fingers are removed, an SGM packet with (x=0, y=0, z=0) is sent.
>>
>> After uploading this patch set, I played with the profile sensor
>> again, and I also saw it sometimes sends y=1 packets.  I don't know
>> what those are.
>>
>> This is mostly useful for debugging the kernel driver.
>> When observing the raw position values, the special 0 (and 1?) cases
>> are more obvious when not inverted.
>> I think I am misleading in my commit message.  I don't believe these
>> are actually ever passed through to userspace.
>
> If they are set, then they are passed through evdev, unless I'm missing
> something... Given the cases listed above, it seems like we should be
> special casing these scenarios. Maybe that means dropping the event or
> handling finger count transitions or something else.
>
> I'm happy with the patch as a cleanup of y inversion, but I don't see
> the utility of passing y = 0 through. With all the debugging techniques
> we have (printk, systemtap, perf, ftrace, etc.), I don't think we need
> this functionality, and it might lead a code reviewer to scratch their
> head wondering what the point was :).

I meant if you were to printk y, it is much more obvious to see y=0
than y=5321 or something like that.
I don't think this reaches userspace because y=0 packets are usually
filtered out since z and x are also 0.

I guess this is the point, actually, that wasn't clear to me in
Henrik's astute observation:
We should do the conversion when reporting to userspace.  Thus, only
do the conversion for packets that aren't already masked!

I will refactor to an inline function, but put the conversion back in
the input_report_abs() calls.

-Dan

>
> Thanks!
>
> -- Chase
>

Re: [PATCH 02/12] Input: synaptics - do not invert y if 0

[Dmitry Torokhov]

On Wed, Jul 06, 2011 at 07:15:07AM +0800, Daniel Kurtz wrote:
> On Wed, Jul 6, 2011 at 7:06 AM, Chase Douglas
> <chase.douglas@canonical.com> wrote:
> > On 07/05/2011 03:50 PM, Daniel Kurtz wrote:
> >> On Wed, Jul 6, 2011 at 1:42 AM, Chase Douglas
> >> <chase.douglas@canonical.com> wrote:
> >>>
> >>> On 06/28/2011 10:07 PM, djkurtz@chromium.org wrote:
> >>>> From: Daniel Kurtz <djkurtz@chromium.org>
> >>>>
> >>>> Synaptics touchpads report increasing y from bottom to top.
> >>>> This is inverted from normal userspace "top of screen is 0" coordinates.
> >>>> Thus, the kernel driver reports inverted y coordinates to userspace.
> >>>>
> >>>> In some cases, however, y = 0 is sent by the touchpad.
> >>>> In these cases, the kernel driver should not invert, and just report 0.
> >>>
> >>> Under what cases is y sent as 0, and why do we want to report it as 0 to
> >>> userspace?
> >>
> >> I know of two such cases for the image sensor:
> >>   (1) When all fingers, save the first finger are removed, an AGM
> >> packet with (x=0, y=0, z=0) is sent.
> >>   (2) When all fingers are removed, an SGM packet with (x=0, y=0, z=0) is sent.
> >>
> >> After uploading this patch set, I played with the profile sensor
> >> again, and I also saw it sometimes sends y=1 packets.  I don't know
> >> what those are.
> >>
> >> This is mostly useful for debugging the kernel driver.
> >> When observing the raw position values, the special 0 (and 1?) cases
> >> are more obvious when not inverted.
> >> I think I am misleading in my commit message.  I don't believe these
> >> are actually ever passed through to userspace.
> >
> > If they are set, then they are passed through evdev, unless I'm missing
> > something... Given the cases listed above, it seems like we should be
> > special casing these scenarios. Maybe that means dropping the event or
> > handling finger count transitions or something else.
> >
> > I'm happy with the patch as a cleanup of y inversion, but I don't see
> > the utility of passing y = 0 through. With all the debugging techniques
> > we have (printk, systemtap, perf, ftrace, etc.), I don't think we need
> > this functionality, and it might lead a code reviewer to scratch their
> > head wondering what the point was :).
> 
> I meant if you were to printk y, it is much more obvious to see y=0
> than y=5321 or something like that.
> I don't think this reaches userspace because y=0 packets are usually
> filtered out since z and x are also 0.
> 
> I guess this is the point, actually, that wasn't clear to me in
> Henrik's astute observation:
> We should do the conversion when reporting to userspace.  Thus, only
> do the conversion for packets that aren't already masked!
> 
> I will refactor to an inline function, but put the conversion back in
> the input_report_abs() calls.

Better yet, drop this patch completely. If there is no (or low) pressure
coordinate values are not interesting and shoudl be ignored by both
humans and userspase consumers.

Thanks.

-- 
Dmitry

[PATCH 03/12] Input: synaptics - fix minimum reported ABS_TOOL_WIDTH

[djkurtz]

From: Daniel Kurtz <djkurtz@chromium.org>

Synaptics touchpads report a 'w' value in each data report.
For touchpads that support palm detection, when there is a single finger
on the pad, the 'w' value reports its width in the range 4 to 15.
Thus, the minimum valid width is 4.

Note: Other values of 'w' are used to report special conditions:
 w=0: 2 fingers are on the pad
 w=1: 3 or more fingers are on the pad
 w=2: the packet contains "Advanced Gesture Mode" data.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/mouse/synaptics.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index f6d0c04..a4b7801 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -706,7 +706,7 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
 	}
 
 	if (SYN_CAP_PALMDETECT(priv->capabilities))
-		input_set_abs_params(dev, ABS_TOOL_WIDTH, 0, 15, 0, 0);
+		input_set_abs_params(dev, ABS_TOOL_WIDTH, 4, 15, 0, 0);
 
 	__set_bit(EV_KEY, dev->evbit);
 	__set_bit(BTN_TOUCH, dev->keybit);
-- 
1.7.3.1

Re: [PATCH 03/12] Input: synaptics - fix minimum reported ABS_TOOL_WIDTH

[Chris Bagwell]

On Wed, Jun 29, 2011 at 12:07 AM,  <djkurtz@chromium.org> wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
>
> Synaptics touchpads report a 'w' value in each data report.
> For touchpads that support palm detection, when there is a single finger
> on the pad, the 'w' value reports its width in the range 4 to 15.
> Thus, the minimum valid width is 4.

FYI: I had debated on this as well.  When driver was first modified to
report min/max width, the min width of zero was chosen because when
not touching the pad a value of zero is forced by driver to user.  So
the range is 0, 4-15.

>
> Note: Other values of 'w' are used to report special conditions:
>  w=0: 2 fingers are on the pad
>  w=1: 3 or more fingers are on the pad
>  w=2: the packet contains "Advanced Gesture Mode" data.
>
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/input/mouse/synaptics.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index f6d0c04..a4b7801 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -706,7 +706,7 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
>        }
>
>        if (SYN_CAP_PALMDETECT(priv->capabilities))
> -               input_set_abs_params(dev, ABS_TOOL_WIDTH, 0, 15, 0, 0);
> +               input_set_abs_params(dev, ABS_TOOL_WIDTH, 4, 15, 0, 0);
>
>        __set_bit(EV_KEY, dev->evbit);
>        __set_bit(BTN_TOUCH, dev->keybit);
> --
> 1.7.3.1
>
> --
> 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 03/12] Input: synaptics - fix minimum reported ABS_TOOL_WIDTH

[Daniel Kurtz]

Hi Chris,

Thanks for taking a look!

On Wed, Jun 29, 2011 at 9:28 PM, Chris Bagwell <chris@cnpbagwell.com> wrote:
> On Wed, Jun 29, 2011 at 12:07 AM,  <djkurtz@chromium.org> wrote:
>> From: Daniel Kurtz <djkurtz@chromium.org>
>>
>> Synaptics touchpads report a 'w' value in each data report.
>> For touchpads that support palm detection, when there is a single finger
>> on the pad, the 'w' value reports its width in the range 4 to 15.
>> Thus, the minimum valid width is 4.
>
> FYI: I had debated on this as well.  When driver was first modified to
> report min/max width, the min width of zero was chosen because when
> not touching the pad a value of zero is forced by driver to user.  So
> the range is 0, 4-15.

Yup, good catch.  I now see that this is true for the legacy single-touch case.
For consistency with the existing implementation, I'm ok to drop this patch.

I think we can still keep the range 4-15 for the ABS_MT_TOUCH_MAJOR
axis, though, right?
A pure-mt userspace app can get the correct width range, and deduce 0
fingers from the fact that all MT-B slots have tracking_id == -1.

-Dan

>>
>> Note: Other values of 'w' are used to report special conditions:
>>  w=0: 2 fingers are on the pad
>>  w=1: 3 or more fingers are on the pad
>>  w=2: the packet contains "Advanced Gesture Mode" data.
>>
>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>> ---
>>  drivers/input/mouse/synaptics.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>> index f6d0c04..a4b7801 100644
>> --- a/drivers/input/mouse/synaptics.c
>> +++ b/drivers/input/mouse/synaptics.c
>> @@ -706,7 +706,7 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
>>        }
>>
>>        if (SYN_CAP_PALMDETECT(priv->capabilities))
>> -               input_set_abs_params(dev, ABS_TOOL_WIDTH, 0, 15, 0, 0);
>> +               input_set_abs_params(dev, ABS_TOOL_WIDTH, 4, 15, 0, 0);
>>
>>        __set_bit(EV_KEY, dev->evbit);
>>        __set_bit(BTN_TOUCH, dev->keybit);
>> --
>> 1.7.3.1
>>
>> --
>> 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
>>
> --
> 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 03/12] Input: synaptics - fix minimum reported ABS_TOOL_WIDTH

[Chris Bagwell]

On Wed, Jun 29, 2011 at 11:48 AM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> Hi Chris,
>
> Thanks for taking a look!
>
> On Wed, Jun 29, 2011 at 9:28 PM, Chris Bagwell <chris@cnpbagwell.com> wrote:
>> On Wed, Jun 29, 2011 at 12:07 AM,  <djkurtz@chromium.org> wrote:
>>> From: Daniel Kurtz <djkurtz@chromium.org>
>>>
>>> Synaptics touchpads report a 'w' value in each data report.
>>> For touchpads that support palm detection, when there is a single finger
>>> on the pad, the 'w' value reports its width in the range 4 to 15.
>>> Thus, the minimum valid width is 4.
>>
>> FYI: I had debated on this as well.  When driver was first modified to
>> report min/max width, the min width of zero was chosen because when
>> not touching the pad a value of zero is forced by driver to user.  So
>> the range is 0, 4-15.
>
> Yup, good catch.  I now see that this is true for the legacy single-touch case.
> For consistency with the existing implementation, I'm ok to drop this patch.
>
> I think we can still keep the range 4-15 for the ABS_MT_TOUCH_MAJOR
> axis, though, right?
> A pure-mt userspace app can get the correct width range, and deduce 0
> fingers from the fact that all MT-B slots have tracking_id == -1.

Yeah, at least I'd consider it an app bug.

For that matter, it should be true of ABS_TOOL_WIDTH as well (we could
change to always send minimum 4 as apart of this patch).

Since we are on it, maybe a year ago I tested a version of this patch
and notice xf86-input-synaptics defaulted to non-useful value for
EmulateTwoFingerMinW because of the scale change.  The
xf86-input-synaptics logic in question has since be modified/reverted
so I think thats not an issue anymore.

I found no issues with apps see'ing ABS_TOOL_WIDTH >= 4 always.

Chris
>
> -Dan
>
>>>
>>> Note: Other values of 'w' are used to report special conditions:
>>>  w=0: 2 fingers are on the pad
>>>  w=1: 3 or more fingers are on the pad
>>>  w=2: the packet contains "Advanced Gesture Mode" data.
>>>
>>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>>> ---
>>>  drivers/input/mouse/synaptics.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>>> index f6d0c04..a4b7801 100644
>>> --- a/drivers/input/mouse/synaptics.c
>>> +++ b/drivers/input/mouse/synaptics.c
>>> @@ -706,7 +706,7 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
>>>        }
>>>
>>>        if (SYN_CAP_PALMDETECT(priv->capabilities))
>>> -               input_set_abs_params(dev, ABS_TOOL_WIDTH, 0, 15, 0, 0);
>>> +               input_set_abs_params(dev, ABS_TOOL_WIDTH, 4, 15, 0, 0);
>>>
>>>        __set_bit(EV_KEY, dev->evbit);
>>>        __set_bit(BTN_TOUCH, dev->keybit);
>>> --
>>> 1.7.3.1
>>>
>>> --
>>> 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
>>>
>> --
>> 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 03/12] Input: synaptics - fix minimum reported ABS_TOOL_WIDTH

[Henrik Rydberg]

> > Yup, good catch.  I now see that this is true for the legacy single-touch case.
> > For consistency with the existing implementation, I'm ok to drop this patch.

Thank you.

Henrik

Re: [PATCH 03/12] Input: synaptics - fix minimum reported ABS_TOOL_WIDTH

[Jeffrey Brown]

On Tue, Jun 28, 2011 at 10:07 PM,  <djkurtz@chromium.org> wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
>
> Synaptics touchpads report a 'w' value in each data report.
> For touchpads that support palm detection, when there is a single finger
> on the pad, the 'w' value reports its width in the range 4 to 15.
> Thus, the minimum valid width is 4.
>
> Note: Other values of 'w' are used to report special conditions:
>  w=0: 2 fingers are on the pad
>  w=1: 3 or more fingers are on the pad
>  w=2: the packet contains "Advanced Gesture Mode" data.

Ideally we should scale this to a physically meaningful quantity
instead, such as surface units.  That would require knowing the
distance between each capacitive touch channel and then scaling W - 4
by that amount before reporting it.

These values are all kind of mysterious to user-space anyways unless
the physical nature of the device is known by other means (such as
configuration files).

Jeff.

[PATCH 04/12] Input: synaptics - set resolution for MT_POSITION_X/Y axes

[djkurtz]

From: Daniel Kurtz <djkurtz@chromium.org>

Set resolution for MT_POSITION_X and MT_POSITION_Y to match ABS_X and
ABS_Y, respectively.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/mouse/synaptics.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index a4b7801..1ce47b7 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -703,6 +703,9 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
 				     priv->x_max ?: XMAX_NOMINAL, 0, 0);
 		input_set_abs_params(dev, ABS_MT_POSITION_Y, YMIN_NOMINAL,
 				     priv->y_max ?: YMAX_NOMINAL, 0, 0);
+
+		input_abs_set_res(dev, ABS_MT_POSITION_X, priv->x_res);
+		input_abs_set_res(dev, ABS_MT_POSITION_Y, priv->y_res);
 	}
 
 	if (SYN_CAP_PALMDETECT(priv->capabilities))
-- 
1.7.3.1

Re: [PATCH 04/12] Input: synaptics - set resolution for MT_POSITION_X/Y axes

[Chase Douglas]

On 06/28/2011 10:07 PM, djkurtz@chromium.org wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
> 
> Set resolution for MT_POSITION_X and MT_POSITION_Y to match ABS_X and
> ABS_Y, respectively.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

Acked-by: Chase Douglas <chase.douglas@canonical.com>

> ---
>  drivers/input/mouse/synaptics.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index a4b7801..1ce47b7 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -703,6 +703,9 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
>  				     priv->x_max ?: XMAX_NOMINAL, 0, 0);
>  		input_set_abs_params(dev, ABS_MT_POSITION_Y, YMIN_NOMINAL,
>  				     priv->y_max ?: YMAX_NOMINAL, 0, 0);
> +
> +		input_abs_set_res(dev, ABS_MT_POSITION_X, priv->x_res);
> +		input_abs_set_res(dev, ABS_MT_POSITION_Y, priv->y_res);
>  	}
>  
>  	if (SYN_CAP_PALMDETECT(priv->capabilities))

Re: [PATCH 04/12] Input: synaptics - set resolution for MT_POSITION_X/Y axes

[Dmitry Torokhov]

On Tue, Jul 05, 2011 at 10:44:52AM -0700, Chase Douglas wrote:
> On 06/28/2011 10:07 PM, djkurtz@chromium.org wrote:
> > From: Daniel Kurtz <djkurtz@chromium.org>
> > 
> > Set resolution for MT_POSITION_X and MT_POSITION_Y to match ABS_X and
> > ABS_Y, respectively.
> > 
> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> 
> Acked-by: Chase Douglas <chase.douglas@canonical.com>

Applied.

> 
> > ---
> >  drivers/input/mouse/synaptics.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> > index a4b7801..1ce47b7 100644
> > --- a/drivers/input/mouse/synaptics.c
> > +++ b/drivers/input/mouse/synaptics.c
> > @@ -703,6 +703,9 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
> >  				     priv->x_max ?: XMAX_NOMINAL, 0, 0);
> >  		input_set_abs_params(dev, ABS_MT_POSITION_Y, YMIN_NOMINAL,
> >  				     priv->y_max ?: YMAX_NOMINAL, 0, 0);
> > +
> > +		input_abs_set_res(dev, ABS_MT_POSITION_X, priv->x_res);
> > +		input_abs_set_res(dev, ABS_MT_POSITION_Y, priv->y_res);
> >  	}
> >  
> >  	if (SYN_CAP_PALMDETECT(priv->capabilities))
> 

-- 
Dmitry

[PATCH 05/12] Input: synaptics - process button bits in AGM packets

[djkurtz]

From: Daniel Kurtz <djkurtz@chromium.org>

AGM packets contain valid button bits, too.
This patch refactors packet processing to parse button bits in AGM packets.
However, they aren't actually used or reported.

The point is to more completely process AGM packets,
and prepare for future patches that may actually use AGM packet button bits.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/mouse/synaptics.c |   35 ++++++++++++++++++-----------------
 1 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 1ce47b7..74b1222 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -408,27 +408,10 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
 	memset(hw, 0, sizeof(struct synaptics_hw_state));
 
 	if (SYN_MODEL_NEWABS(priv->model_id)) {
-		hw->x = (((buf[3] & 0x10) << 8) |
-			 ((buf[1] & 0x0f) << 8) |
-			 buf[4]);
-		hw->y = INVERT_Y((((buf[3] & 0x20) << 7) |
-			 ((buf[1] & 0xf0) << 4) |
-			 buf[5]));
-
-		hw->z = buf[2];
 		hw->w = (((buf[0] & 0x30) >> 2) |
 			 ((buf[0] & 0x04) >> 1) |
 			 ((buf[3] & 0x04) >> 2));
 
-		if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) && hw->w == 2) {
-			/* Gesture packet: (x, y, z) at half resolution */
-			priv->mt.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
-			priv->mt.y = INVERT_Y((((buf[4] & 0xf0) << 4)
-					      | buf[2]) << 1);
-			priv->mt.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
-			return 1;
-		}
-
 		hw->left  = (buf[0] & 0x01) ? 1 : 0;
 		hw->right = (buf[0] & 0x02) ? 1 : 0;
 
@@ -451,6 +434,24 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
 			hw->down = ((buf[0] ^ buf[3]) & 0x02) ? 1 : 0;
 		}
 
+		if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) && hw->w == 2) {
+			/* Gesture packet: (x, y, z) at half resolution */
+			priv->mt.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
+			priv->mt.y = INVERT_Y((((buf[4] & 0xf0) << 4)
+					      | buf[2]) << 1);
+			priv->mt.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
+			return 1;
+		} else {
+			hw->x = (((buf[3] & 0x10) << 8) |
+				 ((buf[1] & 0x0f) << 8) |
+				 buf[4]);
+			hw->y = INVERT_Y((((buf[3] & 0x20) << 7) |
+					 ((buf[1] & 0xf0) << 4) |
+					 buf[5]));
+
+			hw->z = buf[2];
+		}
+
 		if (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) &&
 		    ((buf[0] ^ buf[3]) & 0x02)) {
 			switch (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) & ~0x01) {
-- 
1.7.3.1

Re: [PATCH 05/12] Input: synaptics - process button bits in AGM packets

[Henrik Rydberg]

Hi Daniel,

> AGM packets contain valid button bits, too.
> This patch refactors packet processing to parse button bits in AGM packets.
> However, they aren't actually used or reported.
> 
> The point is to more completely process AGM packets,
> and prepare for future patches that may actually use AGM packet button bits.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/input/mouse/synaptics.c |   35 ++++++++++++++++++-----------------
>  1 files changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 1ce47b7..74b1222 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -408,27 +408,10 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
>  	memset(hw, 0, sizeof(struct synaptics_hw_state));
>  
>  	if (SYN_MODEL_NEWABS(priv->model_id)) {
> -		hw->x = (((buf[3] & 0x10) << 8) |
> -			 ((buf[1] & 0x0f) << 8) |
> -			 buf[4]);
> -		hw->y = INVERT_Y((((buf[3] & 0x20) << 7) |
> -			 ((buf[1] & 0xf0) << 4) |
> -			 buf[5]));
> -
> -		hw->z = buf[2];

Any particular reason to move these and leave them unassigned for clickpads?

>  		hw->w = (((buf[0] & 0x30) >> 2) |
>  			 ((buf[0] & 0x04) >> 1) |
>  			 ((buf[3] & 0x04) >> 2));
>  
> -		if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) && hw->w == 2) {
> -			/* Gesture packet: (x, y, z) at half resolution */
> -			priv->mt.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
> -			priv->mt.y = INVERT_Y((((buf[4] & 0xf0) << 4)
> -					      | buf[2]) << 1);
> -			priv->mt.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
> -			return 1;
> -		}
> -
>  		hw->left  = (buf[0] & 0x01) ? 1 : 0;
>  		hw->right = (buf[0] & 0x02) ? 1 : 0;
>  
> @@ -451,6 +434,24 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
>  			hw->down = ((buf[0] ^ buf[3]) & 0x02) ? 1 : 0;
>  		}
>  
> +		if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) && hw->w == 2) {
> +			/* Gesture packet: (x, y, z) at half resolution */
> +			priv->mt.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
> +			priv->mt.y = INVERT_Y((((buf[4] & 0xf0) << 4)
> +					      | buf[2]) << 1);
> +			priv->mt.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
> +			return 1;
> +		} else {
> +			hw->x = (((buf[3] & 0x10) << 8) |
> +				 ((buf[1] & 0x0f) << 8) |
> +				 buf[4]);
> +			hw->y = INVERT_Y((((buf[3] & 0x20) << 7) |
> +					 ((buf[1] & 0xf0) << 4) |
> +					 buf[5]));
> +
> +			hw->z = buf[2];
> +		}
> +
>  		if (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) &&
>  		    ((buf[0] ^ buf[3]) & 0x02)) {
>  			switch (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) & ~0x01) {
> -- 
> 1.7.3.1
> 

Thanks,
Henrik

Re: [PATCH 05/12] Input: synaptics - process button bits in AGM packets

[Daniel Kurtz]

Hi Henrik,

On Tue, Jul 5, 2011 at 5:24 AM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Daniel,
>
>> AGM packets contain valid button bits, too.
>> This patch refactors packet processing to parse button bits in AGM packets.
>> However, they aren't actually used or reported.
>>
>> The point is to more completely process AGM packets,
>> and prepare for future patches that may actually use AGM packet button bits.
>>
>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>> ---
>>  drivers/input/mouse/synaptics.c |   35 ++++++++++++++++++-----------------
>>  1 files changed, 18 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>> index 1ce47b7..74b1222 100644
>> --- a/drivers/input/mouse/synaptics.c
>> +++ b/drivers/input/mouse/synaptics.c
>> @@ -408,27 +408,10 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
>>       memset(hw, 0, sizeof(struct synaptics_hw_state));
>>
>>       if (SYN_MODEL_NEWABS(priv->model_id)) {
>> -             hw->x = (((buf[3] & 0x10) << 8) |
>> -                      ((buf[1] & 0x0f) << 8) |
>> -                      buf[4]);
>> -             hw->y = INVERT_Y((((buf[3] & 0x20) << 7) |
>> -                      ((buf[1] & 0xf0) << 4) |
>> -                      buf[5]));
>> -
>> -             hw->z = buf[2];
>
> Any particular reason to move these and leave them unassigned for clickpads?

Yes.  The current implementation incorrectly parses the x, y, z of AGM
packets and assigns junk values to the corresponding fields of the
temporary hw struct.  Luckily, this struct is then just discarded upon
return (synaptics_parse_hw_state returns 1 causing
synaptics_process_packet() to exit immediately).

Instead, this patch parses the w value first to determine the packet
type, and then use this packet type information to parse the remaining
position and pressure fields correctly...

Notice that the "else" clause is taken for SGM packets (w != 2), even
for clickpads.

-Dan

>
>>               hw->w = (((buf[0] & 0x30) >> 2) |
>>                        ((buf[0] & 0x04) >> 1) |
>>                        ((buf[3] & 0x04) >> 2));
>>
>> -             if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) && hw->w == 2) {
>> -                     /* Gesture packet: (x, y, z) at half resolution */
>> -                     priv->mt.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
>> -                     priv->mt.y = INVERT_Y((((buf[4] & 0xf0) << 4)
>> -                                           | buf[2]) << 1);
>> -                     priv->mt.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
>> -                     return 1;
>> -             }
>> -
>>               hw->left  = (buf[0] & 0x01) ? 1 : 0;
>>               hw->right = (buf[0] & 0x02) ? 1 : 0;
>>
>> @@ -451,6 +434,24 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
>>                       hw->down = ((buf[0] ^ buf[3]) & 0x02) ? 1 : 0;
>>               }
>>
>> +             if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) && hw->w == 2) {
>> +                     /* Gesture packet: (x, y, z) at half resolution */
>> +                     priv->mt.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
>> +                     priv->mt.y = INVERT_Y((((buf[4] & 0xf0) << 4)
>> +                                           | buf[2]) << 1);
>> +                     priv->mt.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
>> +                     return 1;
>> +             } else {
>> +                     hw->x = (((buf[3] & 0x10) << 8) |
>> +                              ((buf[1] & 0x0f) << 8) |
>> +                              buf[4]);
>> +                     hw->y = INVERT_Y((((buf[3] & 0x20) << 7) |
>> +                                      ((buf[1] & 0xf0) << 4) |
>> +                                      buf[5]));
>> +
>> +                     hw->z = buf[2];
>> +             }
>> +
>>               if (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) &&
>>                   ((buf[0] ^ buf[3]) & 0x02)) {
>>                       switch (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) & ~0x01) {
>> --
>> 1.7.3.1
>>
>
> Thanks,
> Henrik
>

Re: [PATCH 05/12] Input: synaptics - process button bits in AGM packets

[Henrik Rydberg]

> > Any particular reason to move these and leave them unassigned for clickpads?
> 
> Yes.  The current implementation incorrectly parses the x, y, z of AGM
> packets and assigns junk values to the corresponding fields of the
> temporary hw struct.  Luckily, this struct is then just discarded upon
> return (synaptics_parse_hw_state returns 1 causing
> synaptics_process_packet() to exit immediately).
>
> Instead, this patch parses the w value first to determine the packet
> type, and then use this packet type information to parse the remaining
> position and pressure fields correctly...
> 
> Notice that the "else" clause is taken for SGM packets (w != 2), even
> for clickpads.

Functionally, there is no difference between assigning new junk or
reusing old junk, hence that part of the patch is not strictly needed.

Thanks,
Henrik

Re: [PATCH 05/12] Input: synaptics - process button bits in AGM packets

[Chase Douglas]

On 07/05/2011 11:18 AM, Henrik Rydberg wrote:
>>> Any particular reason to move these and leave them unassigned for clickpads?
>>
>> Yes.  The current implementation incorrectly parses the x, y, z of AGM
>> packets and assigns junk values to the corresponding fields of the
>> temporary hw struct.  Luckily, this struct is then just discarded upon
>> return (synaptics_parse_hw_state returns 1 causing
>> synaptics_process_packet() to exit immediately).
>>
>> Instead, this patch parses the w value first to determine the packet
>> type, and then use this packet type information to parse the remaining
>> position and pressure fields correctly...
>>
>> Notice that the "else" clause is taken for SGM packets (w != 2), even
>> for clickpads.
> 
> Functionally, there is no difference between assigning new junk or
> reusing old junk, hence that part of the patch is not strictly needed.

True, it's not necessary, but I do like the new flow better. I was
reviewing the patch myself when I started wondering why we ever assigned
junk that was incorrect. I think this will help clean up the code and
make it more readable.

-- Chase

Re: [PATCH 05/12] Input: synaptics - process button bits in AGM packets

[Chase Douglas]

On 06/28/2011 10:07 PM, djkurtz@chromium.org wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
> 
> AGM packets contain valid button bits, too.
> This patch refactors packet processing to parse button bits in AGM packets.
> However, they aren't actually used or reported.
> 
> The point is to more completely process AGM packets,
> and prepare for future patches that may actually use AGM packet button bits.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

I can't see anything wrong with this :).

Acked-by: Chase Douglas <chase.douglas@canonical.com>

> ---
>  drivers/input/mouse/synaptics.c |   35 ++++++++++++++++++-----------------
>  1 files changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 1ce47b7..74b1222 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -408,27 +408,10 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
>  	memset(hw, 0, sizeof(struct synaptics_hw_state));
>  
>  	if (SYN_MODEL_NEWABS(priv->model_id)) {
> -		hw->x = (((buf[3] & 0x10) << 8) |
> -			 ((buf[1] & 0x0f) << 8) |
> -			 buf[4]);
> -		hw->y = INVERT_Y((((buf[3] & 0x20) << 7) |
> -			 ((buf[1] & 0xf0) << 4) |
> -			 buf[5]));
> -
> -		hw->z = buf[2];
>  		hw->w = (((buf[0] & 0x30) >> 2) |
>  			 ((buf[0] & 0x04) >> 1) |
>  			 ((buf[3] & 0x04) >> 2));
>  
> -		if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) && hw->w == 2) {
> -			/* Gesture packet: (x, y, z) at half resolution */
> -			priv->mt.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
> -			priv->mt.y = INVERT_Y((((buf[4] & 0xf0) << 4)
> -					      | buf[2]) << 1);
> -			priv->mt.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
> -			return 1;
> -		}
> -
>  		hw->left  = (buf[0] & 0x01) ? 1 : 0;
>  		hw->right = (buf[0] & 0x02) ? 1 : 0;
>  
> @@ -451,6 +434,24 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
>  			hw->down = ((buf[0] ^ buf[3]) & 0x02) ? 1 : 0;
>  		}
>  
> +		if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) && hw->w == 2) {
> +			/* Gesture packet: (x, y, z) at half resolution */
> +			priv->mt.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
> +			priv->mt.y = INVERT_Y((((buf[4] & 0xf0) << 4)
> +					      | buf[2]) << 1);
> +			priv->mt.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
> +			return 1;
> +		} else {
> +			hw->x = (((buf[3] & 0x10) << 8) |
> +				 ((buf[1] & 0x0f) << 8) |
> +				 buf[4]);
> +			hw->y = INVERT_Y((((buf[3] & 0x20) << 7) |
> +					 ((buf[1] & 0xf0) << 4) |
> +					 buf[5]));
> +
> +			hw->z = buf[2];
> +		}
> +
>  		if (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) &&
>  		    ((buf[0] ^ buf[3]) & 0x02)) {
>  			switch (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) & ~0x01) {

Re: [PATCH 05/12] Input: synaptics - process button bits in AGM packets

[Dmitry Torokhov]

On Tue, Jul 05, 2011 at 10:47:19AM -0700, Chase Douglas wrote:
> On 06/28/2011 10:07 PM, djkurtz@chromium.org wrote:
> > From: Daniel Kurtz <djkurtz@chromium.org>
> > 
> > AGM packets contain valid button bits, too.
> > This patch refactors packet processing to parse button bits in AGM packets.
> > However, they aren't actually used or reported.
> > 
> > The point is to more completely process AGM packets,
> > and prepare for future patches that may actually use AGM packet button bits.
> > 
> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> 
> I can't see anything wrong with this :).
> 
> Acked-by: Chase Douglas <chase.douglas@canonical.com>
> 

Applied with minor edits.

> > ---
> >  drivers/input/mouse/synaptics.c |   35 ++++++++++++++++++-----------------
> >  1 files changed, 18 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> > index 1ce47b7..74b1222 100644
> > --- a/drivers/input/mouse/synaptics.c
> > +++ b/drivers/input/mouse/synaptics.c
> > @@ -408,27 +408,10 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
> >  	memset(hw, 0, sizeof(struct synaptics_hw_state));
> >  
> >  	if (SYN_MODEL_NEWABS(priv->model_id)) {
> > -		hw->x = (((buf[3] & 0x10) << 8) |
> > -			 ((buf[1] & 0x0f) << 8) |
> > -			 buf[4]);
> > -		hw->y = INVERT_Y((((buf[3] & 0x20) << 7) |
> > -			 ((buf[1] & 0xf0) << 4) |
> > -			 buf[5]));
> > -
> > -		hw->z = buf[2];
> >  		hw->w = (((buf[0] & 0x30) >> 2) |
> >  			 ((buf[0] & 0x04) >> 1) |
> >  			 ((buf[3] & 0x04) >> 2));
> >  
> > -		if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) && hw->w == 2) {
> > -			/* Gesture packet: (x, y, z) at half resolution */
> > -			priv->mt.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
> > -			priv->mt.y = INVERT_Y((((buf[4] & 0xf0) << 4)
> > -					      | buf[2]) << 1);
> > -			priv->mt.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
> > -			return 1;
> > -		}
> > -
> >  		hw->left  = (buf[0] & 0x01) ? 1 : 0;
> >  		hw->right = (buf[0] & 0x02) ? 1 : 0;
> >  
> > @@ -451,6 +434,24 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
> >  			hw->down = ((buf[0] ^ buf[3]) & 0x02) ? 1 : 0;
> >  		}
> >  
> > +		if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) && hw->w == 2) {
> > +			/* Gesture packet: (x, y, z) at half resolution */
> > +			priv->mt.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
> > +			priv->mt.y = INVERT_Y((((buf[4] & 0xf0) << 4)
> > +					      | buf[2]) << 1);
> > +			priv->mt.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
> > +			return 1;
> > +		} else {
> > +			hw->x = (((buf[3] & 0x10) << 8) |
> > +				 ((buf[1] & 0x0f) << 8) |
> > +				 buf[4]);
> > +			hw->y = INVERT_Y((((buf[3] & 0x20) << 7) |
> > +					 ((buf[1] & 0xf0) << 4) |
> > +					 buf[5]));
> > +
> > +			hw->z = buf[2];
> > +		}
> > +
> >  		if (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) &&
> >  		    ((buf[0] ^ buf[3]) & 0x02)) {
> >  			switch (SYN_CAP_MULTI_BUTTON_NO(priv->ext_cap) & ~0x01) {
> 

-- 
Dmitry

[PATCH 06/12] Input: synaptics - fuzz position if touchpad reports reduced filtering

[djkurtz]

From: Daniel Kurtz <djkurtz@chromium.org>

Synaptics touchpads indicate via a capability bit when they perform
reduced filtering on position data.
In such a case, use a non-zero fuzz value.
Fuzz = 8 was chosen empirically by observing the raw position data
reported by a clickpad indicating it had reduced filtering.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/mouse/synaptics.c |   17 ++++++++++-------
 drivers/input/mouse/synaptics.h |    3 +++
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 74b1222..dc54675 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -687,23 +687,27 @@ static psmouse_ret_t synaptics_process_byte(struct psmouse *psmouse)
 static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
 {
 	int i;
+	int fuzz = 0;
 
 	__set_bit(INPUT_PROP_POINTER, dev->propbit);
 
+	if (SYN_CAP_REDUCED_FILTERING(priv->ext_cap_0c))
+		fuzz = SYN_REDUCED_FILTER_FUZZ;
+
 	__set_bit(EV_ABS, dev->evbit);
-	input_set_abs_params(dev, ABS_X,
-			     XMIN_NOMINAL, priv->x_max ?: XMAX_NOMINAL, 0, 0);
-	input_set_abs_params(dev, ABS_Y,
-			     YMIN_NOMINAL, priv->y_max ?: YMAX_NOMINAL, 0, 0);
+	input_set_abs_params(dev, ABS_X, XMIN_NOMINAL,
+			     priv->x_max ?: XMAX_NOMINAL, fuzz, 0);
+	input_set_abs_params(dev, ABS_Y, YMIN_NOMINAL,
+			     priv->y_max ?: YMAX_NOMINAL, fuzz, 0);
 	input_set_abs_params(dev, ABS_PRESSURE, 0, 255, 0, 0);
 
 	if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c)) {
 		__set_bit(INPUT_PROP_SEMI_MT, dev->propbit);
 		input_mt_init_slots(dev, 2);
 		input_set_abs_params(dev, ABS_MT_POSITION_X, XMIN_NOMINAL,
-				     priv->x_max ?: XMAX_NOMINAL, 0, 0);
+				     priv->x_max ?: XMAX_NOMINAL, fuzz, 0);
 		input_set_abs_params(dev, ABS_MT_POSITION_Y, YMIN_NOMINAL,
-				     priv->y_max ?: YMAX_NOMINAL, 0, 0);
+				     priv->y_max ?: YMAX_NOMINAL, fuzz, 0);
 
 		input_abs_set_res(dev, ABS_MT_POSITION_X, priv->x_res);
 		input_abs_set_res(dev, ABS_MT_POSITION_Y, priv->y_res);
@@ -977,4 +981,3 @@ bool synaptics_supported(void)
 }
 
 #endif /* CONFIG_MOUSE_PS2_SYNAPTICS */
-
diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
index 34bedde..8a68e66 100644
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -110,6 +110,9 @@
 #define SYN_NEWABS_RELAXED		2
 #define SYN_OLDABS			3
 
+/* amount to fuzz position data when touchpad reports reduced filtering */
+#define SYN_REDUCED_FILTER_FUZZ		8
+
 /*
  * A structure to describe the state of the touchpad hardware (buttons and pad)
  */
-- 
1.7.3.1

Re: [PATCH 06/12] Input: synaptics - fuzz position if touchpad reports reduced filtering

[Chase Douglas]

On 06/28/2011 10:07 PM, djkurtz@chromium.org wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
> 
> Synaptics touchpads indicate via a capability bit when they perform
> reduced filtering on position data.
> In such a case, use a non-zero fuzz value.
> Fuzz = 8 was chosen empirically by observing the raw position data
> reported by a clickpad indicating it had reduced filtering.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

Seems reasonable to me.

Acked-by: Chase Douglas <chase.douglas@canonical.com>

> ---
>  drivers/input/mouse/synaptics.c |   17 ++++++++++-------
>  drivers/input/mouse/synaptics.h |    3 +++
>  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 74b1222..dc54675 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -687,23 +687,27 @@ static psmouse_ret_t synaptics_process_byte(struct psmouse *psmouse)
>  static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
>  {
>  	int i;
> +	int fuzz = 0;
>  
>  	__set_bit(INPUT_PROP_POINTER, dev->propbit);
>  
> +	if (SYN_CAP_REDUCED_FILTERING(priv->ext_cap_0c))
> +		fuzz = SYN_REDUCED_FILTER_FUZZ;
> +
>  	__set_bit(EV_ABS, dev->evbit);
> -	input_set_abs_params(dev, ABS_X,
> -			     XMIN_NOMINAL, priv->x_max ?: XMAX_NOMINAL, 0, 0);
> -	input_set_abs_params(dev, ABS_Y,
> -			     YMIN_NOMINAL, priv->y_max ?: YMAX_NOMINAL, 0, 0);
> +	input_set_abs_params(dev, ABS_X, XMIN_NOMINAL,
> +			     priv->x_max ?: XMAX_NOMINAL, fuzz, 0);
> +	input_set_abs_params(dev, ABS_Y, YMIN_NOMINAL,
> +			     priv->y_max ?: YMAX_NOMINAL, fuzz, 0);
>  	input_set_abs_params(dev, ABS_PRESSURE, 0, 255, 0, 0);
>  
>  	if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c)) {
>  		__set_bit(INPUT_PROP_SEMI_MT, dev->propbit);
>  		input_mt_init_slots(dev, 2);
>  		input_set_abs_params(dev, ABS_MT_POSITION_X, XMIN_NOMINAL,
> -				     priv->x_max ?: XMAX_NOMINAL, 0, 0);
> +				     priv->x_max ?: XMAX_NOMINAL, fuzz, 0);
>  		input_set_abs_params(dev, ABS_MT_POSITION_Y, YMIN_NOMINAL,
> -				     priv->y_max ?: YMAX_NOMINAL, 0, 0);
> +				     priv->y_max ?: YMAX_NOMINAL, fuzz, 0);
>  
>  		input_abs_set_res(dev, ABS_MT_POSITION_X, priv->x_res);
>  		input_abs_set_res(dev, ABS_MT_POSITION_Y, priv->y_res);
> @@ -977,4 +981,3 @@ bool synaptics_supported(void)
>  }
>  
>  #endif /* CONFIG_MOUSE_PS2_SYNAPTICS */
> -
> diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
> index 34bedde..8a68e66 100644
> --- a/drivers/input/mouse/synaptics.h
> +++ b/drivers/input/mouse/synaptics.h
> @@ -110,6 +110,9 @@
>  #define SYN_NEWABS_RELAXED		2
>  #define SYN_OLDABS			3
>  
> +/* amount to fuzz position data when touchpad reports reduced filtering */
> +#define SYN_REDUCED_FILTER_FUZZ		8
> +
>  /*
>   * A structure to describe the state of the touchpad hardware (buttons and pad)
>   */

Re: [PATCH 06/12] Input: synaptics - fuzz position if touchpad reports reduced filtering

[Dmitry Torokhov]

On Tue, Jul 05, 2011 at 10:49:59AM -0700, Chase Douglas wrote:
> On 06/28/2011 10:07 PM, djkurtz@chromium.org wrote:
> > From: Daniel Kurtz <djkurtz@chromium.org>
> > 
> > Synaptics touchpads indicate via a capability bit when they perform
> > reduced filtering on position data.
> > In such a case, use a non-zero fuzz value.
> > Fuzz = 8 was chosen empirically by observing the raw position data
> > reported by a clickpad indicating it had reduced filtering.
> > 
> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> 
> Seems reasonable to me.
> 
> Acked-by: Chase Douglas <chase.douglas@canonical.com>

Applied (folding necessary parts of the patch 1 into this one), thanks
Daniel.

-- 
Dmitry

[PATCH 07/12] Input: synaptics - rename synaptics_data.mt to agm

[djkurtz]

From: Daniel Kurtz <djkurtz@chromium.org>

Rename the synaptics_data.mt field to priv->agm to indicate it is the
hardware state reported in a synaptics AGM packet.

When a Synaptics touchpad is in "AGM" mode, and multiple fingers are
detected, the touchpad sends alternating "Advanced Gesture Mode" (AGM) and
"Simple Gesture Mode" (SGM) packets.
  The AGM packets have w=2, and contain reduced resolution finger data.
  The SGM packets have w={0,1} and contain full resolution finger data.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/mouse/synaptics.c |    9 +++++----
 drivers/input/mouse/synaptics.h |    2 +-
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index dc54675..4e5e454 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -436,10 +436,10 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
 
 		if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) && hw->w == 2) {
 			/* Gesture packet: (x, y, z) at half resolution */
-			priv->mt.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
-			priv->mt.y = INVERT_Y((((buf[4] & 0xf0) << 4)
+			priv->agm.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
+			priv->agm.y = INVERT_Y((((buf[4] & 0xf0) << 4)
 					      | buf[2]) << 1);
-			priv->mt.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
+			priv->agm.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
 			return 1;
 		} else {
 			hw->x = (((buf[3] & 0x10) << 8) |
@@ -576,7 +576,8 @@ static void synaptics_process_packet(struct psmouse *psmouse)
 	}
 
 	if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c))
-		synaptics_report_semi_mt_data(dev, &hw, &priv->mt, num_fingers);
+		synaptics_report_semi_mt_data(dev, &hw, &priv->agm,
+					      num_fingers);
 
 	/* Post events
 	 * BTN_TOUCH has to be first as mousedev relies on it when doing
diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
index 8a68e66..e367239 100644
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -147,7 +147,7 @@ struct synaptics_data {
 
 	struct serio *pt_port;			/* Pass-through serio port */
 
-	struct synaptics_hw_state mt;		/* current gesture packet */
+	struct synaptics_hw_state agm;		/* last AGM packet */
 };
 
 void synaptics_module_init(void);
-- 
1.7.3.1

Re: [PATCH 07/12] Input: synaptics - rename synaptics_data.mt to agm

[Henrik Rydberg]

On Wed, Jun 29, 2011 at 01:07:17PM +0800, djkurtz@chromium.org wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
> 
> Rename the synaptics_data.mt field to priv->agm to indicate it is the
> hardware state reported in a synaptics AGM packet.
> 
> When a Synaptics touchpad is in "AGM" mode, and multiple fingers are
> detected, the touchpad sends alternating "Advanced Gesture Mode" (AGM) and
> "Simple Gesture Mode" (SGM) packets.
>   The AGM packets have w=2, and contain reduced resolution finger data.
>   The SGM packets have w={0,1} and contain full resolution finger data.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---

How about just modifying the comment, i.e., leaving this hunk?

> @@ -147,7 +147,7 @@ struct synaptics_data {
>  
>  	struct serio *pt_port;			/* Pass-through serio port */
>  
> -	struct synaptics_hw_state mt;		/* current gesture packet */
> +	struct synaptics_hw_state agm;		/* last AGM packet */
>  };
>  
>  void synaptics_module_init(void);
> -- 

Thanks,
Henrik

Re: [PATCH 07/12] Input: synaptics - rename synaptics_data.mt to agm

[Daniel Kurtz]

On Tue, Jul 5, 2011 at 5:26 AM, Henrik Rydberg <rydberg@euromail.se> wrote:
> On Wed, Jun 29, 2011 at 01:07:17PM +0800, djkurtz@chromium.org wrote:
>> From: Daniel Kurtz <djkurtz@chromium.org>
>>
>> Rename the synaptics_data.mt field to priv->agm to indicate it is the
>> hardware state reported in a synaptics AGM packet.
>>
>> When a Synaptics touchpad is in "AGM" mode, and multiple fingers are
>> detected, the touchpad sends alternating "Advanced Gesture Mode" (AGM) and
>> "Simple Gesture Mode" (SGM) packets.
>>   The AGM packets have w=2, and contain reduced resolution finger data.
>>   The SGM packets have w={0,1} and contain full resolution finger data.
>>
>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>> ---
>
> How about just modifying the comment, i.e., leaving this hunk?

IMHO, changing the field name from mt to agm makes the following
patches more clear, by making it easier to see the difference between
sgm and agm packets.

>
>> @@ -147,7 +147,7 @@ struct synaptics_data {
>>
>>       struct serio *pt_port;                  /* Pass-through serio port */
>>
>> -     struct synaptics_hw_state mt;           /* current gesture packet */
>> +     struct synaptics_hw_state agm;          /* last AGM packet */
>>  };
>>
>>  void synaptics_module_init(void);
>> --
>
> Thanks,
> Henrik
> --
> 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 07/12] Input: synaptics - rename synaptics_data.mt to agm

[Henrik Rydberg]

> > How about just modifying the comment, i.e., leaving this hunk?
> 
> IMHO, changing the field name from mt to agm makes the following
> patches more clear, by making it easier to see the difference between
> sgm and agm packets.

If there was a field called sgm, I would agree with you. ;-)

Thanks,
Henrik

Re: [PATCH 07/12] Input: synaptics - rename synaptics_data.mt to agm

[Chase Douglas]

On 06/28/2011 10:07 PM, djkurtz@chromium.org wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
> 
> Rename the synaptics_data.mt field to priv->agm to indicate it is the
> hardware state reported in a synaptics AGM packet.
> 
> When a Synaptics touchpad is in "AGM" mode, and multiple fingers are
> detected, the touchpad sends alternating "Advanced Gesture Mode" (AGM) and
> "Simple Gesture Mode" (SGM) packets.
>   The AGM packets have w=2, and contain reduced resolution finger data.
>   The SGM packets have w={0,1} and contain full resolution finger data.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/input/mouse/synaptics.c |    9 +++++----
>  drivers/input/mouse/synaptics.h |    2 +-
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index dc54675..4e5e454 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -436,10 +436,10 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
>  
>  		if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) && hw->w == 2) {
>  			/* Gesture packet: (x, y, z) at half resolution */
> -			priv->mt.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
> -			priv->mt.y = INVERT_Y((((buf[4] & 0xf0) << 4)
> +			priv->agm.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
> +			priv->agm.y = INVERT_Y((((buf[4] & 0xf0) << 4)
>  					      | buf[2]) << 1);
> -			priv->mt.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
> +			priv->agm.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
>  			return 1;
>  		} else {
>  			hw->x = (((buf[3] & 0x10) << 8) |
> @@ -576,7 +576,8 @@ static void synaptics_process_packet(struct psmouse *psmouse)
>  	}
>  
>  	if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c))
> -		synaptics_report_semi_mt_data(dev, &hw, &priv->mt, num_fingers);
> +		synaptics_report_semi_mt_data(dev, &hw, &priv->agm,
> +					      num_fingers);
>  
>  	/* Post events
>  	 * BTN_TOUCH has to be first as mousedev relies on it when doing
> diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
> index 8a68e66..e367239 100644
> --- a/drivers/input/mouse/synaptics.h
> +++ b/drivers/input/mouse/synaptics.h
> @@ -147,7 +147,7 @@ struct synaptics_data {
>  
>  	struct serio *pt_port;			/* Pass-through serio port */
>  
> -	struct synaptics_hw_state mt;		/* current gesture packet */
> +	struct synaptics_hw_state agm;		/* last AGM packet */
>  };
>  
>  void synaptics_module_init(void);

I'm fine with changing the name of the element, especially if it
conforms to some protocol documentation. I'd like to ask that AGM be
spelled out in the comment, with a note that it contains half-resolution
second-touch data.

Otherwise:

Acked-by: Chase Douglas <chase.douglas@canonical.com>

[PATCH 08/12] Input: synaptics - rename set_slot to be more descriptive

[djkurtz]

From: Daniel Kurtz <djkurtz@chromium.org>

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/mouse/synaptics.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 4e5e454..0a51b0ba 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -489,7 +489,8 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
 	return 0;
 }
 
-static void set_slot(struct input_dev *dev, int slot, bool active, int x, int y)
+static void synaptics_report_semi_mt_slot(struct input_dev *dev, int slot,
+					  bool active, int x, int y)
 {
 	input_mt_slot(dev, slot);
 	input_mt_report_slot_state(dev, MT_TOOL_FINGER, active);
@@ -505,14 +506,16 @@ static void synaptics_report_semi_mt_data(struct input_dev *dev,
 					  int num_fingers)
 {
 	if (num_fingers >= 2) {
-		set_slot(dev, 0, true, min(a->x, b->x), min(a->y, b->y));
-		set_slot(dev, 1, true, max(a->x, b->x), max(a->y, b->y));
+		synaptics_report_semi_mt_slot(dev, 0, true, min(a->x, b->x),
+					      min(a->y, b->y));
+		synaptics_report_semi_mt_slot(dev, 1, true, max(a->x, b->x),
+					      max(a->y, b->y));
 	} else if (num_fingers == 1) {
-		set_slot(dev, 0, true, a->x, a->y);
-		set_slot(dev, 1, false, 0, 0);
+		synaptics_report_semi_mt_slot(dev, 0, true, a->x, a->y);
+		synaptics_report_semi_mt_slot(dev, 1, false, 0, 0);
 	} else {
-		set_slot(dev, 0, false, 0, 0);
-		set_slot(dev, 1, false, 0, 0);
+		synaptics_report_semi_mt_slot(dev, 0, false, 0, 0);
+		synaptics_report_semi_mt_slot(dev, 1, false, 0, 0);
 	}
 }
 
-- 
1.7.3.1

Re: [PATCH 08/12] Input: synaptics - rename set_slot to be more descriptive

[Chase Douglas]

On 06/28/2011 10:07 PM, djkurtz@chromium.org wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

I like the new name much better :).

Acked-by: Chase Douglas <chase.douglas@canonical.com>

> ---
>  drivers/input/mouse/synaptics.c |   17 ++++++++++-------
>  1 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 4e5e454..0a51b0ba 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -489,7 +489,8 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
>  	return 0;
>  }
>  
> -static void set_slot(struct input_dev *dev, int slot, bool active, int x, int y)
> +static void synaptics_report_semi_mt_slot(struct input_dev *dev, int slot,
> +					  bool active, int x, int y)
>  {
>  	input_mt_slot(dev, slot);
>  	input_mt_report_slot_state(dev, MT_TOOL_FINGER, active);
> @@ -505,14 +506,16 @@ static void synaptics_report_semi_mt_data(struct input_dev *dev,
>  					  int num_fingers)
>  {
>  	if (num_fingers >= 2) {
> -		set_slot(dev, 0, true, min(a->x, b->x), min(a->y, b->y));
> -		set_slot(dev, 1, true, max(a->x, b->x), max(a->y, b->y));
> +		synaptics_report_semi_mt_slot(dev, 0, true, min(a->x, b->x),
> +					      min(a->y, b->y));
> +		synaptics_report_semi_mt_slot(dev, 1, true, max(a->x, b->x),
> +					      max(a->y, b->y));
>  	} else if (num_fingers == 1) {
> -		set_slot(dev, 0, true, a->x, a->y);
> -		set_slot(dev, 1, false, 0, 0);
> +		synaptics_report_semi_mt_slot(dev, 0, true, a->x, a->y);
> +		synaptics_report_semi_mt_slot(dev, 1, false, 0, 0);
>  	} else {
> -		set_slot(dev, 0, false, 0, 0);
> -		set_slot(dev, 1, false, 0, 0);
> +		synaptics_report_semi_mt_slot(dev, 0, false, 0, 0);
> +		synaptics_report_semi_mt_slot(dev, 1, false, 0, 0);
>  	}
>  }
>  

Re: [PATCH 08/12] Input: synaptics - rename set_slot to be more descriptive

[Dmitry Torokhov]

On Tue, Jul 05, 2011 at 10:54:07AM -0700, Chase Douglas wrote:
> On 06/28/2011 10:07 PM, djkurtz@chromium.org wrote:
> > From: Daniel Kurtz <djkurtz@chromium.org>
> > 
> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> 
> I like the new name much better :).
> 
> Acked-by: Chase Douglas <chase.douglas@canonical.com>

Applied.

-- 
Dmitry

[PATCH 09/12] Input: synaptics - add image sensor support

[djkurtz]

From: Daniel Kurtz <djkurtz@chromium.org>

Synaptics makes (at least) two kinds of touchpad sensors:
 * Older pads use a profile sensor that could only infer the location
   of individual fingers based on the projection of their profiles
   onto row and column sensors.
 * Newer pads use an image sensor that can track true finger position
   using a two-dimensional sensor grid.

Both sensor types support an "Advanced Gesture Mode":
 When multiple fingers are detected, the touchpad sends alternating
 "Advanced Gesture Mode" (AGM) and "Simple Gesture Mode" (SGM)
 packets.
 The AGM packets have w=2, and contain reduced resolution finger data
 The SGM packets have w={0,1} and contain full resolution finger data

Profile sensors try to report the "upper" (larger y value) finger in
 the SGM packet, and the lower (smaller y value) in the AGM packet.
 However, due to the nature of the profile sensor, they easily get
 confused when fingers cross, and can start reporting the x-coordinate
 of one with the y-coordinate of the other.  Thus, for profile
 sensors, "semi-mt" was created, which reports a "bounding box"
 created by pairing min and max coordinates of the two pairs of
 reported fingers.

Image sensors can report the actual coordinates of two of the fingers
 present.  This patch detects if the touchpad is an image sensor and
 reports finger data using the MT-B protocol.

NOTE: This patch only adds partial support for 2-finger gestures.
      The proper interpretation of the slot contents when more than
      two fingers are present is left to later patches.  Also,
      handling of 'number of fingers' transitions is incomplete.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/mouse/synaptics.c |  116 ++++++++++++++++++++++++++++++++++++++-
 drivers/input/mouse/synaptics.h |   13 ++++
 2 files changed, 126 insertions(+), 3 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 0a51b0ba..2d7ac0a 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -286,7 +286,8 @@ static int synaptics_set_advanced_gesture_mode(struct psmouse *psmouse)
 	static unsigned char param = 0xc8;
 	struct synaptics_data *priv = psmouse->private;
 
-	if (!SYN_CAP_ADV_GESTURE(priv->ext_cap_0c))
+	if (!(SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) ||
+			SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c)))
 		return 0;
 
 	if (psmouse_sliced_command(psmouse, SYN_QUE_MODEL))
@@ -434,7 +435,9 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
 			hw->down = ((buf[0] ^ buf[3]) & 0x02) ? 1 : 0;
 		}
 
-		if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c) && hw->w == 2) {
+		if ((SYN_CAP_ADV_GESTURE(priv->ext_cap_0c)
+				|| SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c))
+				&& hw->w == 2) {
 			/* Gesture packet: (x, y, z) at half resolution */
 			priv->agm.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
 			priv->agm.y = INVERT_Y((((buf[4] & 0xf0) << 4)
@@ -519,6 +522,92 @@ static void synaptics_report_semi_mt_data(struct input_dev *dev,
 	}
 }
 
+static void synaptics_report_mt_slot(struct input_dev *dev, int slot, int state,
+				     const struct synaptics_hw_state *sgm,
+				     const struct synaptics_hw_state *agm)
+{
+	input_mt_slot(dev, slot);
+	switch (state) {
+	case SYN_SLOT_EMPTY:
+		input_mt_report_slot_state(dev, MT_TOOL_FINGER, false);
+		break;
+	case SYN_SLOT_SGM:
+		input_mt_report_slot_state(dev, MT_TOOL_FINGER, true);
+		input_report_abs(dev, ABS_MT_POSITION_X, sgm->x);
+		input_report_abs(dev, ABS_MT_POSITION_Y, sgm->y);
+		input_report_abs(dev, ABS_MT_PRESSURE, sgm->z);
+		if (sgm->w >= 4)
+			input_report_abs(dev, ABS_MT_TOUCH_MAJOR, sgm->w);
+		break;
+	case SYN_SLOT_AGM:
+		input_mt_report_slot_state(dev, MT_TOOL_FINGER, true);
+		input_report_abs(dev, ABS_MT_POSITION_X, agm->x);
+		input_report_abs(dev, ABS_MT_POSITION_Y, agm->y);
+		input_report_abs(dev, ABS_MT_PRESSURE, agm->z);
+		break;
+	}
+}
+
+static void synaptics_update_slots(struct synaptics_data *priv, int count,
+				   int sgm, int agm)
+{
+	int i;
+
+	/* First, clear previous slots. */
+	for (i = 0; i < SYN_TRACK_SLOT_COUNT; i++)
+		priv->slot[i] = SYN_SLOT_EMPTY;
+
+	if (count < 1)
+		return;
+	priv->slot[sgm] = SYN_SLOT_SGM;
+
+	if (count < 2)
+		return;
+	priv->slot[agm] = SYN_SLOT_AGM;
+}
+
+static void synaptics_process_hw_state(struct synaptics_data *priv,
+				       struct synaptics_hw_state *sgm)
+{
+	int new_num_fingers;
+
+	if (sgm->z == 0)
+		new_num_fingers = 0;
+	else if (sgm->w >= 4)
+		new_num_fingers = 1;
+	else if (sgm->w == 0)
+		new_num_fingers = 2;
+	else if (sgm->w == 1)
+		new_num_fingers = 3;
+
+	switch (new_num_fingers) {
+	case 0:
+		synaptics_update_slots(priv, 0, 0, 0);
+		break;
+	case 1:
+		synaptics_update_slots(priv, 1, 0, 0);
+		break;
+	case 2:
+	case 3: /* Fall-through case */
+		synaptics_update_slots(priv, 2, 0, 1);
+		break;
+	}
+
+	priv->num_fingers = new_num_fingers;
+}
+
+static void synaptics_report_mt_data(struct psmouse *psmouse,
+				     struct synaptics_hw_state *sgm)
+{
+	struct input_dev *dev = psmouse->dev;
+	struct synaptics_data *priv = psmouse->private;
+	struct synaptics_hw_state *agm = &priv->agm;
+	int i;
+
+	for (i = 0; i < SYN_TRACK_SLOT_COUNT; i++)
+		synaptics_report_mt_slot(dev, i, priv->slot[i], sgm, agm);
+}
+
 /*
  *  called for each full received packet from the touchpad
  */
@@ -534,6 +623,15 @@ static void synaptics_process_packet(struct psmouse *psmouse)
 	if (synaptics_parse_hw_state(psmouse->packet, priv, &hw))
 		return;
 
+	if (SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c)) {
+		synaptics_process_hw_state(priv, &hw);
+		synaptics_report_mt_data(psmouse, &hw);
+		input_mt_report_pointer_emulation(dev, true);
+		input_report_key(dev, BTN_LEFT, hw.left);
+		input_sync(dev);
+		return;
+	}
+
 	if (hw.scroll) {
 		priv->scroll += hw.scroll;
 
@@ -705,7 +803,19 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
 			     priv->y_max ?: YMAX_NOMINAL, fuzz, 0);
 	input_set_abs_params(dev, ABS_PRESSURE, 0, 255, 0, 0);
 
-	if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c)) {
+	if (SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c)) {
+		input_mt_init_slots(dev, SYN_TRACK_SLOT_COUNT);
+		input_set_abs_params(dev, ABS_MT_POSITION_X, XMIN_NOMINAL,
+				     priv->x_max ?: XMAX_NOMINAL, fuzz, 0);
+		input_set_abs_params(dev, ABS_MT_POSITION_Y, YMIN_NOMINAL,
+				     priv->y_max ?: YMAX_NOMINAL, fuzz, 0);
+		input_set_abs_params(dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
+		input_set_abs_params(dev, ABS_MT_TOUCH_MAJOR, 4, 15, 0, 0);
+
+		input_abs_set_res(dev, ABS_MT_POSITION_X, priv->x_res);
+		input_abs_set_res(dev, ABS_MT_POSITION_Y, priv->y_res);
+
+	} else if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c)) {
 		__set_bit(INPUT_PROP_SEMI_MT, dev->propbit);
 		input_mt_init_slots(dev, 2);
 		input_set_abs_params(dev, ABS_MT_POSITION_X, XMIN_NOMINAL,
diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
index e367239..1de2256 100644
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -73,6 +73,8 @@
  * 2	0x04	reduced filtering	firmware does less filtering on
  *					position data, driver should watch
  *					for noise.
+ * 2	0x08	image sensor		image sensor tracks 5 fingers, but only
+ *					reports 2.
  */
 #define SYN_CAP_ADJ_THRESHOLD(ex0c)	((ex0c) & 0x010000)
 #define SYN_CAP_MAX_DIMENSIONS(ex0c)	((ex0c) & 0x020000)
@@ -84,6 +86,7 @@
 #define SYN_CAP_CLICKPAD2BTN(ex0c)	((ex0c) & 0x000100) /* 2-button ClickPad */
 #define SYN_CAP_DELUXE_LED(ex0c)	((ex0c) & 0x000200)
 #define SYN_CAP_REDUCED_FILTERING(ex0c)	((ex0c) & 0x000400)
+#define SYN_CAP_IMAGE_SENSOR(ex0c)	((ex0c) & 0x000800)
 
 /* synaptics modes query bits */
 #define SYN_MODE_ABSOLUTE(m)		((m) & (1 << 7))
@@ -113,6 +116,14 @@
 /* amount to fuzz position data when touchpad reports reduced filtering */
 #define SYN_REDUCED_FILTER_FUZZ		8
 
+/* synaptics tracking slot states */
+#define SYN_SLOT_EMPTY			0
+#define SYN_SLOT_SGM			1
+#define SYN_SLOT_AGM			2
+
+/* number of tracking slots for Image Sensor firmware */
+#define SYN_TRACK_SLOT_COUNT		2
+
 /*
  * A structure to describe the state of the touchpad hardware (buttons and pad)
  */
@@ -148,6 +159,8 @@ struct synaptics_data {
 	struct serio *pt_port;			/* Pass-through serio port */
 
 	struct synaptics_hw_state agm;		/* last AGM packet */
+	int num_fingers;			/* current finger count */
+	int slot[SYN_TRACK_SLOT_COUNT];		/* finger slot state */
 };
 
 void synaptics_module_init(void);
-- 
1.7.3.1

Re: [PATCH 09/12] Input: synaptics - add image sensor support

[Henrik Rydberg]

Hi Daniel,

> Synaptics makes (at least) two kinds of touchpad sensors:
>  * Older pads use a profile sensor that could only infer the location
>    of individual fingers based on the projection of their profiles
>    onto row and column sensors.
>  * Newer pads use an image sensor that can track true finger position
>    using a two-dimensional sensor grid.
> 
> Both sensor types support an "Advanced Gesture Mode":
>  When multiple fingers are detected, the touchpad sends alternating
>  "Advanced Gesture Mode" (AGM) and "Simple Gesture Mode" (SGM)
>  packets.
>  The AGM packets have w=2, and contain reduced resolution finger data
>  The SGM packets have w={0,1} and contain full resolution finger data
> 
> Profile sensors try to report the "upper" (larger y value) finger in
>  the SGM packet, and the lower (smaller y value) in the AGM packet.
>  However, due to the nature of the profile sensor, they easily get
>  confused when fingers cross, and can start reporting the x-coordinate
>  of one with the y-coordinate of the other.  Thus, for profile
>  sensors, "semi-mt" was created, which reports a "bounding box"
>  created by pairing min and max coordinates of the two pairs of
>  reported fingers.
> 
> Image sensors can report the actual coordinates of two of the fingers
>  present.  This patch detects if the touchpad is an image sensor and
>  reports finger data using the MT-B protocol.
> 
> NOTE: This patch only adds partial support for 2-finger gestures.
>       The proper interpretation of the slot contents when more than
>       two fingers are present is left to later patches.  Also,
>       handling of 'number of fingers' transitions is incomplete.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

I guess the real question here is: do the following patches really
help? Creating additional logic to band-aid yet another special case,
which still does not give full MT support, seems to create more
problems than it solves. If the code was needed to ensure proper five
finger support to userspace, then maybe one could live with
it. However, as it stands, keeping the semi-mt behavior also for the
slightly better devices may not be such a bad idea, after all.

_Iff_ the whole series can be formulated as true protocol B support
(no special flags, please), and _iff_ it helps to use software finger
tracking for less than four fingers, then please do tell, and we can
add that part to the input core to simplify the synaptics
implementation a bit.

Thanks.
Henrik

Re: [PATCH 09/12] Input: synaptics - add image sensor support

[Daniel Kurtz]

Hi Henrik,

On Tue, Jul 5, 2011 at 5:42 AM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Daniel,
>
>> Synaptics makes (at least) two kinds of touchpad sensors:
>>  * Older pads use a profile sensor that could only infer the location
>>    of individual fingers based on the projection of their profiles
>>    onto row and column sensors.
>>  * Newer pads use an image sensor that can track true finger position
>>    using a two-dimensional sensor grid.
>>
>> Both sensor types support an "Advanced Gesture Mode":
>>  When multiple fingers are detected, the touchpad sends alternating
>>  "Advanced Gesture Mode" (AGM) and "Simple Gesture Mode" (SGM)
>>  packets.
>>  The AGM packets have w=2, and contain reduced resolution finger data
>>  The SGM packets have w={0,1} and contain full resolution finger data
>>
>> Profile sensors try to report the "upper" (larger y value) finger in
>>  the SGM packet, and the lower (smaller y value) in the AGM packet.
>>  However, due to the nature of the profile sensor, they easily get
>>  confused when fingers cross, and can start reporting the x-coordinate
>>  of one with the y-coordinate of the other.  Thus, for profile
>>  sensors, "semi-mt" was created, which reports a "bounding box"
>>  created by pairing min and max coordinates of the two pairs of
>>  reported fingers.
>>
>> Image sensors can report the actual coordinates of two of the fingers
>>  present.  This patch detects if the touchpad is an image sensor and
>>  reports finger data using the MT-B protocol.
>>
>> NOTE: This patch only adds partial support for 2-finger gestures.
>>       The proper interpretation of the slot contents when more than
>>       two fingers are present is left to later patches.  Also,
>>       handling of 'number of fingers' transitions is incomplete.
>>
>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>
> I guess the real question here is: do the following patches really
> help? Creating additional logic to band-aid yet another special case,
> which still does not give full MT support, seems to create more
> problems than it solves. If the code was needed to ensure proper five
> finger support to userspace, then maybe one could live with
> it. However, as it stands, keeping the semi-mt behavior also for the
> slightly better devices may not be such a bad idea, after all.
>
> _Iff_ the whole series can be formulated as true protocol B support
> (no special flags, please), and _iff_ it helps to use software finger
> tracking for less than four fingers, then please do tell, and we can
> add that part to the input core to simplify the synaptics
> implementation a bit.

Image sensors and profile sensors behave differently.  However, even
the image sensors do not allow true MT-B, since they only report 2
fingers.  Hence, it makes sense to add a property to distinguish the 3
cases.

This implementation addresses the following issues:

  (1) Improves handling of the 2-finger case.  Image sensors due allow
true MT-B for two finger case.  This, for example, allows detecting
whether the click in a click+drag is happening in bottom right or
bottom left quadrant of the pad.

  (2) Helps improve handling of number-of-fingers transitions.  Most
of the complexity of the synaptics driver comes with dealing with the
complicated way that the protocol handles number-of-fingers
transitions.  Just ignoring them with semi-mt could cause lots of
jumpy behavior as the transitions are mapped to bounding boxes whose
coordinates jump around.  Thus, this implementation tries to notify
userspace of finger transition by 'rolling slots' when necessary.
What I mean is, if the driver deduces that a given slot may not
contain the same finger as a previous report, it releases it (sets its
tracking_id = -1), and reestablishes the slot with a new tracking_id
when it is more confident that it is now tracking a new finger.

  (3) Properly decode the "AGM_CONTACT" packet type.  4- and 5- finger
gestures may never be supported, however, I think it is still a good
idea to detect and parse these packets properly in the kernel driver,
and leave policy to userspace.  I'm open to alternative suggestions
for ways to represent this to userspace.  The idea in this patch set
is to reuse the MT-B plumbing as much as possible, but use the device
property to mark the fact that the interpretation of the resulting
slots is somewhat different.

A bare minimum patchset might do:
   (1) Do true MT-B for two fingers
   (2) Improved number-of-finger tracking for just the 2-finger case.
(Keep tracking finger 1 when finger 2 is removed, and vice versa)
   (3) Properly parse, but don't use, AGM_CONTACT packets

We can then argue about what to do with 3+ fingers :).

Thanks,
Daniel

>
> Thanks.
> Henrik
>

Re: [PATCH 09/12] Input: synaptics - add image sensor support

[Henrik Rydberg]

> > I guess the real question here is: do the following patches really
> > help? Creating additional logic to band-aid yet another special case,
> > which still does not give full MT support, seems to create more
> > problems than it solves. If the code was needed to ensure proper five
> > finger support to userspace, then maybe one could live with
> > it. However, as it stands, keeping the semi-mt behavior also for the
> > slightly better devices may not be such a bad idea, after all.
> >
> > _Iff_ the whole series can be formulated as true protocol B support
> > (no special flags, please), and _iff_ it helps to use software finger
> > tracking for less than four fingers, then please do tell, and we can
> > add that part to the input core to simplify the synaptics
> > implementation a bit.
> 
> Image sensors and profile sensors behave differently.  However, even
> the image sensors do not allow true MT-B, since they only report 2
> fingers.  Hence, it makes sense to add a property to distinguish the 3
> cases.

The lifetime of such a solution should also be considered.

> This implementation addresses the following issues:
> 
>   (1) Improves handling of the 2-finger case.  Image sensors due allow
> true MT-B for two finger case.  This, for example, allows detecting
> whether the click in a click+drag is happening in bottom right or
> bottom left quadrant of the pad.

In principle, this could be done within the semi-mt realm by adding a
binary value to the protocol, distinguishing the diagonals of the
bounding box. It would be ugly too, but at least it would be
compatible with the current semi-mt effort in userspace.

>   (2) Helps improve handling of number-of-fingers transitions.  Most
> of the complexity of the synaptics driver comes with dealing with the
> complicated way that the protocol handles number-of-fingers
> transitions.  Just ignoring them with semi-mt could cause lots of
> jumpy behavior as the transitions are mapped to bounding boxes whose
> coordinates jump around.  Thus, this implementation tries to notify
> userspace of finger transition by 'rolling slots' when necessary.

If the kernel driver cannot create a smooth bounding box, which by all
means is simpler than providing proper finger tracking, then there is
no way this can be done in userspace either.

> What I mean is, if the driver deduces that a given slot may not
> contain the same finger as a previous report, it releases it (sets its
> tracking_id = -1), and reestablishes the slot with a new tracking_id
> when it is more confident that it is now tracking a new finger.

As a general question, one might ask oneself: If the new device
_really_ can track five fingers, then why does it not send enough
information to recover that information? A proper tracking id would
suffice.

>   (3) Properly decode the "AGM_CONTACT" packet type.  4- and 5- finger
> gestures may never be supported, however, I think it is still a good
> idea to detect and parse these packets properly in the kernel driver,
> and leave policy to userspace.  I'm open to alternative suggestions
> for ways to represent this to userspace.  The idea in this patch set
> is to reuse the MT-B plumbing as much as possible, but use the device
> property to mark the fact that the interpretation of the resulting
> slots is somewhat different.

If the data cannot be reliably utilized, I doubt anyone cares.  There
is a lot of hardware out there capable of tracking ten and more
fingers, without the agonizing pain.

> A bare minimum patchset might do:
>    (1) Do true MT-B for two fingers
>    (2) Improved number-of-finger tracking for just the 2-finger case.
> (Keep tracking finger 1 when finger 2 is removed, and vice versa)

In what way is this different from 1)?

>    (3) Properly parse, but don't use, AGM_CONTACT packets

I am tempted to write something about Schrödinger's cat here... ;-)

Thanks,
Henrik

Re: [PATCH 09/12] Input: synaptics - add image sensor support

[Daniel Kurtz]

On Wed, Jul 6, 2011 at 3:27 AM, Henrik Rydberg <rydberg@euromail.se> wrote:
>> ...
>> This implementation addresses the following issues:
>>
>>   (1) Improves handling of the 2-finger case.  Image sensors due allow
>> true MT-B for two finger case.  This, for example, allows detecting
>> whether the click in a click+drag is happening in bottom right or
>> bottom left quadrant of the pad.
>
> In principle, this could be done within the semi-mt realm by adding a
> binary value to the protocol, distinguishing the diagonals of the
> bounding box. It would be ugly too, but at least it would be
> compatible with the current semi-mt effort in userspace.

(A) I think the "semi-mt" protocol works like this:
  (1) When two or more fingers are on the pad, report two MT-B slots, where:
    (a) slot[0] contains (x_min, y_min) of the two fingers
    (b) slot[1] contains (x_max, y_max) of the two fingers
    (c) Total number of fingers on pad is indicated by EV_KEY/BTN_TOOL_* is set.
  (2) If one of the two fingers is removed, then the first slot is
used to report
      the remaining finger, whichever it is, without changing the
slot's tracking_id.
  (3) Advertise this behavior with the SEMI_MT device property

(B) This is what the proposed T5R2 driver does:
  (1) When N fingers are on the pad, report N MT-B slots, where:
    (a) The lowest indexed active slot (tracking_id != -1) contains
valid finger position data.
    (b) The highest indexed active slot (tracking_id != -1) contains
valid finger position data.
    (c) Any active slots (tracking_id != -1) between these does not
contain valid finger position data, these 'hidden' slots are just
active to indicate that there are that many fingers on the pad.
    (d) Total number of fingers on the pad is always the number of active slots.
  (2) When fingers are removed from the pad:
    (a) keep reporting the fingers that are left in the same slots,
with the same tracking_ids, if possible.
    (b) If the identities of the fingers that remain is ambiguous,
invalidate all slots,then re-assign the fingers that remain to new
slots, with new tracking ids.
  (3) Advertise this behavior with the T5R2 device property

(C) A third, compromise implementation might be to do something like this:
  (1) When >=2 fingers are on the pad, report 2 MT-B slots, where:
    (a) slot[0] reports finger 1
    (b) slot[1] reports finger 2
    (c) Total number of fingers on pad is indicated by EV_KEY/BTN_TOOL_* is set.
  (2) If 1 finger is removed, the tracking_id for its slot is invalidated;
      but, the finger that remains stays in its same slot with the
same tracking_id.
  (3) Whenever the number of fingers changes from/to 3 or more
fingers, both slots are always invalidated.
      New tracking_ids are assigned to both slots which contain the
two fingers now being reported.
  (4) No special driver property is required for this, not even "semi-mt".
      It should be as pure MT-B as we can get (plus BTN_TOOL_* hack).


IMHO, (C) is an improvement over (A) for two major reasons:
  (1) tracking the individual touches allows distinguishing a clicking
finger in bottom right versus bottom left, for example.
  (2) It makes it possible to track the touch that remains when one of
the two touches is released.
      The touch that remains will stay in the same slot, with the same
tracking id.
      The tracking id of the touch that is removed will be set to -1.
        => This eliminates unnecessary gaps in the event stream,
without introducing position jumps.
      This behavior occurs often when switching between 1-finger
motion and 2f-scroll, or when performing 2f-click+drag gestures.

Note: Tracking which fingers are left on the pad gets much more
complicated with 3, 4, or 5 fingers.
This is addressed more fully by the T5R2 solution, but (C), while
slightly less optimal, should at least not do the wrong thing.

Sorry, I find it a bit difficult to explain this clearly in writing.
I guess I'll just have to submit a new patchset that shows what I mean.


>> A bare minimum patchset might do:
>>    (1) Do true MT-B for two fingers
>>    (2) Improved number-of-finger tracking for just the 2-finger case.
>> (Keep tracking finger 1 when finger 2 is removed, and vice versa)
>
> In what way is this different from 1)?
>
>>    (3) Properly parse, but don't use, AGM_CONTACT packets

The image sensor AGM mode acts different than the profile sensor.  For
the profile sensor, there is always one AGM for every SGM when 2 or
more fingers are present.  With the image sensor, there may be more 1
AGM per SGM if the number of fingers is changing - especially when
there are more than 3 fingers present.  This is what I meant by
properly parse these packets.  At least it is a form of documentation
for the next poor guy who comes along and tries to figure out what in
the world his trackpad is sending him.

Thanks,
-Daniel

>
> I am tempted to write something about Schrödinger's cat here... ;-)
>
> Thanks,
> Henrik
>

Re: [PATCH 09/12] Input: synaptics - add image sensor support

[Chase Douglas]

On 07/06/2011 09:41 AM, Daniel Kurtz wrote:
> On Wed, Jul 6, 2011 at 3:27 AM, Henrik Rydberg <rydberg@euromail.se> wrote:
>>> ...
>>> This implementation addresses the following issues:
>>>
>>>   (1) Improves handling of the 2-finger case.  Image sensors due allow
>>> true MT-B for two finger case.  This, for example, allows detecting
>>> whether the click in a click+drag is happening in bottom right or
>>> bottom left quadrant of the pad.
>>
>> In principle, this could be done within the semi-mt realm by adding a
>> binary value to the protocol, distinguishing the diagonals of the
>> bounding box. It would be ugly too, but at least it would be
>> compatible with the current semi-mt effort in userspace.
> 
> (A) I think the "semi-mt" protocol works like this:
>   (1) When two or more fingers are on the pad, report two MT-B slots, where:
>     (a) slot[0] contains (x_min, y_min) of the two fingers
>     (b) slot[1] contains (x_max, y_max) of the two fingers
>     (c) Total number of fingers on pad is indicated by EV_KEY/BTN_TOOL_* is set.
>   (2) If one of the two fingers is removed, then the first slot is
> used to report
>       the remaining finger, whichever it is, without changing the
> slot's tracking_id.
>   (3) Advertise this behavior with the SEMI_MT device property
> 
> (B) This is what the proposed T5R2 driver does:
>   (1) When N fingers are on the pad, report N MT-B slots, where:
>     (a) The lowest indexed active slot (tracking_id != -1) contains
> valid finger position data.
>     (b) The highest indexed active slot (tracking_id != -1) contains
> valid finger position data.
>     (c) Any active slots (tracking_id != -1) between these does not
> contain valid finger position data, these 'hidden' slots are just
> active to indicate that there are that many fingers on the pad.
>     (d) Total number of fingers on the pad is always the number of active slots.
>   (2) When fingers are removed from the pad:
>     (a) keep reporting the fingers that are left in the same slots,
> with the same tracking_ids, if possible.
>     (b) If the identities of the fingers that remain is ambiguous,
> invalidate all slots,then re-assign the fingers that remain to new
> slots, with new tracking ids.
>   (3) Advertise this behavior with the T5R2 device property
> 
> (C) A third, compromise implementation might be to do something like this:
>   (1) When >=2 fingers are on the pad, report 2 MT-B slots, where:
>     (a) slot[0] reports finger 1
>     (b) slot[1] reports finger 2
>     (c) Total number of fingers on pad is indicated by EV_KEY/BTN_TOOL_* is set.
>   (2) If 1 finger is removed, the tracking_id for its slot is invalidated;
>       but, the finger that remains stays in its same slot with the
> same tracking_id.
>   (3) Whenever the number of fingers changes from/to 3 or more
> fingers, both slots are always invalidated.
>       New tracking_ids are assigned to both slots which contain the
> two fingers now being reported.
>   (4) No special driver property is required for this, not even "semi-mt".
>       It should be as pure MT-B as we can get (plus BTN_TOOL_* hack).

This approach seems better, but there's still an ambiguity. When we go
from two to three or more fingers we get lossy. We can't tell what
touches cross the transition with their data intact.

I'm racking my brain trying to come up with a solution that would make
sense, but I don't think there is one that works seamlessly. I think we
should do what you've written above with two changes:

* Use a new property (T5R2?) to fully denote how screwed up this class
of devices is.
* Don't roll the tracking_id on any number of touch transitions. We're
already telling userspace that the number of touches is changing, so the
tracking_id change is superfluous. The tracking_id of a slot should only
change when we know that the touch is beginning or ending.

No matter what we do, userspace will have to rely on complex heuristics
for this device, which makes me wonder if these devices will really ever
be handled properly...

> IMHO, (C) is an improvement over (A) for two major reasons:
>   (1) tracking the individual touches allows distinguishing a clicking
> finger in bottom right versus bottom left, for example.
>   (2) It makes it possible to track the touch that remains when one of
> the two touches is released.
>       The touch that remains will stay in the same slot, with the same
> tracking id.
>       The tracking id of the touch that is removed will be set to -1.
>         => This eliminates unnecessary gaps in the event stream,
> without introducing position jumps.
>       This behavior occurs often when switching between 1-finger
> motion and 2f-scroll, or when performing 2f-click+drag gestures.
> 
> Note: Tracking which fingers are left on the pad gets much more
> complicated with 3, 4, or 5 fingers.
> This is addressed more fully by the T5R2 solution, but (C), while
> slightly less optimal, should at least not do the wrong thing.
> 
> Sorry, I find it a bit difficult to explain this clearly in writing.
> I guess I'll just have to submit a new patchset that shows what I mean.

You're not the first to be tied up trying to document Synaptics behavior :).

Thanks!

-- Chase

Re: [PATCH 09/12] Input: synaptics - add image sensor support

[Dmitry Torokhov]

On Thu, Jul 07, 2011 at 12:41:26AM +0800, Daniel Kurtz wrote:
> On Wed, Jul 6, 2011 at 3:27 AM, Henrik Rydberg <rydberg@euromail.se> wrote:
> >> ...
> >> This implementation addresses the following issues:
> >>
> >>   (1) Improves handling of the 2-finger case.  Image sensors due allow
> >> true MT-B for two finger case.  This, for example, allows detecting
> >> whether the click in a click+drag is happening in bottom right or
> >> bottom left quadrant of the pad.
> >
> > In principle, this could be done within the semi-mt realm by adding a
> > binary value to the protocol, distinguishing the diagonals of the
> > bounding box. It would be ugly too, but at least it would be
> > compatible with the current semi-mt effort in userspace.
> 
> (A) I think the "semi-mt" protocol works like this:
>   (1) When two or more fingers are on the pad, report two MT-B slots, where:
>     (a) slot[0] contains (x_min, y_min) of the two fingers
>     (b) slot[1] contains (x_max, y_max) of the two fingers
>     (c) Total number of fingers on pad is indicated by EV_KEY/BTN_TOOL_* is set.
>   (2) If one of the two fingers is removed, then the first slot is
> used to report
>       the remaining finger, whichever it is, without changing the
> slot's tracking_id.
>   (3) Advertise this behavior with the SEMI_MT device property
> 
> (B) This is what the proposed T5R2 driver does:
>   (1) When N fingers are on the pad, report N MT-B slots, where:
>     (a) The lowest indexed active slot (tracking_id != -1) contains
> valid finger position data.
>     (b) The highest indexed active slot (tracking_id != -1) contains
> valid finger position data.
>     (c) Any active slots (tracking_id != -1) between these does not
> contain valid finger position data, these 'hidden' slots are just
> active to indicate that there are that many fingers on the pad.
>     (d) Total number of fingers on the pad is always the number of active slots.
>   (2) When fingers are removed from the pad:
>     (a) keep reporting the fingers that are left in the same slots,
> with the same tracking_ids, if possible.
>     (b) If the identities of the fingers that remain is ambiguous,
> invalidate all slots,then re-assign the fingers that remain to new
> slots, with new tracking ids.
>   (3) Advertise this behavior with the T5R2 device property
> 

This is a no-go. I do not want to add TxRy or similar "properties". We
have a workable semi-mt (counding box) and full mt concepts and we
should present userspace with data stream that conforms either one or
another.

> (C) A third, compromise implementation might be to do something like this:
>   (1) When >=2 fingers are on the pad, report 2 MT-B slots, where:
>     (a) slot[0] reports finger 1
>     (b) slot[1] reports finger 2
>     (c) Total number of fingers on pad is indicated by EV_KEY/BTN_TOOL_* is set.
>   (2) If 1 finger is removed, the tracking_id for its slot is invalidated;
>       but, the finger that remains stays in its same slot with the
> same tracking_id.
>   (3) Whenever the number of fingers changes from/to 3 or more
> fingers, both slots are always invalidated.
>       New tracking_ids are assigned to both slots which contain the
> two fingers now being reported.
>   (4) No special driver property is required for this, not even "semi-mt".
>       It should be as pure MT-B as we can get (plus BTN_TOOL_* hack).
> 

This is the best option in my opinion. We will present 2 finger position
data plus extended finger count.

Thanks.

-- 
Dmitry

Re: [PATCH 09/12] Input: synaptics - add image sensor support

[Henrik Rydberg]

> > (C) A third, compromise implementation might be to do something like this:
> >   (1) When >=2 fingers are on the pad, report 2 MT-B slots, where:
> >     (a) slot[0] reports finger 1
> >     (b) slot[1] reports finger 2
> >     (c) Total number of fingers on pad is indicated by EV_KEY/BTN_TOOL_* is set.
> >   (2) If 1 finger is removed, the tracking_id for its slot is invalidated;
> >       but, the finger that remains stays in its same slot with the
> > same tracking_id.
> >   (3) Whenever the number of fingers changes from/to 3 or more
> > fingers, both slots are always invalidated.
> >       New tracking_ids are assigned to both slots which contain the
> > two fingers now being reported.
> >   (4) No special driver property is required for this, not even "semi-mt".
> >       It should be as pure MT-B as we can get (plus BTN_TOOL_* hack).

I believe there is a strong userspace assumption that BTN_TOOL_* has
no meaning for real MT devices. Rightfully so, IMO. Hence, I think
semi-mt needs to be used here as well.

> > 
> 
> This is the best option in my opinion. We will present 2 finger position
> data plus extended finger count.

We never did put all the details of the bounding box coordinates in
writing, so perhaps this is an opportunity to both fix that and extend
usability to the case so described. The only question is whether there
are applications out there which now assume min/max instead of contact
positions. If anyone knows, please speak up. :-) Otherwise, I am very
much for Daniel's case C, with Dmitry's modification.

In short: Use the semi-MT property, and send two suitable fingers
along with it.

Cheers,
Henrik

Re: [PATCH 09/12] Input: synaptics - add image sensor support

[Dmitry Torokhov]

On Wed, Jul 06, 2011 at 08:47:59PM +0200, Henrik Rydberg wrote:
> > > (C) A third, compromise implementation might be to do something like this:
> > >   (1) When >=2 fingers are on the pad, report 2 MT-B slots, where:
> > >     (a) slot[0] reports finger 1
> > >     (b) slot[1] reports finger 2
> > >     (c) Total number of fingers on pad is indicated by EV_KEY/BTN_TOOL_* is set.
> > >   (2) If 1 finger is removed, the tracking_id for its slot is invalidated;
> > >       but, the finger that remains stays in its same slot with the
> > > same tracking_id.
> > >   (3) Whenever the number of fingers changes from/to 3 or more
> > > fingers, both slots are always invalidated.
> > >       New tracking_ids are assigned to both slots which contain the
> > > two fingers now being reported.
> > >   (4) No special driver property is required for this, not even "semi-mt".
> > >       It should be as pure MT-B as we can get (plus BTN_TOOL_* hack).
> 
> I believe there is a strong userspace assumption that BTN_TOOL_* has
> no meaning for real MT devices. Rightfully so, IMO. Hence, I think
> semi-mt needs to be used here as well.

I think we need to adjust userspace to pay attention to BTN_TOOL_* for
MT-B too so that if number of slots advertised does not match
BTN_TOOL_* capabilities that means that the device does not privide
tracking data for all contacts.

Luckily this should be backward-compatible (i.e. older userspace will
ignore "extended" fingercounts, newer will pay attention to it).

> 
> > > 
> > 
> > This is the best option in my opinion. We will present 2 finger position
> > data plus extended finger count.
> 
> We never did put all the details of the bounding box coordinates in
> writing, so perhaps this is an opportunity to both fix that and extend
> usability to the case so described. The only question is whether there
> are applications out there which now assume min/max instead of contact
> positions. If anyone knows, please speak up. :-) Otherwise, I am very
> much for Daniel's case C, with Dmitry's modification.
> 
> In short: Use the semi-MT property, and send two suitable fingers
> along with it.

Umm... but it is my understanding that 2 fingers will provide real
tracking data, not bounding box, so why would we set semi-MT?

Maybe we have different notions of what semi-MT property conveys? For me
semi-MT indicates that the device provides 2 coordinates for bounding
box.  However if semi-MT is not set does not mean that the device
provides true tracking for all contacts, but only for advertised slots.
There still may be additional data transmitted.

Thanks.

-- 
Dmitry

Re: [PATCH 09/12] Input: synaptics - add image sensor support

[Henrik Rydberg]

> > I believe there is a strong userspace assumption that BTN_TOOL_* has
> > no meaning for real MT devices. Rightfully so, IMO. Hence, I think
> > semi-mt needs to be used here as well.
> 
> I think we need to adjust userspace to pay attention to BTN_TOOL_* for
> MT-B too so that if number of slots advertised does not match
> BTN_TOOL_* capabilities that means that the device does not privide
> tracking data for all contacts.

Well, it is possible, but it is a great deal more complex than just
looking at what the slots contain. At least we should all be able to
agree that MT-B is sufficient for any "proper" MT device.

> Luckily this should be backward-compatible (i.e. older userspace will
> ignore "extended" fingercounts, newer will pay attention to it).

OTOH, letting semi-mt engulf all devices which requires the use of
BTN_TOOL_* for finger count makes it easier to differentiate between
various userspace support levels. "This app supports pure MT-B only",
etc.

> > > This is the best option in my opinion. We will present 2 finger position
> > > data plus extended finger count.
> > 
> > We never did put all the details of the bounding box coordinates in
> > writing, so perhaps this is an opportunity to both fix that and extend
> > usability to the case so described. The only question is whether there
> > are applications out there which now assume min/max instead of contact
> > positions. If anyone knows, please speak up. :-) Otherwise, I am very
> > much for Daniel's case C, with Dmitry's modification.
> > 
> > In short: Use the semi-MT property, and send two suitable fingers
> > along with it.
> 
> Umm... but it is my understanding that 2 fingers will provide real
> tracking data, not bounding box, so why would we set semi-MT?

To indicate that a) the two positions may not represent true fingers
but a bounding box, and b) the contact count is determined by
BTN_TOOL_*.

True, there is no way to distinguish between the real-fingers and
bounding-box cases here (that is why I suggested another binary value
in a previous mail), but without semi-mt, there is no way to know a
priori if special logic is needed for the number of fingers.

> Maybe we have different notions of what semi-MT property conveys? For me
> semi-MT indicates that the device provides 2 coordinates for bounding
> box.  However if semi-MT is not set does not mean that the device
> provides true tracking for all contacts, but only for advertised slots.
> There still may be additional data transmitted.

Yes, it seems we do have different assumptions here. The more reason
to document it further. :-)

To me, it seems we do need a little bit of extra information to
determine this new type of device.

Thanks,
Henrik

Re: [PATCH 09/12] Input: synaptics - add image sensor support

[Dmitry Torokhov]

On Wed, Jul 06, 2011 at 09:31:20PM +0200, Henrik Rydberg wrote:
> > > I believe there is a strong userspace assumption that BTN_TOOL_* has
> > > no meaning for real MT devices. Rightfully so, IMO. Hence, I think
> > > semi-mt needs to be used here as well.
> > 
> > I think we need to adjust userspace to pay attention to BTN_TOOL_* for
> > MT-B too so that if number of slots advertised does not match
> > BTN_TOOL_* capabilities that means that the device does not privide
> > tracking data for all contacts.
> 
> Well, it is possible, but it is a great deal more complex than just
> looking at what the slots contain. At least we should all be able to
> agree that MT-B is sufficient for any "proper" MT device.
> 
> > Luckily this should be backward-compatible (i.e. older userspace will
> > ignore "extended" fingercounts, newer will pay attention to it).
> 
> OTOH, letting semi-mt engulf all devices which requires the use of
> BTN_TOOL_* for finger count makes it easier to differentiate between
> various userspace support levels. "This app supports pure MT-B only",
> etc.

Do app writers really want to exclude semi-MT devices even though they
might be usable? I can see wanting to support only MT-B protocol (as opposed
to chatty MT-A) but if semi-MT stream is usable why not use it?

> 
> > > > This is the best option in my opinion. We will present 2 finger position
> > > > data plus extended finger count.
> > > 
> > > We never did put all the details of the bounding box coordinates in
> > > writing, so perhaps this is an opportunity to both fix that and extend
> > > usability to the case so described. The only question is whether there
> > > are applications out there which now assume min/max instead of contact
> > > positions. If anyone knows, please speak up. :-) Otherwise, I am very
> > > much for Daniel's case C, with Dmitry's modification.
> > > 
> > > In short: Use the semi-MT property, and send two suitable fingers
> > > along with it.
> > 
> > Umm... but it is my understanding that 2 fingers will provide real
> > tracking data, not bounding box, so why would we set semi-MT?
> 
> To indicate that a) the two positions may not represent true fingers
> but a bounding box, and b) the contact count is determined by
> BTN_TOOL_*.
> 
> True, there is no way to distinguish between the real-fingers and
> bounding-box cases here

And that is the problem.

> (that is why I suggested another binary value
> in a previous mail), but without semi-mt, there is no way to know a
> priori if special logic is needed for the number of fingers.

This should be pretty straightforward:

	num_fingers = calc_fingers_from_btn_tool(device); // via EVIOCGKEY
	num_slots = get_number_of_slots(device); // EVIOCGABS
	num_untracked_contacts = max(num_fingers - num_slots, 0);

> 
> > Maybe we have different notions of what semi-MT property conveys? For me
> > semi-MT indicates that the device provides 2 coordinates for bounding
> > box.  However if semi-MT is not set does not mean that the device
> > provides true tracking for all contacts, but only for advertised slots.
> > There still may be additional data transmitted.
> 
> Yes, it seems we do have different assumptions here. The more reason
> to document it further. :-)

I'll take patches ;)

> 
> To me, it seems we do need a little bit of extra information to
> determine this new type of device.

I think we already have all we need (see above).

Thanks.

-- 
Dmitry

Re: [PATCH 09/12] Input: synaptics - add image sensor support

[Henrik Rydberg]

> This should be pretty straightforward:
> 
> 	num_fingers = calc_fingers_from_btn_tool(device); // via EVIOCGKEY
> 	num_slots = get_number_of_slots(device); // EVIOCGABS
> 	num_untracked_contacts = max(num_fingers - num_slots, 0);

Heh, I forgot we _do_ know in advance how many fingers are supported
via BTN_TOOL_*. Ok, no more issues.

> > > Maybe we have different notions of what semi-MT property conveys? For me
> > > semi-MT indicates that the device provides 2 coordinates for bounding
> > > box.  However if semi-MT is not set does not mean that the device
> > > provides true tracking for all contacts, but only for advertised slots.
> > > There still may be additional data transmitted.
> > 
> > Yes, it seems we do have different assumptions here. The more reason
> > to document it further. :-)
> 
> I'll take patches ;)

And thou shalt receive them - some day ;-)

> > To me, it seems we do need a little bit of extra information to
> > determine this new type of device.
> 
> I think we already have all we need (see above).

I concur. So, to conclude:

a) The improved synaptics behavior can be achieved by simply using
MT-B plus BTN_TOOL_*.

b) Userspace should check BTN_TOOL_* for any discrepancies between the
maximum number of available slots (always two in this case) and the
maximum number of fingers reported (BTN_TOOL_TRIPLETAP etc). Extra
actions may then be taken to support more fingers than slots.

c) The semi-mt flag is only used to signal that the two points sent
via MT-B are the corners of a bounding box.

Cheers,
Henrik

Re: [PATCH 09/12] Input: synaptics - add image sensor support

[Chase Douglas]

On 07/06/2011 01:20 PM, Henrik Rydberg wrote:
>>> To me, it seems we do need a little bit of extra information to
>>> determine this new type of device.
>>
>> I think we already have all we need (see above).
> 
> I concur. So, to conclude:
> 
> a) The improved synaptics behavior can be achieved by simply using
> MT-B plus BTN_TOOL_*.
> 
> b) Userspace should check BTN_TOOL_* for any discrepancies between the
> maximum number of available slots (always two in this case) and the
> maximum number of fingers reported (BTN_TOOL_TRIPLETAP etc). Extra
> actions may then be taken to support more fingers than slots.
> 
> c) The semi-mt flag is only used to signal that the two points sent
> via MT-B are the corners of a bounding box.

This isn't quite enough. If we don't set the semi-mt flag or any other
new flag, then we'll have slots that become inconsistent when touches
are added or removed. For example, start with two touches being tracked
correctly. Now, add a touch. The second slot will now get the data of
the third touch, which is in a different location. You haven't changed
the tracking_id though, so it looks like the same touch. This is
incorrect behavior. Or, you could change the tracking_id, but that
implies that a touch was lifted and another was placed. This is also
incorrect behavior.

We need to tell userspace that this is a messed up device that can't
accurately track touch locations across touch up/down boundaries. Once
userspace sees this, it can act appropriately when it sees a transition
from BTN_TOOL_DOUBLETAP to BTN_TOOL_TRIPLETAP, for example. This is how
we handle transitions in the uTouch stack for semi-mt devices.

Perhaps a clean implementation would be to keep semi-mt as a flag
stating that there will only ever be two slots, the 0th will be the
minimum (x,y) of the bounding box and the 1st will be the maximum. Then,
we add a flag like NO_TOUCH_TRANSITION_TRACKING that would be set on
both semi-mt and these new devices that denote the slot data may
transition from one physical touchpoint to another when the number of
touches changes.

We could leave this up to userspace and have it detect a
NO_TOUCH_TRANSITION_TRACKING device based on the fact that the max slots
is less than the max fingers, but I would argue here that a more clear
protocol is a better protocol.

-- Chase

Re: [PATCH 09/12] Input: synaptics - add image sensor support

[Dmitry Torokhov]

On Wed, Jul 06, 2011 at 02:22:53PM -0700, Chase Douglas wrote:
> On 07/06/2011 01:20 PM, Henrik Rydberg wrote:
> >>> To me, it seems we do need a little bit of extra information to
> >>> determine this new type of device.
> >>
> >> I think we already have all we need (see above).
> > 
> > I concur. So, to conclude:
> > 
> > a) The improved synaptics behavior can be achieved by simply using
> > MT-B plus BTN_TOOL_*.
> > 
> > b) Userspace should check BTN_TOOL_* for any discrepancies between the
> > maximum number of available slots (always two in this case) and the
> > maximum number of fingers reported (BTN_TOOL_TRIPLETAP etc). Extra
> > actions may then be taken to support more fingers than slots.
> > 
> > c) The semi-mt flag is only used to signal that the two points sent
> > via MT-B are the corners of a bounding box.
> 
> This isn't quite enough. If we don't set the semi-mt flag or any other
> new flag, then we'll have slots that become inconsistent when touches
> are added or removed. For example, start with two touches being tracked
> correctly. Now, add a touch. The second slot will now get the data of
> the third touch, which is in a different location. You haven't changed
> the tracking_id though, so it looks like the same touch. This is
> incorrect behavior. Or, you could change the tracking_id, but that
> implies that a touch was lifted and another was placed. This is also
> incorrect behavior.

The tracking ID needs to be changed as we start trackign and reporting
new touch. We could see that the old touch was not removed from the fact
that total number of finger reported increased.

> 
> We need to tell userspace that this is a messed up device that can't
> accurately track touch locations across touch up/down boundaries. Once
> userspace sees this, it can act appropriately when it sees a transition
> from BTN_TOOL_DOUBLETAP to BTN_TOOL_TRIPLETAP, for example. This is how
> we handle transitions in the uTouch stack for semi-mt devices.
> 
> Perhaps a clean implementation would be to keep semi-mt as a flag
> stating that there will only ever be two slots, the 0th will be the
> minimum (x,y) of the bounding box and the 1st will be the maximum. Then,
> we add a flag like NO_TOUCH_TRANSITION_TRACKING that would be set on
> both semi-mt and these new devices that denote the slot data may
> transition from one physical touchpoint to another when the number of
> touches changes.
> 
> We could leave this up to userspace and have it detect a
> NO_TOUCH_TRANSITION_TRACKING device based on the fact that the max slots
> is less than the max fingers, but I would argue here that a more clear
> protocol is a better protocol.

I'll ask this - how much realistically do we care about 3+ finger
transitions in context of these particular devices? This is a touchpad
so as long as basic 2 finger gestures work (zoom, pinch, 2-finger
scroll) with Synaptics X driver we should be fine. I do not want to add
all kinds of custom flags to the protocol to deal with this generation
of touchpads.

It sounds to me like latest generation of Synaptocs protocol is a dud
and hopefully they will fix it to something more flexible in the next
generationof chips...

Thanks.

-- 
Dmitry

Re: [PATCH 09/12] Input: synaptics - add image sensor support

[Chase Douglas]

On 07/06/2011 02:36 PM, Dmitry Torokhov wrote:
> On Wed, Jul 06, 2011 at 02:22:53PM -0700, Chase Douglas wrote:
>> On 07/06/2011 01:20 PM, Henrik Rydberg wrote:
>>>>> To me, it seems we do need a little bit of extra information to
>>>>> determine this new type of device.
>>>>
>>>> I think we already have all we need (see above).
>>>
>>> I concur. So, to conclude:
>>>
>>> a) The improved synaptics behavior can be achieved by simply using
>>> MT-B plus BTN_TOOL_*.
>>>
>>> b) Userspace should check BTN_TOOL_* for any discrepancies between the
>>> maximum number of available slots (always two in this case) and the
>>> maximum number of fingers reported (BTN_TOOL_TRIPLETAP etc). Extra
>>> actions may then be taken to support more fingers than slots.
>>>
>>> c) The semi-mt flag is only used to signal that the two points sent
>>> via MT-B are the corners of a bounding box.
>>
>> This isn't quite enough. If we don't set the semi-mt flag or any other
>> new flag, then we'll have slots that become inconsistent when touches
>> are added or removed. For example, start with two touches being tracked
>> correctly. Now, add a touch. The second slot will now get the data of
>> the third touch, which is in a different location. You haven't changed
>> the tracking_id though, so it looks like the same touch. This is
>> incorrect behavior. Or, you could change the tracking_id, but that
>> implies that a touch was lifted and another was placed. This is also
>> incorrect behavior.
> 
> The tracking ID needs to be changed as we start trackign and reporting
> new touch. We could see that the old touch was not removed from the fact
> that total number of finger reported increased.

The evdev protocol is discrete, not continuous, so it's theoretically
possible that one touch could end at the same time that another begins.
IIRC, the time resolution of some MT devices makes this completely
possible, perhaps to the point that it would be trival to make this
happen in a few seconds of trying.

I think the MT-B protocol has only ever used tracking_id for signalling
touch begin and touch end, where the id goes from -1 to something else
and vice versa. Maybe the protocol could be "extended" by saying that a
transition from one valid id to another means there is inconsistent
touch state, but the old touch hasn't necessarily ended and the new
touch hasn't necessarily started at this point in time.

I'm not sure this is any easier than flagging these as bad devices
because now we need to watch tracking ID changes on top of touch count
changes. From someone who has attempted to implement semi-mt in X
synaptics, adding more complexity here should be avoided at all cost :).

>> We need to tell userspace that this is a messed up device that can't
>> accurately track touch locations across touch up/down boundaries. Once
>> userspace sees this, it can act appropriately when it sees a transition
>> from BTN_TOOL_DOUBLETAP to BTN_TOOL_TRIPLETAP, for example. This is how
>> we handle transitions in the uTouch stack for semi-mt devices.
>>
>> Perhaps a clean implementation would be to keep semi-mt as a flag
>> stating that there will only ever be two slots, the 0th will be the
>> minimum (x,y) of the bounding box and the 1st will be the maximum. Then,
>> we add a flag like NO_TOUCH_TRANSITION_TRACKING that would be set on
>> both semi-mt and these new devices that denote the slot data may
>> transition from one physical touchpoint to another when the number of
>> touches changes.
>>
>> We could leave this up to userspace and have it detect a
>> NO_TOUCH_TRANSITION_TRACKING device based on the fact that the max slots
>> is less than the max fingers, but I would argue here that a more clear
>> protocol is a better protocol.
> 
> I'll ask this - how much realistically do we care about 3+ finger
> transitions in context of these particular devices? This is a touchpad
> so as long as basic 2 finger gestures work (zoom, pinch, 2-finger
> scroll) with Synaptics X driver we should be fine. I do not want to add
> all kinds of custom flags to the protocol to deal with this generation
> of touchpads.

I've given up on trying to send semi-mt data through the proposed XInput
2.1 multitouch protocol. I think the best option is to send all this
data as valuators of a single touch (a touch and not a traditional
pointer event due to the multitouch/gesture semantics). Thus, we should
be focusing on what is possible in the gesture realm since we have
thrown full multiouch out the window for these devices.

With these devices we can support one touch drag, two touch pinch,
rotate, and zoom, and 1-5 touch tap. For these to work, we need to know
the number of touches at any given time, the locations of the two
touches when only two touches are active, and some representative
location for the 1 and 3-5 touch cases.

I am sitting here writing possible solutions trying to come up with sane
ways to handle all this, but every 5 minutes I erase what I came up with
and start over because you only see the problems once you've analysed
every scenario. I can't see any way to cater for these devices aside
from: handle them as single touch because they suck, or something
similar to what has been described in the past few hours.

> It sounds to me like latest generation of Synaptocs protocol is a dud
> and hopefully they will fix it to something more flexible in the next
> generationof chips...

We can only hope. In the meantime, it looks like Google is pushing to
use these devices on reference designs for Chrome OS, and big vendors
like Dell are perfectly happy to ship Ubuntu with the 100 times worse
(because we don't know their protocol) ALPS devices. Waiting for sanity
to win out seems like a lost cause to me :(.

-- Chase

Re: [PATCH 09/12] Input: synaptics - add image sensor support

[Henrik Rydberg]

> The evdev protocol is discrete, not continuous, so it's theoretically
> possible that one touch could end at the same time that another begins.
> IIRC, the time resolution of some MT devices makes this completely
> possible, perhaps to the point that it would be trival to make this
> happen in a few seconds of trying.
> 
> I think the MT-B protocol has only ever used tracking_id for signalling
> touch begin and touch end, where the id goes from -1 to something else
> and vice versa. Maybe the protocol could be "extended" by saying that a
> transition from one valid id to another means there is inconsistent
> touch state, but the old touch hasn't necessarily ended and the new
> touch hasn't necessarily started at this point in time.

The in-flight change of tracking id is actually part of the design; it
makes the protocol independent of sample rate. If a particular
tracking id is no longer found in any slot, that touch has ended.

> I'm not sure this is any easier than flagging these as bad devices
> because now we need to watch tracking ID changes on top of touch count
> changes. From someone who has attempted to implement semi-mt in X
> synaptics, adding more complexity here should be avoided at all cost :).

The information available in the proposition suffices to determine
what the device is. Surely the method of transferring that information
will not have any impact on the extra code required.

> > I'll ask this - how much realistically do we care about 3+ finger
> > transitions in context of these particular devices? This is a touchpad
> > so as long as basic 2 finger gestures work (zoom, pinch, 2-finger
> > scroll) with Synaptics X driver we should be fine. I do not want to add
> > all kinds of custom flags to the protocol to deal with this generation
> > of touchpads.
> 
> I've given up on trying to send semi-mt data through the proposed XInput
> 2.1 multitouch protocol. I think the best option is to send all this
> data as valuators of a single touch (a touch and not a traditional
> pointer event due to the multitouch/gesture semantics). Thus, we should
> be focusing on what is possible in the gesture realm since we have
> thrown full multiouch out the window for these devices.
> 
> With these devices we can support one touch drag, two touch pinch,
> rotate, and zoom, and 1-5 touch tap. For these to work, we need to know
> the number of touches at any given time, the locations of the two
> touches when only two touches are active, and some representative
> location for the 1 and 3-5 touch cases.

Right, and we do, so there is no problem there, is there?

> I am sitting here writing possible solutions trying to come up with sane
> ways to handle all this, but every 5 minutes I erase what I came up with
> and start over because you only see the problems once you've analysed
> every scenario. I can't see any way to cater for these devices aside
> from: handle them as single touch because they suck, or something
> similar to what has been described in the past few hours.
> 
> > It sounds to me like latest generation of Synaptocs protocol is a dud
> > and hopefully they will fix it to something more flexible in the next
> > generationof chips...
> 
> We can only hope. In the meantime, it looks like Google is pushing to
> use these devices on reference designs for Chrome OS, and big vendors
> like Dell are perfectly happy to ship Ubuntu with the 100 times worse
> (because we don't know their protocol) ALPS devices. Waiting for sanity
> to win out seems like a lost cause to me :(.

Let us bide our time and see.

Cheers,
Henrik

Re: [PATCH 09/12] Input: synaptics - add image sensor support

[Chase Douglas]

On 07/06/2011 03:35 PM, Henrik Rydberg wrote:
>> The evdev protocol is discrete, not continuous, so it's theoretically
>> possible that one touch could end at the same time that another begins.
>> IIRC, the time resolution of some MT devices makes this completely
>> possible, perhaps to the point that it would be trival to make this
>> happen in a few seconds of trying.
>>
>> I think the MT-B protocol has only ever used tracking_id for signalling
>> touch begin and touch end, where the id goes from -1 to something else
>> and vice versa. Maybe the protocol could be "extended" by saying that a
>> transition from one valid id to another means there is inconsistent
>> touch state, but the old touch hasn't necessarily ended and the new
>> touch hasn't necessarily started at this point in time.
> 
> The in-flight change of tracking id is actually part of the design; it
> makes the protocol independent of sample rate. If a particular
> tracking id is no longer found in any slot, that touch has ended.

Ok. I agree that this is the best way to handle things.

It does mean that we can't just change tracking_ids when the number of
touches changes, so that option is out.

>> I'm not sure this is any easier than flagging these as bad devices
>> because now we need to watch tracking ID changes on top of touch count
>> changes. From someone who has attempted to implement semi-mt in X
>> synaptics, adding more complexity here should be avoided at all cost :).
> 
> The information available in the proposition suffices to determine
> what the device is. Surely the method of transferring that information
> will not have any impact on the extra code required.

My (unstated) question was: is watching tracking_id transitions
reasonable for noting inconsistent touch changes when the number of
touches changes, or should we have a way to tell if a device is broken
and we must assume touch state inconsistency when the finger count
changes. Given your reply above that we cannot repurpose valid
tracking_id -> valid tracking_id transitions as a means of denoting
touch state inconsistency, that leaves us with the latter option.

You're right that the information available from the axes is enough to
tell if the device is broken (i.e. if max finger count > max slot count).

Ancillary to this is the question of whether we should depend on the
userspace side knowing this hidden meaning, or if we should state it up
front by setting a separate flag in the properties bits. I really don't
want to leave this up to userspace developers having to figure out or
dive into kernel code to determine what is going on, so I'm in favor of
having an explicit (documented!) property flag for these devices. Why
have property bits if you aren't going to use them :).

>>> I'll ask this - how much realistically do we care about 3+ finger
>>> transitions in context of these particular devices? This is a touchpad
>>> so as long as basic 2 finger gestures work (zoom, pinch, 2-finger
>>> scroll) with Synaptics X driver we should be fine. I do not want to add
>>> all kinds of custom flags to the protocol to deal with this generation
>>> of touchpads.
>>
>> I've given up on trying to send semi-mt data through the proposed XInput
>> 2.1 multitouch protocol. I think the best option is to send all this
>> data as valuators of a single touch (a touch and not a traditional
>> pointer event due to the multitouch/gesture semantics). Thus, we should
>> be focusing on what is possible in the gesture realm since we have
>> thrown full multiouch out the window for these devices.
>>
>> With these devices we can support one touch drag, two touch pinch,
>> rotate, and zoom, and 1-5 touch tap. For these to work, we need to know
>> the number of touches at any given time, the locations of the two
>> touches when only two touches are active, and some representative
>> location for the 1 and 3-5 touch cases.
> 
> Right, and we do, so there is no problem there, is there?

Agreed, there is no problem if we go with the assumption that for these
devices the touch slot data is inconsistent on touch count changes.

I am satisfied with the approach that's been laid out, the only point of
contention is whether userspace must infer touch state inconsistency on
touch count changes by looking at max fingers and max slots, or if we
provide this detail as a device property.

-- Chase

[PATCH 10/12] Input: synaptics - decode AGM packet types

[djkurtz]

From: Daniel Kurtz <djkurtz@chromium.org>

A Synaptics image sensor tracks 5 fingers, but can only report 2.
This behavior is called "T5R2" = Track 5 Report 2

Algorithm for choosing which 2 fingers to report in which packet:
  Touchpad maintains 5 slots, numbered 0 to 4.
  Initially all slots are empty.
  As new fingers are detected, they are assigned the lowest available
  slot.
  Touchpad always reports:
    SGM: lowest numbered non-empty slot
    AGM: highest numbered non-empty slot, if there is one.

In addition, T5R2 touchpads have a special AGM packet type which reports
the number of fingers currently being tracked, and which finger is in
each of the two slots.  Unfortunately, these "TYPE=2" packets are only used
when more than 3 fingers are being tracked.  When less than 4 fingers
are present, the 'w' value must be used to track how many fingers are
present, and knowing which fingers are being reported is much more
difficult, if not impossible.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/mouse/synaptics.c |   39 ++++++++++++++++++++++++++++++++++-----
 drivers/input/mouse/synaptics.h |    7 ++++++-
 include/linux/input.h           |    1 +
 3 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 2d7ac0a..19a9b7f 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -401,6 +401,14 @@ static void synaptics_pt_create(struct psmouse *psmouse)
 /*****************************************************************************
  *	Functions to interpret the absolute mode packets
  ****************************************************************************/
+/* Set AGM-CONTACT finger state */
+static void synaptics_agm_finger_update(struct synaptics_data *priv, int count,
+					int sgm, int agm)
+{
+	priv->agm.finger_count = count;
+	priv->agm.finger_sgm = sgm;
+	priv->agm.finger_agm = agm;
+}
 
 static int synaptics_parse_hw_state(const unsigned char buf[],
 				    struct synaptics_data *priv,
@@ -438,11 +446,31 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
 		if ((SYN_CAP_ADV_GESTURE(priv->ext_cap_0c)
 				|| SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c))
 				&& hw->w == 2) {
-			/* Gesture packet: (x, y, z) at half resolution */
-			priv->agm.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
-			priv->agm.y = INVERT_Y((((buf[4] & 0xf0) << 4)
-					      | buf[2]) << 1);
-			priv->agm.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
+			int type; /* Packet type */
+
+			type = (buf[5] & 0x30) >> 4;
+
+			switch (type) {
+			case 1:
+				/* Gesture packet: (x, y, z) half resolution */
+				priv->agm.w = hw->w;
+				priv->agm.x = (((buf[4] & 0x0f) << 8)
+						| buf[1]) << 1;
+				priv->agm.y = INVERT_Y((((buf[4] & 0xf0) << 4)
+						       | buf[2]) << 1);
+				priv->agm.z = ((buf[3] & 0x30)
+						| (buf[5] & 0x0f)) << 1;
+				break;
+
+			case 2:
+				/* Finger slot update */
+				synaptics_agm_finger_update(priv, buf[1],
+							    buf[2], buf[4]);
+				break;
+
+			default:
+				break;
+			}
 			return 1;
 		} else {
 			hw->x = (((buf[3] & 0x10) << 8) |
@@ -804,6 +832,7 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
 	input_set_abs_params(dev, ABS_PRESSURE, 0, 255, 0, 0);
 
 	if (SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c)) {
+		__set_bit(INPUT_PROP_SYNAPTICS_T5R2, dev->propbit);
 		input_mt_init_slots(dev, SYN_TRACK_SLOT_COUNT);
 		input_set_abs_params(dev, ABS_MT_POSITION_X, XMIN_NOMINAL,
 				     priv->x_max ?: XMAX_NOMINAL, fuzz, 0);
diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
index 1de2256..2214af6 100644
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -122,7 +122,7 @@
 #define SYN_SLOT_AGM			2
 
 /* number of tracking slots for Image Sensor firmware */
-#define SYN_TRACK_SLOT_COUNT		2
+#define SYN_TRACK_SLOT_COUNT		5
 
 /*
  * A structure to describe the state of the touchpad hardware (buttons and pad)
@@ -140,6 +140,11 @@ struct synaptics_hw_state {
 	unsigned int down:1;
 	unsigned char ext_buttons;
 	signed char scroll;
+
+	/* Reported in AGM-CONTACT packets */
+	unsigned int finger_count;		/* num fingers being tracked */
+	unsigned int finger_sgm;		/* finger described by sgm */
+	unsigned int finger_agm;		/* finger described by agm */
 };
 
 struct synaptics_data {
diff --git a/include/linux/input.h b/include/linux/input.h
index 771d6d8..732c14e 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -137,6 +137,7 @@ struct input_keymap_entry {
 #define INPUT_PROP_DIRECT		0x01	/* direct input devices */
 #define INPUT_PROP_BUTTONPAD		0x02	/* has button(s) under pad */
 #define INPUT_PROP_SEMI_MT		0x03	/* touch rectangle only */
+#define INPUT_PROP_SYNAPTICS_T5R2	0x04	/* synaptics track 5 report 2 */
 
 #define INPUT_PROP_MAX			0x1f
 #define INPUT_PROP_CNT			(INPUT_PROP_MAX + 1)
-- 
1.7.3.1

Re: [PATCH 10/12] Input: synaptics - decode AGM packet types

[Chase Douglas]

On 06/29/2011 06:07 AM, djkurtz@chromium.org wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
> 
> A Synaptics image sensor tracks 5 fingers, but can only report 2.
> This behavior is called "T5R2" = Track 5 Report 2
> 
> Algorithm for choosing which 2 fingers to report in which packet:
>   Touchpad maintains 5 slots, numbered 0 to 4.
>   Initially all slots are empty.
>   As new fingers are detected, they are assigned the lowest available
>   slot.
>   Touchpad always reports:
>     SGM: lowest numbered non-empty slot
>     AGM: highest numbered non-empty slot, if there is one.

Hi Daniel,

Thanks for the new patches! I will review them soon in detail, but I
would like to first confirm some details about the above.

In the older "profile" devices, we got essentially the bounding box of
all touches. This allows us to detect pinch gestures between the two
most extreme touches when there are three touches on the touchpad.

The above description makes it sound like we will no longer have a
bounding box of touches. According to the description, if I put four
fingers down in a square formation and then touch a fifth finger in the
middle of the square, I will only see the locations of one corner and
the middle of the square. This would make meaningful gesture detection
beyond two touches nearly impossible. Is this really how the touchpad
works, or is the above description not quite right?

Thanks!

-- Chase

Re: [PATCH 10/12] Input: synaptics - decode AGM packet types

[Daniel Stone]

Hi Chase,

On Wed, Jun 29, 2011 at 11:02:53AM +0100, Chase Douglas wrote:
> In the older "profile" devices, we got essentially the bounding box of
> all touches. This allows us to detect pinch gestures between the two
> most extreme touches when there are three touches on the touchpad.

This isn't necessarily true for all pads.  If you look at Derek's
patchset from a couple of weeks ago, it was for a profile sensor which
had the same reporting behaviour.  Internally, the touchpad could track
many fingers, but would always report the first (in SGM packet) and most
recent (in AGM packet) fingers.  You could fake a bounding-box
calculation, but it would be the bounding box of fingers 0 and n, rather
than the bounding box of all fingers.

> The above description makes it sound like we will no longer have a
> bounding box of touches. According to the description, if I put four
> fingers down in a square formation and then touch a fifth finger in the
> middle of the square, I will only see the locations of one corner and
> the middle of the square. This would make meaningful gesture detection
> beyond two touches nearly impossible. Is this really how the touchpad
> works, or is the above description not quite right?

Yes, I believe that's how the touchpad works.  It would be nice to get
all fingers, but unfortunately PS/2 doesn't give us enough bandwidth to
do that.

Cheers,
Daniel

Re: [PATCH 10/12] Input: synaptics - decode AGM packet types

[Chase Douglas]

On 06/29/2011 11:07 AM, Daniel Stone wrote:
> Hi Chase,
> 
> On Wed, Jun 29, 2011 at 11:02:53AM +0100, Chase Douglas wrote:
>> In the older "profile" devices, we got essentially the bounding box of
>> all touches. This allows us to detect pinch gestures between the two
>> most extreme touches when there are three touches on the touchpad.
> 
> This isn't necessarily true for all pads.  If you look at Derek's
> patchset from a couple of weeks ago, it was for a profile sensor which
> had the same reporting behaviour.  Internally, the touchpad could track
> many fingers, but would always report the first (in SGM packet) and most
> recent (in AGM packet) fingers.  You could fake a bounding-box
> calculation, but it would be the bounding box of fingers 0 and n, rather
> than the bounding box of all fingers.

Are we really sure that Derek's trackpad was a profile device? I mean
*really* sure. Because if Synaptics is putting out two different devices
that are identical in protocol but behave differently, then that's a
*big* problem for us gesture developers :).

I would be surprised if a profile device were providing the first and
third touch locations as opposed to the bounding box because then
there's no difference between it and this new "image sensor" device type.

>> The above description makes it sound like we will no longer have a
>> bounding box of touches. According to the description, if I put four
>> fingers down in a square formation and then touch a fifth finger in the
>> middle of the square, I will only see the locations of one corner and
>> the middle of the square. This would make meaningful gesture detection
>> beyond two touches nearly impossible. Is this really how the touchpad
>> works, or is the above description not quite right?
> 
> Yes, I believe that's how the touchpad works.  It would be nice to get
> all fingers, but unfortunately PS/2 doesn't give us enough bandwidth to
> do that.

If this is true for these new image devices, then we'll need to set a
different property bit other than SEMI_MT (maybe FIRST_LAST_MT? I'm not
good with names :). I'm thinking we should disable gesture support for
these trackpads in uTouch when there are more than two touches because
we can't be sure what is going on in any meaningful way. The bounding
box is more useful than this, which is really sad.

-- Chase

Re: [PATCH 10/12] Input: synaptics - decode AGM packet types

[Daniel Kurtz]

Hi Daniel, Chase,

On Wed, Jun 29, 2011 at 6:32 PM, Chase Douglas
<chase.douglas@canonical.com> wrote:
> On 06/29/2011 11:07 AM, Daniel Stone wrote:
>> Hi Chase,
>>
>> On Wed, Jun 29, 2011 at 11:02:53AM +0100, Chase Douglas wrote:
>>> In the older "profile" devices, we got essentially the bounding box of
>>> all touches. This allows us to detect pinch gestures between the two
>>> most extreme touches when there are three touches on the touchpad.
>>
>> This isn't necessarily true for all pads.  If you look at Derek's
>> patchset from a couple of weeks ago, it was for a profile sensor which
>> had the same reporting behaviour.  Internally, the touchpad could track
>> many fingers, but would always report the first (in SGM packet) and most
>> recent (in AGM packet) fingers.  You could fake a bounding-box
>> calculation, but it would be the bounding box of fingers 0 and n, rather
>> than the bounding box of all fingers.
>
> Are we really sure that Derek's trackpad was a profile device? I mean
> *really* sure. Because if Synaptics is putting out two different devices
> that are identical in protocol but behave differently, then that's a
> *big* problem for us gesture developers :).
>
> I would be surprised if a profile device were providing the first and
> third touch locations as opposed to the bounding box because then
> there's no difference between it and this new "image sensor" device type.

I agree with Chase, the 0:n box behavior sounds more like image sensor
than profile sensor.
However, as I just mentioned in the previous email, I have looked at a
profile sensor that tries to report first and second finger positions,
not a bounding box.
In any case, the result is:
You end up computing (usually) a 0:1 "bounding box", with n >= 2
fingers on the pad.
I say usually because it is pretty easy to confuse the finger tracking
of a profile sensor.

>>> The above description makes it sound like we will no longer have a
>>> bounding box of touches. According to the description, if I put four
>>> fingers down in a square formation and then touch a fifth finger in the
>>> middle of the square, I will only see the locations of one corner and
>>> the middle of the square. This would make meaningful gesture detection
>>> beyond two touches nearly impossible. Is this really how the touchpad
>>> works, or is the above description not quite right?
>>
>> Yes, I believe that's how the touchpad works.  It would be nice to get
>> all fingers, but unfortunately PS/2 doesn't give us enough bandwidth to
>> do that.
>
> If this is true for these new image devices, then we'll need to set a
> different property bit other than SEMI_MT (maybe FIRST_LAST_MT? I'm not
> good with names :). I'm thinking we should disable gesture support for
> these trackpads in uTouch when there are more than two touches because
> we can't be sure what is going on in any meaningful way. The bounding
> box is more useful than this, which is really sad.
>
> -- Chase

This is exactly what this patch set does by defining the T5R2 property.

It is very challenging to support any 3+ finger gestures with a
touchpad that only reports two fingers... whether it is semi-mt or
T5R2.
You must always make an assumption about where those hidden fingers are.
But, that's a problem for userspace... :)

-Daniel

Re: [PATCH 10/12] Input: synaptics - decode AGM packet types

[Daniel Kurtz]

Hi Chase,

Thanks for looking at the patches!

On Wed, Jun 29, 2011 at 6:02 PM, Chase Douglas
<chase.douglas@canonical.com> wrote:
>
> On 06/29/2011 06:07 AM, djkurtz@chromium.org wrote:
> > From: Daniel Kurtz <djkurtz@chromium.org>
> >
> > A Synaptics image sensor tracks 5 fingers, but can only report 2.
> > This behavior is called "T5R2" = Track 5 Report 2
> >
> > Algorithm for choosing which 2 fingers to report in which packet:
> >   Touchpad maintains 5 slots, numbered 0 to 4.
> >   Initially all slots are empty.
> >   As new fingers are detected, they are assigned the lowest available
> >   slot.
> >   Touchpad always reports:
> >     SGM: lowest numbered non-empty slot
> >     AGM: highest numbered non-empty slot, if there is one.
>
> Hi Daniel,
>
> Thanks for the new patches! I will review them soon in detail, but I
> would like to first confirm some details about the above.
>
> In the older "profile" devices, we got essentially the bounding box of
> all touches. This allows us to detect pinch gestures between the two
> most extreme touches when there are three touches on the touchpad.
>
> The above description makes it sound like we will no longer have a
> bounding box of touches. According to the description, if I put four
> fingers down in a square formation and then touch a fifth finger in the
> middle of the square, I will only see the locations of one corner and
> the middle of the square. This would make meaningful gesture detection
> beyond two touches nearly impossible. Is this really how the touchpad
> works, or is the above description not quite right?

This is how the image sensor works, from my observations.

As for profile sensors the kernel driver creates a "best guess
bounding box" by picking:
   top_left = (min(x1,x2), min(y1,y2))
   bottom_right = (max(x1,x2), max(y1,y2))

However, at least from my experiments with one profile sensor, there
is no guarantee that this rectangle actually contains all touches on
the pad.
In fact, I think it is trying really hard to return the position of
the first two touches that it is tracking.
When there are two fingers, it returns the finger with the larger
y-value (and what the trackpad believes to be its x-), and the SGM
packet, and the other finger in the AGM.
When there are three or more fingers, it still reports the first two,
irregardless of where the new fingers are added.

It would be very curious if different profile sensors behaved differently.

In any case, this patch should not change the behavior of profile
sensors, since it only applies to image sensors.

Thanks,
-Daniel

Re: [PATCH 10/12] Input: synaptics - decode AGM packet types

[Chase Douglas]

On 06/28/2011 10:07 PM, djkurtz@chromium.org wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
> 
> A Synaptics image sensor tracks 5 fingers, but can only report 2.
> This behavior is called "T5R2" = Track 5 Report 2
> 
> Algorithm for choosing which 2 fingers to report in which packet:
>   Touchpad maintains 5 slots, numbered 0 to 4.
>   Initially all slots are empty.
>   As new fingers are detected, they are assigned the lowest available
>   slot.
>   Touchpad always reports:
>     SGM: lowest numbered non-empty slot
>     AGM: highest numbered non-empty slot, if there is one.
> 
> In addition, T5R2 touchpads have a special AGM packet type which reports
> the number of fingers currently being tracked, and which finger is in
> each of the two slots.  Unfortunately, these "TYPE=2" packets are only used
> when more than 3 fingers are being tracked.  When less than 4 fingers
> are present, the 'w' value must be used to track how many fingers are
> present, and knowing which fingers are being reported is much more
> difficult, if not impossible.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/input/mouse/synaptics.c |   39 ++++++++++++++++++++++++++++++++++-----
>  drivers/input/mouse/synaptics.h |    7 ++++++-
>  include/linux/input.h           |    1 +
>  3 files changed, 41 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 2d7ac0a..19a9b7f 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -401,6 +401,14 @@ static void synaptics_pt_create(struct psmouse *psmouse)
>  /*****************************************************************************
>   *	Functions to interpret the absolute mode packets
>   ****************************************************************************/
> +/* Set AGM-CONTACT finger state */
> +static void synaptics_agm_finger_update(struct synaptics_data *priv, int count,
> +					int sgm, int agm)
> +{
> +	priv->agm.finger_count = count;
> +	priv->agm.finger_sgm = sgm;
> +	priv->agm.finger_agm = agm;
> +}
>  
>  static int synaptics_parse_hw_state(const unsigned char buf[],
>  				    struct synaptics_data *priv,
> @@ -438,11 +446,31 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
>  		if ((SYN_CAP_ADV_GESTURE(priv->ext_cap_0c)
>  				|| SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c))
>  				&& hw->w == 2) {
> -			/* Gesture packet: (x, y, z) at half resolution */
> -			priv->agm.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
> -			priv->agm.y = INVERT_Y((((buf[4] & 0xf0) << 4)
> -					      | buf[2]) << 1);
> -			priv->agm.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
> +			int type; /* Packet type */
> +
> +			type = (buf[5] & 0x30) >> 4;
> +
> +			switch (type) {
> +			case 1:
> +				/* Gesture packet: (x, y, z) half resolution */
> +				priv->agm.w = hw->w;
> +				priv->agm.x = (((buf[4] & 0x0f) << 8)
> +						| buf[1]) << 1;
> +				priv->agm.y = INVERT_Y((((buf[4] & 0xf0) << 4)
> +						       | buf[2]) << 1);
> +				priv->agm.z = ((buf[3] & 0x30)
> +						| (buf[5] & 0x0f)) << 1;
> +				break;
> +
> +			case 2:
> +				/* Finger slot update */
> +				synaptics_agm_finger_update(priv, buf[1],
> +							    buf[2], buf[4]);
> +				break;
> +
> +			default:
> +				break;
> +			}
>  			return 1;
>  		} else {
>  			hw->x = (((buf[3] & 0x10) << 8) |
> @@ -804,6 +832,7 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
>  	input_set_abs_params(dev, ABS_PRESSURE, 0, 255, 0, 0);
>  
>  	if (SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c)) {
> +		__set_bit(INPUT_PROP_SYNAPTICS_T5R2, dev->propbit);
>  		input_mt_init_slots(dev, SYN_TRACK_SLOT_COUNT);
>  		input_set_abs_params(dev, ABS_MT_POSITION_X, XMIN_NOMINAL,
>  				     priv->x_max ?: XMAX_NOMINAL, fuzz, 0);
> diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
> index 1de2256..2214af6 100644
> --- a/drivers/input/mouse/synaptics.h
> +++ b/drivers/input/mouse/synaptics.h
> @@ -122,7 +122,7 @@
>  #define SYN_SLOT_AGM			2
>  
>  /* number of tracking slots for Image Sensor firmware */
> -#define SYN_TRACK_SLOT_COUNT		2
> +#define SYN_TRACK_SLOT_COUNT		5
>  
>  /*
>   * A structure to describe the state of the touchpad hardware (buttons and pad)
> @@ -140,6 +140,11 @@ struct synaptics_hw_state {
>  	unsigned int down:1;
>  	unsigned char ext_buttons;
>  	signed char scroll;
> +
> +	/* Reported in AGM-CONTACT packets */
> +	unsigned int finger_count;		/* num fingers being tracked */
> +	unsigned int finger_sgm;		/* finger described by sgm */
> +	unsigned int finger_agm;		/* finger described by agm */
>  };
>  
>  struct synaptics_data {
> diff --git a/include/linux/input.h b/include/linux/input.h
> index 771d6d8..732c14e 100644
> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -137,6 +137,7 @@ struct input_keymap_entry {
>  #define INPUT_PROP_DIRECT		0x01	/* direct input devices */
>  #define INPUT_PROP_BUTTONPAD		0x02	/* has button(s) under pad */
>  #define INPUT_PROP_SEMI_MT		0x03	/* touch rectangle only */
> +#define INPUT_PROP_SYNAPTICS_T5R2	0x04	/* synaptics track 5 report 2 */
>  
>  #define INPUT_PROP_MAX			0x1f
>  #define INPUT_PROP_CNT			(INPUT_PROP_MAX + 1)

I'm trying to understand these later patches for T5R2. There are
"hidden" touch slots now. How does userspace tell whether a touch slot
is a hidden touch?

-- Chase

Re: [PATCH 10/12] Input: synaptics - decode AGM packet types

[Chris Bagwell]

On Tue, Jul 5, 2011 at 1:17 PM, Chase Douglas
<chase.douglas@canonical.com> wrote:
> On 06/28/2011 10:07 PM, djkurtz@chromium.org wrote:
>> From: Daniel Kurtz <djkurtz@chromium.org>
>>
>> A Synaptics image sensor tracks 5 fingers, but can only report 2.
>> This behavior is called "T5R2" = Track 5 Report 2
>>
>> Algorithm for choosing which 2 fingers to report in which packet:
>>   Touchpad maintains 5 slots, numbered 0 to 4.
>>   Initially all slots are empty.
>>   As new fingers are detected, they are assigned the lowest available
>>   slot.
>>   Touchpad always reports:
>>     SGM: lowest numbered non-empty slot
>>     AGM: highest numbered non-empty slot, if there is one.
>>
>> In addition, T5R2 touchpads have a special AGM packet type which reports
>> the number of fingers currently being tracked, and which finger is in
>> each of the two slots.  Unfortunately, these "TYPE=2" packets are only used
>> when more than 3 fingers are being tracked.  When less than 4 fingers
>> are present, the 'w' value must be used to track how many fingers are
>> present, and knowing which fingers are being reported is much more
>> difficult, if not impossible.
>>
>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>> ---
>>  drivers/input/mouse/synaptics.c |   39 ++++++++++++++++++++++++++++++++++-----
>>  drivers/input/mouse/synaptics.h |    7 ++++++-
>>  include/linux/input.h           |    1 +
>>  3 files changed, 41 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>> index 2d7ac0a..19a9b7f 100644
>> --- a/drivers/input/mouse/synaptics.c
>> +++ b/drivers/input/mouse/synaptics.c
>> @@ -401,6 +401,14 @@ static void synaptics_pt_create(struct psmouse *psmouse)
>>  /*****************************************************************************
>>   *   Functions to interpret the absolute mode packets
>>   ****************************************************************************/
>> +/* Set AGM-CONTACT finger state */
>> +static void synaptics_agm_finger_update(struct synaptics_data *priv, int count,
>> +                                     int sgm, int agm)
>> +{
>> +     priv->agm.finger_count = count;
>> +     priv->agm.finger_sgm = sgm;
>> +     priv->agm.finger_agm = agm;
>> +}
>>
>>  static int synaptics_parse_hw_state(const unsigned char buf[],
>>                                   struct synaptics_data *priv,
>> @@ -438,11 +446,31 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
>>               if ((SYN_CAP_ADV_GESTURE(priv->ext_cap_0c)
>>                               || SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c))
>>                               && hw->w == 2) {
>> -                     /* Gesture packet: (x, y, z) at half resolution */
>> -                     priv->agm.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
>> -                     priv->agm.y = INVERT_Y((((buf[4] & 0xf0) << 4)
>> -                                           | buf[2]) << 1);
>> -                     priv->agm.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
>> +                     int type; /* Packet type */
>> +
>> +                     type = (buf[5] & 0x30) >> 4;
>> +
>> +                     switch (type) {
>> +                     case 1:
>> +                             /* Gesture packet: (x, y, z) half resolution */
>> +                             priv->agm.w = hw->w;
>> +                             priv->agm.x = (((buf[4] & 0x0f) << 8)
>> +                                             | buf[1]) << 1;
>> +                             priv->agm.y = INVERT_Y((((buf[4] & 0xf0) << 4)
>> +                                                    | buf[2]) << 1);
>> +                             priv->agm.z = ((buf[3] & 0x30)
>> +                                             | (buf[5] & 0x0f)) << 1;
>> +                             break;
>> +
>> +                     case 2:
>> +                             /* Finger slot update */
>> +                             synaptics_agm_finger_update(priv, buf[1],
>> +                                                         buf[2], buf[4]);
>> +                             break;
>> +
>> +                     default:
>> +                             break;
>> +                     }
>>                       return 1;
>>               } else {
>>                       hw->x = (((buf[3] & 0x10) << 8) |
>> @@ -804,6 +832,7 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
>>       input_set_abs_params(dev, ABS_PRESSURE, 0, 255, 0, 0);
>>
>>       if (SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c)) {
>> +             __set_bit(INPUT_PROP_SYNAPTICS_T5R2, dev->propbit);
>>               input_mt_init_slots(dev, SYN_TRACK_SLOT_COUNT);
>>               input_set_abs_params(dev, ABS_MT_POSITION_X, XMIN_NOMINAL,
>>                                    priv->x_max ?: XMAX_NOMINAL, fuzz, 0);
>> diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
>> index 1de2256..2214af6 100644
>> --- a/drivers/input/mouse/synaptics.h
>> +++ b/drivers/input/mouse/synaptics.h
>> @@ -122,7 +122,7 @@
>>  #define SYN_SLOT_AGM                 2
>>
>>  /* number of tracking slots for Image Sensor firmware */
>> -#define SYN_TRACK_SLOT_COUNT         2
>> +#define SYN_TRACK_SLOT_COUNT         5
>>
>>  /*
>>   * A structure to describe the state of the touchpad hardware (buttons and pad)
>> @@ -140,6 +140,11 @@ struct synaptics_hw_state {
>>       unsigned int down:1;
>>       unsigned char ext_buttons;
>>       signed char scroll;
>> +
>> +     /* Reported in AGM-CONTACT packets */
>> +     unsigned int finger_count;              /* num fingers being tracked */
>> +     unsigned int finger_sgm;                /* finger described by sgm */
>> +     unsigned int finger_agm;                /* finger described by agm */
>>  };
>>
>>  struct synaptics_data {
>> diff --git a/include/linux/input.h b/include/linux/input.h
>> index 771d6d8..732c14e 100644
>> --- a/include/linux/input.h
>> +++ b/include/linux/input.h
>> @@ -137,6 +137,7 @@ struct input_keymap_entry {
>>  #define INPUT_PROP_DIRECT            0x01    /* direct input devices */
>>  #define INPUT_PROP_BUTTONPAD         0x02    /* has button(s) under pad */
>>  #define INPUT_PROP_SEMI_MT           0x03    /* touch rectangle only */
>> +#define INPUT_PROP_SYNAPTICS_T5R2    0x04    /* synaptics track 5 report 2 */
>>
>>  #define INPUT_PROP_MAX                       0x1f
>>  #define INPUT_PROP_CNT                       (INPUT_PROP_MAX + 1)
>
> I'm trying to understand these later patches for T5R2. There are
> "hidden" touch slots now. How does userspace tell whether a touch slot
> is a hidden touch?
>

I'm trying to understand as well.  If I was a consumer of events and I
knew touchpad supports 2 touch tracking but 5 finger detection, I'd
assume:

* ABS_X/Y reports oldest touch and
BTN_TOOL_DOUBLETAP/TRIPLETAP/QUADTAP reports # of fingers touching
(ignoring lost of 5 finger for now).
* ABS_MT_* only contains 2 slots and without SEMI_MT declared.
BTN_TOOL_* could be used to detect greater than 2 finger touches.

I'm not sure I could guess what to do with 5 slots and how to expect
touch data to come over them.

Where you saying in the other thread that the 5 slots are to help user
detect finger transitions and re-act bounces in X/Y accordingly?  I'm
assuming they could react in same way to BTN_TOOL_* changes if we can
extend to 5 finger case?

Chris

Re: [PATCH 10/12] Input: synaptics - decode AGM packet types

[Daniel Kurtz]

On Wed, Jul 6, 2011 at 2:55 AM, Chris Bagwell <chris@cnpbagwell.com> wrote:
> On Tue, Jul 5, 2011 at 1:17 PM, Chase Douglas
> <chase.douglas@canonical.com> wrote:
>> On 06/28/2011 10:07 PM, djkurtz@chromium.org wrote:
>>> From: Daniel Kurtz <djkurtz@chromium.org>
>>>
>>> A Synaptics image sensor tracks 5 fingers, but can only report 2.
>>> This behavior is called "T5R2" = Track 5 Report 2
>>>
>>> Algorithm for choosing which 2 fingers to report in which packet:
>>>   Touchpad maintains 5 slots, numbered 0 to 4.
>>>   Initially all slots are empty.
>>>   As new fingers are detected, they are assigned the lowest available
>>>   slot.
>>>   Touchpad always reports:
>>>     SGM: lowest numbered non-empty slot
>>>     AGM: highest numbered non-empty slot, if there is one.
>>>
>>> In addition, T5R2 touchpads have a special AGM packet type which reports
>>> the number of fingers currently being tracked, and which finger is in
>>> each of the two slots.  Unfortunately, these "TYPE=2" packets are only used
>>> when more than 3 fingers are being tracked.  When less than 4 fingers
>>> are present, the 'w' value must be used to track how many fingers are
>>> present, and knowing which fingers are being reported is much more
>>> difficult, if not impossible.
>>>
>>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>>> ---
>>>  drivers/input/mouse/synaptics.c |   39 ++++++++++++++++++++++++++++++++++-----
>>>  drivers/input/mouse/synaptics.h |    7 ++++++-
>>>  include/linux/input.h           |    1 +
>>>  3 files changed, 41 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>>> index 2d7ac0a..19a9b7f 100644
>>> --- a/drivers/input/mouse/synaptics.c
>>> +++ b/drivers/input/mouse/synaptics.c
>>> @@ -401,6 +401,14 @@ static void synaptics_pt_create(struct psmouse *psmouse)
>>>  /*****************************************************************************
>>>   *   Functions to interpret the absolute mode packets
>>>   ****************************************************************************/
>>> +/* Set AGM-CONTACT finger state */
>>> +static void synaptics_agm_finger_update(struct synaptics_data *priv, int count,
>>> +                                     int sgm, int agm)
>>> +{
>>> +     priv->agm.finger_count = count;
>>> +     priv->agm.finger_sgm = sgm;
>>> +     priv->agm.finger_agm = agm;
>>> +}
>>>
>>>  static int synaptics_parse_hw_state(const unsigned char buf[],
>>>                                   struct synaptics_data *priv,
>>> @@ -438,11 +446,31 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
>>>               if ((SYN_CAP_ADV_GESTURE(priv->ext_cap_0c)
>>>                               || SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c))
>>>                               && hw->w == 2) {
>>> -                     /* Gesture packet: (x, y, z) at half resolution */
>>> -                     priv->agm.x = (((buf[4] & 0x0f) << 8) | buf[1]) << 1;
>>> -                     priv->agm.y = INVERT_Y((((buf[4] & 0xf0) << 4)
>>> -                                           | buf[2]) << 1);
>>> -                     priv->agm.z = ((buf[3] & 0x30) | (buf[5] & 0x0f)) << 1;
>>> +                     int type; /* Packet type */
>>> +
>>> +                     type = (buf[5] & 0x30) >> 4;
>>> +
>>> +                     switch (type) {
>>> +                     case 1:
>>> +                             /* Gesture packet: (x, y, z) half resolution */
>>> +                             priv->agm.w = hw->w;
>>> +                             priv->agm.x = (((buf[4] & 0x0f) << 8)
>>> +                                             | buf[1]) << 1;
>>> +                             priv->agm.y = INVERT_Y((((buf[4] & 0xf0) << 4)
>>> +                                                    | buf[2]) << 1);
>>> +                             priv->agm.z = ((buf[3] & 0x30)
>>> +                                             | (buf[5] & 0x0f)) << 1;
>>> +                             break;
>>> +
>>> +                     case 2:
>>> +                             /* Finger slot update */
>>> +                             synaptics_agm_finger_update(priv, buf[1],
>>> +                                                         buf[2], buf[4]);
>>> +                             break;
>>> +
>>> +                     default:
>>> +                             break;
>>> +                     }
>>>                       return 1;
>>>               } else {
>>>                       hw->x = (((buf[3] & 0x10) << 8) |
>>> @@ -804,6 +832,7 @@ static void set_input_params(struct input_dev *dev, struct synaptics_data *priv)
>>>       input_set_abs_params(dev, ABS_PRESSURE, 0, 255, 0, 0);
>>>
>>>       if (SYN_CAP_IMAGE_SENSOR(priv->ext_cap_0c)) {
>>> +             __set_bit(INPUT_PROP_SYNAPTICS_T5R2, dev->propbit);
>>>               input_mt_init_slots(dev, SYN_TRACK_SLOT_COUNT);
>>>               input_set_abs_params(dev, ABS_MT_POSITION_X, XMIN_NOMINAL,
>>>                                    priv->x_max ?: XMAX_NOMINAL, fuzz, 0);
>>> diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
>>> index 1de2256..2214af6 100644
>>> --- a/drivers/input/mouse/synaptics.h
>>> +++ b/drivers/input/mouse/synaptics.h
>>> @@ -122,7 +122,7 @@
>>>  #define SYN_SLOT_AGM                 2
>>>
>>>  /* number of tracking slots for Image Sensor firmware */
>>> -#define SYN_TRACK_SLOT_COUNT         2
>>> +#define SYN_TRACK_SLOT_COUNT         5
>>>
>>>  /*
>>>   * A structure to describe the state of the touchpad hardware (buttons and pad)
>>> @@ -140,6 +140,11 @@ struct synaptics_hw_state {
>>>       unsigned int down:1;
>>>       unsigned char ext_buttons;
>>>       signed char scroll;
>>> +
>>> +     /* Reported in AGM-CONTACT packets */
>>> +     unsigned int finger_count;              /* num fingers being tracked */
>>> +     unsigned int finger_sgm;                /* finger described by sgm */
>>> +     unsigned int finger_agm;                /* finger described by agm */
>>>  };
>>>
>>>  struct synaptics_data {
>>> diff --git a/include/linux/input.h b/include/linux/input.h
>>> index 771d6d8..732c14e 100644
>>> --- a/include/linux/input.h
>>> +++ b/include/linux/input.h
>>> @@ -137,6 +137,7 @@ struct input_keymap_entry {
>>>  #define INPUT_PROP_DIRECT            0x01    /* direct input devices */
>>>  #define INPUT_PROP_BUTTONPAD         0x02    /* has button(s) under pad */
>>>  #define INPUT_PROP_SEMI_MT           0x03    /* touch rectangle only */
>>> +#define INPUT_PROP_SYNAPTICS_T5R2    0x04    /* synaptics track 5 report 2 */
>>>
>>>  #define INPUT_PROP_MAX                       0x1f
>>>  #define INPUT_PROP_CNT                       (INPUT_PROP_MAX + 1)
>>
>> I'm trying to understand these later patches for T5R2. There are
>> "hidden" touch slots now. How does userspace tell whether a touch slot
>> is a hidden touch?
>>
>
> I'm trying to understand as well.  If I was a consumer of events and I
> knew touchpad supports 2 touch tracking but 5 finger detection, I'd
> assume:
>
> * ABS_X/Y reports oldest touch and
> BTN_TOOL_DOUBLETAP/TRIPLETAP/QUADTAP reports # of fingers touching
> (ignoring lost of 5 finger for now).
> * ABS_MT_* only contains 2 slots and without SEMI_MT declared.
> BTN_TOOL_* could be used to detect greater than 2 finger touches.
>
> I'm not sure I could guess what to do with 5 slots and how to expect
> touch data to come over them.
>
> Where you saying in the other thread that the 5 slots are to help user
> detect finger transitions and re-act bounces in X/Y accordingly?  I'm
> assuming they could react in same way to BTN_TOOL_* changes if we can
> extend to 5 finger case?

The "number of active slots" (ie slots with tracking_id != -1) can
also always be used to determine the number of touches.

Given 5 slots, each assigned a slot_id: {0, 1, 2, 3, 4}.
Then:
  (a) the active slot with lowest slot_id contains one valid position
(contact from SGM packet).
  (b) the active slot with highest slot_id contains the other valid
position (contact from AGM packet).
  (c) any active slots between these are "hidden", meaning that their
position data is stale, but they count towards the finger count, and
their tracking_ids stay valid.

The real benefit is what happens when fingers are removed.   Let's say
there are 4 touches: {0, 1, 2, 4}.  Now, if a subset of fingers was
removed {1, 4}, then the T5R2 method could actually identify that
fingers {0, 2} were still being reported.

Curiously if another finger were to get added at this point, it would
become the new hidden finger 1.  This is just how the image sensor
works.

This patchset deduces what the firmware is doing in the kernel, and
T5R2 allows us to use the MT-B protocol to express this state up to
userspace.

Thanks,
-Dan

>
> Chris
>

[PATCH 11/12] Input: synaptics - process finger (<=3) transitions

[djkurtz]

From: Daniel Kurtz <djkurtz@chromium.org>

Synaptics T5R2 touchpads track 5 fingers, but only report 2.

This patch attempts to deal with some idiosyncrasies of the T5R2
protocol:

 * When there are 3 fingers, one finger is 'hidden', meaning its position is
   never sent to the host.
 * The number of fingers can change at any time, but is only reported in
   SGM packets, thus at a number-of-fingers change, it is not possible
   to tell whether the AGM finger is for the original or new number of
   fingers.
 * When the number of fingers changes from 2->3 it is not
   possible to tell which of the 2 fingers are now reported.
 * When number of fingers changes from 3->2 it is often not possible to
   tell which finger was removed, and which are now being reported.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/mouse/synaptics.c |  197 +++++++++++++++++++++++++++++++++++++-
 drivers/input/mouse/synaptics.h |    2 +
 2 files changed, 193 insertions(+), 6 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 19a9b7f..8b38e08 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -448,6 +448,8 @@ static int synaptics_parse_hw_state(const unsigned char buf[],
 				&& hw->w == 2) {
 			int type; /* Packet type */
 
+			priv->agm_pending = true;
+
 			type = (buf[5] & 0x30) >> 4;
 
 			switch (type) {
@@ -573,13 +575,20 @@ static void synaptics_report_mt_slot(struct input_dev *dev, int slot, int state,
 		input_report_abs(dev, ABS_MT_POSITION_Y, agm->y);
 		input_report_abs(dev, ABS_MT_PRESSURE, agm->z);
 		break;
+	case SYN_SLOT_HIDDEN:
+		input_mt_report_slot_state(dev, MT_TOOL_FINGER, true);
+		break;
 	}
 }
 
+/*
+ * Invariant: hidden_count = (count - 2);  hidden_count < (agm - sgm)
+ */
 static void synaptics_update_slots(struct synaptics_data *priv, int count,
 				   int sgm, int agm)
 {
 	int i;
+	int hidden_count;
 
 	/* First, clear previous slots. */
 	for (i = 0; i < SYN_TRACK_SLOT_COUNT; i++)
@@ -592,12 +601,45 @@ static void synaptics_update_slots(struct synaptics_data *priv, int count,
 	if (count < 2)
 		return;
 	priv->slot[agm] = SYN_SLOT_AGM;
+
+	/* Assign hidden slots between sgm and agm */
+	hidden_count = count - 2;
+	if (hidden_count < 1)
+		return;
+	if (hidden_count >= (agm-sgm))
+		hidden_count = agm-sgm-1;
+	for (i = 0; i < hidden_count; i++)
+		priv->slot[sgm + 1 + i] = SYN_SLOT_HIDDEN;
+}
+
+static int synaptics_find_sgm(struct synaptics_data *priv)
+{
+	int i;
+
+	for (i = 0; i < SYN_TRACK_SLOT_COUNT; i++)
+		if (priv->slot[i] == SYN_SLOT_SGM)
+			return i;
+	return -ENOENT;
+}
+
+static int synaptics_find_agm(struct synaptics_data *priv)
+{
+	int i;
+
+	for (i = 1; i < SYN_TRACK_SLOT_COUNT; i++)
+		if (priv->slot[i] == SYN_SLOT_AGM)
+			return i;
+	return -ENOENT;
 }
 
 static void synaptics_process_hw_state(struct synaptics_data *priv,
 				       struct synaptics_hw_state *sgm)
 {
+	struct synaptics_hw_state *agm = &priv->agm;
+	int slot_sgm;
+	int slot_agm;
 	int new_num_fingers;
+	int old_num_fingers = priv->num_fingers;
 
 	if (sgm->z == 0)
 		new_num_fingers = 0;
@@ -608,20 +650,163 @@ static void synaptics_process_hw_state(struct synaptics_data *priv,
 	else if (sgm->w == 1)
 		new_num_fingers = 3;
 
-	switch (new_num_fingers) {
-	case 0:
+	if (new_num_fingers == 0) {
 		synaptics_update_slots(priv, 0, 0, 0);
-		break;
-	case 1:
+		goto process_mt_data_done;
+	}
+
+	/*
+	 * If the last agm was (0,0,0), then SGM is in slot[0], and all other
+	 * fingers have been removed.
+	 */
+	if (priv->agm_pending && agm->z == 0) {
 		synaptics_update_slots(priv, 1, 0, 0);
+		goto process_mt_data_done;
+	}
+
+	/*
+	 * Update slots to in response to number of fingers changing from
+	 * old_num_fingers to new_num_fingers.
+	 */
+	switch (new_num_fingers) {
+	case 1:
+		switch (old_num_fingers) {
+		case 0:
+			synaptics_update_slots(priv, 1, 0, 0);
+			break;
+		case 1:
+			/*
+			 * If received AGM and previous SGM slot was 0, or
+			 * there was no SGM slot, then switch SGM slot to 1.
+			 *
+			 * The "SGM slot = 0" case happens with very rapid
+			 * "drum roll" gestures, where slot 0 finger is lifted
+			 * and a new slot 1 finger touches within one reporting
+			 * interval.
+			 * The "no SGM slot" case happens if initially two
+			 * or more fingers tap briefly, and all but one lift
+			 * before the end of the first reporting interval.
+			 * (In these cases, slot 0 becomes empty, only
+			 * slot 1, is reported with the SGM)
+			 *
+			 * Else if there was no previous SGM, use slot 0.
+			 *
+			 * Else, reuse the current SGM slot.
+			 */
+
+			/* Determine previous SGM slot, if there was one. */
+			slot_sgm = synaptics_find_sgm(priv);
+
+			if (priv->agm_pending && slot_sgm <= 0)
+				slot_sgm = 1;
+			else if (slot_sgm < 0)
+				slot_sgm = 0;
+
+			synaptics_update_slots(priv, 1, slot_sgm, 0);
+			break;
+		case 2:
+			/*
+			 * Since last AGM was not (0,0,0),
+			 * previous AGM is now the SGM.
+			 */
+			slot_agm = synaptics_find_agm(priv);
+			synaptics_update_slots(priv, 1, slot_agm, 0);
+			break;
+		case 3:
+			/*
+			 * Since last AGM was not (0,0,0), we don't know what
+			 * finger is left. So empty all slots.
+			 * The slot gets filled on a subsequent 1->1
+			 */
+			synaptics_update_slots(priv, 0, 0, 0);
+			break;
+		}
 		break;
+
 	case 2:
-	case 3: /* Fall-through case */
-		synaptics_update_slots(priv, 2, 0, 1);
+		switch (old_num_fingers) {
+		case 0:
+			synaptics_update_slots(priv, 2, 0, 1);
+			break;
+		case 1:
+			/*
+			 * If previous slot 0 had SGM,
+			 * the new finger, slot 1, is in AGM.
+			 * Otherwise, new finger, slot 0, is in SGM.
+			 */
+			slot_sgm = synaptics_find_sgm(priv);
+			if (slot_sgm < 1)
+				slot_sgm = 1;
+			synaptics_update_slots(priv, 2, 0, slot_sgm);
+			break;
+		case 2:
+			/* Assuming no change in fingers... */
+			slot_sgm = synaptics_find_sgm(priv);
+			slot_agm = synaptics_find_agm(priv);
+			if (slot_sgm < 0)
+				slot_sgm = 0;
+			if (slot_agm < slot_sgm + 1)
+				slot_agm = slot_sgm + 1;
+			synaptics_update_slots(priv, 2, slot_sgm, slot_agm);
+			break;
+		case 3:
+			/*
+			 * 3->2 transitions have two unsolvable problems:
+			 *  1) no indication is given which finger was removed
+			 *  2) no way to tell if agm packet was for finger 3
+			 *     before 3->2, or finger 2 after 3->2.
+			 *
+			 * So, empty all slots.
+			 * The slots get filled on a subsequent 2->2
+			 */
+			synaptics_update_slots(priv, 0, 0, 0);
+			break;
+		}
+		break;
+
+	case 3:
+		switch (old_num_fingers) {
+		case 0:
+			synaptics_update_slots(priv, 3, 0, 2);
+			break;
+		case 1:
+			/*
+			 * If old SGM was slot 2 or higher, that slot is now
+			 * AGM, with one of the new fingers in slot 0 as SGM.
+			 * Otherwise, the 3 used slots are 0,1,2.
+			 */
+			slot_sgm = synaptics_find_sgm(priv);
+			if (slot_sgm < 2)
+				slot_sgm = 2;
+			synaptics_update_slots(priv, 3, 0, slot_sgm);
+			break;
+		case 2:
+			/* On 2->3 transitions, we are given no indication
+			 * which finger was added.
+			 * We don't even know what finger the current AGM packet
+			 * contained.
+			 * So, empty all slots.
+			 * The slots get filled on a subsequent 3->3
+			 */
+			synaptics_update_slots(priv, 0, 0, 0);
+			break;
+		case 3:
+			/* Assuming no change in fingers... */
+			slot_sgm = synaptics_find_sgm(priv);
+			slot_agm = synaptics_find_agm(priv);
+			if (slot_sgm < 0)
+				slot_sgm = 0;
+			if (slot_agm < slot_sgm + 2)
+				slot_agm = slot_sgm + 2;
+			synaptics_update_slots(priv, 3, slot_sgm, slot_agm);
+			break;
+		}
 		break;
 	}
 
+process_mt_data_done:
 	priv->num_fingers = new_num_fingers;
+	priv->agm_pending = false;
 }
 
 static void synaptics_report_mt_data(struct psmouse *psmouse,
diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
index 2214af6..cc193b6 100644
--- a/drivers/input/mouse/synaptics.h
+++ b/drivers/input/mouse/synaptics.h
@@ -120,6 +120,7 @@
 #define SYN_SLOT_EMPTY			0
 #define SYN_SLOT_SGM			1
 #define SYN_SLOT_AGM			2
+#define SYN_SLOT_HIDDEN			3
 
 /* number of tracking slots for Image Sensor firmware */
 #define SYN_TRACK_SLOT_COUNT		5
@@ -166,6 +167,7 @@ struct synaptics_data {
 	struct synaptics_hw_state agm;		/* last AGM packet */
 	int num_fingers;			/* current finger count */
 	int slot[SYN_TRACK_SLOT_COUNT];		/* finger slot state */
+	bool agm_pending;			/* new AGM packet received */
 };
 
 void synaptics_module_init(void);
-- 
1.7.3.1

[PATCH 12/12] Input: synaptics - process finger (<=5) transitions

[djkurtz]

From: Daniel Kurtz <djkurtz@chromium.org>

T5R2 touchpads track up to 5 fingers, but only report 2.
They introduce a special "TYPE=2" (AGM-CONTACT) packet type that reports
the number of tracked fingers and which finger is reported in the SGM
and AGM packets.

With this new packet type, it is possible to send up to 5 MTB slots to
userspace.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/input/mouse/synaptics.c |   33 +++++++++++++++++++++++++++++++--
 1 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 8b38e08..5f227be 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -647,11 +647,16 @@ static void synaptics_process_hw_state(struct synaptics_data *priv,
 		new_num_fingers = 1;
 	else if (sgm->w == 0)
 		new_num_fingers = 2;
-	else if (sgm->w == 1)
-		new_num_fingers = 3;
+	else if (sgm->w == 1) {
+		if (agm->finger_count > 3)
+			new_num_fingers = agm->finger_count;
+		else
+			new_num_fingers = 3;
+	}
 
 	if (new_num_fingers == 0) {
 		synaptics_update_slots(priv, 0, 0, 0);
+		synaptics_agm_finger_update(priv, 0, 0, 0);
 		goto process_mt_data_done;
 	}
 
@@ -661,6 +666,7 @@ static void synaptics_process_hw_state(struct synaptics_data *priv,
 	 */
 	if (priv->agm_pending && agm->z == 0) {
 		synaptics_update_slots(priv, 1, 0, 0);
+		synaptics_agm_finger_update(priv, 0, 0, 0);
 		goto process_mt_data_done;
 	}
 
@@ -720,6 +726,11 @@ static void synaptics_process_hw_state(struct synaptics_data *priv,
 			 */
 			synaptics_update_slots(priv, 0, 0, 0);
 			break;
+
+		case 4:
+		case 5:
+			synaptics_update_slots(priv, 1, agm->finger_sgm, 0);
+			break;
 		}
 		break;
 
@@ -761,6 +772,12 @@ static void synaptics_process_hw_state(struct synaptics_data *priv,
 			 */
 			synaptics_update_slots(priv, 0, 0, 0);
 			break;
+
+		case 4:
+		case 5:
+			synaptics_update_slots(priv, 2, agm->finger_sgm,
+					       agm->finger_agm);
+			break;
 		}
 		break;
 
@@ -800,8 +817,20 @@ static void synaptics_process_hw_state(struct synaptics_data *priv,
 				slot_agm = slot_sgm + 2;
 			synaptics_update_slots(priv, 3, slot_sgm, slot_agm);
 			break;
+
+		case 4:
+		case 5:
+			synaptics_update_slots(priv, 3, agm->finger_sgm,
+					       agm->finger_agm);
+			break;
 		}
 		break;
+
+	case 4:
+	case 5:
+		synaptics_update_slots(priv, agm->finger_count, agm->finger_sgm,
+				       agm->finger_agm);
+		break;
 	}
 
 process_mt_data_done:
-- 
1.7.3.1