[PATCH] Fix sentelic multi-touch driver

[PATCH] Fix sentelic multi-touch driver

[Olivier Goffart]

Always report both finger between the sync.  Even if the documentation
of input-mt says one can only report one at the time, it does not
work otherwise. This fix scrolling in X11 which flickers a lot.

Ignore the duplicate or release events. They keep some gestures from
working (such as the two-finger tap)

This seems to fix the problems reported here:
https://bugzilla.kernel.org/show_bug.cgi?id=43591

Signed-off-by: Olivier Goffart <ogoffart@woboq.com>
---
 drivers/input/mouse/sentelic.c | 46 +++++++++++++++++++++++++++++++++++++-----
 drivers/input/mouse/sentelic.h |  3 +++
 2 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/drivers/input/mouse/sentelic.c b/drivers/input/mouse/sentelic.c
index 3f5649f..b205857 100644
--- a/drivers/input/mouse/sentelic.c
+++ b/drivers/input/mouse/sentelic.c
@@ -731,6 +731,17 @@ static psmouse_ret_t fsp_process_byte(struct psmouse *psmouse)
 			 */
 			fgrs = 2;
 
+			if (abs_x == 0 && abs_y == 0) {
+				/*
+				 * We can ignore multi-finger release events.
+				 * We notice a finger has left the touchpad
+				 * when we receive events for the other finger.
+				 * And for the last finger, we receive
+				 * single-finger release events.
+				 */
+				return PSMOUSE_FULL_PACKET;
+			}
+
 			/* MFMC packet */
 			if (packet[0] & FSP_PB0_MFMC_FGR2) {
 				/* 2nd finger */
@@ -742,10 +753,16 @@ static psmouse_ret_t fsp_process_byte(struct psmouse *psmouse)
 					 */
 					fgrs = 1;
 					fsp_set_slot(dev, 0, false, 0, 0);
+					fsp_set_slot(dev, 1, fgrs == 1,
+						     abs_x, abs_y);
+				} else {
+					ad->last_mt_fgr = 2;
+					fsp_set_slot(dev, 0, true,
+						     ad->finger1_abs_x,
+						     ad->finger1_abs_y);
+					fsp_set_slot(dev, 1, fgrs == 2,
+						     abs_x, abs_y);
 				}
-				ad->last_mt_fgr = 2;
-
-				fsp_set_slot(dev, 1, fgrs == 2, abs_x, abs_y);
 			} else {
 				/* 1st finger */
 				if (ad->last_mt_fgr == 1) {
@@ -755,10 +772,29 @@ static psmouse_ret_t fsp_process_byte(struct psmouse *psmouse)
 					 * the 2nd finger is up
 					 */
 					fgrs = 1;
+					fsp_set_slot(dev, 0, fgrs != 0,
+						     abs_x, abs_y);
 					fsp_set_slot(dev, 1, false, 0, 0);
+				} else {
+					ad->finger1_abs_x = abs_x;
+					ad->finger1_abs_y = abs_y;
+
+					/*
+					 * There is a superflous event from the
+					 * driver when the two fingers are put
+					 * on the touchpad. Don't set
+					 * last_mt_fgr if we have not seen the
+					 * second figner yet, so the previous
+					 * workarond is not triggered and the
+					 * superflous event is ignored.
+					 * This is required to get the two
+					 * finger tap gesture working.
+					 */
+					if (ad->last_mt_fgr != 0)
+						ad->last_mt_fgr = 1;
+
+					return PSMOUSE_FULL_PACKET;
 				}
-				ad->last_mt_fgr = 1;
-				fsp_set_slot(dev, 0, fgrs != 0, abs_x, abs_y);
 			}
 		} else {
 			/* SFAC packet */
diff --git a/drivers/input/mouse/sentelic.h b/drivers/input/mouse/sentelic.h
index aa697ec..7224f84 100644
--- a/drivers/input/mouse/sentelic.h
+++ b/drivers/input/mouse/sentelic.h
@@ -117,6 +117,9 @@ struct fsp_data {
 	unsigned char	last_reg;	/* Last register we requested read from */
 	unsigned char	last_val;
 	unsigned int	last_mt_fgr;	/* Last seen finger(multitouch) */
+
+	unsigned short finger1_abs_x; /* finger1 absolute position */
+	unsigned short finger1_abs_y;
 };
 
 #ifdef CONFIG_MOUSE_PS2_SENTELIC
-- 
1.7.11.1

Re: [PATCH] Fix sentelic multi-touch driver

[Tai-hwa Liang]

On Wed, 27 Jun 2012, Olivier Goffart wrote:
> Always report both finger between the sync.  Even if the documentation
> of input-mt says one can only report one at the time, it does not
> work otherwise. This fix scrolling in X11 which flickers a lot.

   I have a similar patch to address this issue. Would you please try to
see if https://bugzilla.kernel.org/attachment.cgi?id=73991 works for you?

> Ignore the duplicate or release events. They keep some gestures from
> working (such as the two-finger tap)
>
> This seems to fix the problems reported here:
> https://bugzilla.kernel.org/show_bug.cgi?id=43591

   FWIW, https://bugzilla.kernel.org/show_bug.cgi?id=43197 is the original
thread of the aforementioned patch.

Thanks,

Tai-hwa Liang

Re: [PATCH] Fix sentelic multi-touch driver

[Olivier Goffart]

On Thursday 28 June 2012 16:02:15 Tai-hwa Liang wrote:
> On Wed, 27 Jun 2012, Olivier Goffart wrote:
> > Always report both finger between the sync.  Even if the documentation
> > of input-mt says one can only report one at the time, it does not
> > work otherwise. This fix scrolling in X11 which flickers a lot.
> 
>    I have a similar patch to address this issue. Would you please try to
> see if https://bugzilla.kernel.org/attachment.cgi?id=73991 works for you?


Hi,

That patch also fixes the issue here.  (It fixes scrolling and two finger tap)
But the case in which you put two finger into the touchpad then release one 
finger is not handled properly. (I took care of that issue)

(I have a Asus Zenbook UX31)

 
> > Ignore the duplicate or release events. They keep some gestures from
> > working (such as the two-finger tap)
> > 
> > This seems to fix the problems reported here:
> > https://bugzilla.kernel.org/show_bug.cgi?id=43591
> 
>    FWIW, https://bugzilla.kernel.org/show_bug.cgi?id=43197 is the original
> thread of the aforementioned patch.

Re: [PATCH] Fix sentelic multi-touch driver

[Tai-hwa Liang]

On Thu, 28 Jun 2012, Olivier Goffart wrote:
> On Thursday 28 June 2012 16:02:15 Tai-hwa Liang wrote:
[...]
>>    I have a similar patch to address this issue. Would you please try to
>> see if https://bugzilla.kernel.org/attachment.cgi?id=73991 works for you?
>
> Hi,
>
> That patch also fixes the issue here.  (It fixes scrolling and two finger tap)
> But the case in which you put two finger into the touchpad then release one
> finger is not handled properly. (I took care of that issue)

   Would you please be more specific about the steps to reproduce this?
I've tested both of these two patches but failed to see the difference in
this case.

Thanks,

Tai-hwa Liang

Re: [PATCH] Fix sentelic multi-touch driver

[Olivier Goffart]

On Thursday 28 June 2012 17:17:50 Tai-hwa Liang wrote:
> On Thu, 28 Jun 2012, Olivier Goffart wrote:
> > On Thursday 28 June 2012 16:02:15 Tai-hwa Liang wrote:
> [...]
> 
> >>    I have a similar patch to address this issue. Would you please try to
> >> 
> >> see if https://bugzilla.kernel.org/attachment.cgi?id=73991 works for you?
> > 
> > Hi,
> > 
> > That patch also fixes the issue here.  (It fixes scrolling and two finger
> > tap) But the case in which you put two finger into the touchpad then
> > release one finger is not handled properly. (I took care of that issue)
> 
>    Would you please be more specific about the steps to reproduce this?
> I've tested both of these two patches but failed to see the difference in
> this case.


 - Touch the touchpad with one finger and move it: this moves the cursor
 - Place a second finger: this allows you to scoll
 - Release a finger: you should now be able to move the cursor again.  This is 
the part that does not work in one of the patch

Re: [PATCH] Fix sentelic multi-touch driver

[Tai-hwa Liang]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 794 bytes --]

On Thu, 28 Jun 2012, Olivier Goffart wrote:
[...]
> - Touch the touchpad with one finger and move it: this moves the cursor
> - Place a second finger: this allows you to scoll
> - Release a finger: you should now be able to move the cursor again.  This is
> the part that does not work in one of the patch

   Sorry, I didn't realise that one of my patch submitted a few months ago
(namely, http://www.spinics.net/lists/linux-input/msg19443.html) isn't
in the master tree, which is different from my local working copy.

   With the correct flag(FSP_BIT_SWC1_EN_GID) being set, the hardware
clears MFMC bit during 2 -> 1 finger transition.  The attached patch is
an aggregated version against Dmitry's master branch, which supposes to fix 
the testing case listed above.

Thanks,

Tai-hwa Liang

[-- Attachment #2: Type: TEXT/x-diff, Size: 9665 bytes --]

diff --git a/drivers/input/mouse/sentelic.c b/drivers/input/mouse/sentelic.c
index 3f5649f..c8a20c0 100644
--- a/drivers/input/mouse/sentelic.c
+++ b/drivers/input/mouse/sentelic.c
@@ -706,8 +706,9 @@ static psmouse_ret_t fsp_process_byte(struct psmouse *psmouse)
 	struct input_dev *dev = psmouse->dev;
 	struct fsp_data *ad = psmouse->private;
 	unsigned char *packet = psmouse->packet;
-	unsigned char button_status = 0, lscroll = 0, rscroll = 0;
-	unsigned short abs_x, abs_y, fgrs = 0;
+	unsigned char button_status = 0;
+	unsigned short abs_x, abs_y, fgrs;
+	unsigned short vscroll = 0, hscroll = 0, lscroll = 0, rscroll = 0;
 	int rel_x, rel_y;
 
 	if (psmouse->pktcnt < 4)
@@ -720,65 +721,99 @@ static psmouse_ret_t fsp_process_byte(struct psmouse *psmouse)
 	fsp_packet_debug(psmouse, packet);
 
 	switch (psmouse->packet[0] >> FSP_PKT_TYPE_SHIFT) {
+	case FSP_PKT_TYPE_NOTIFY:
+		if (packet[1] != FSP_NOTIFY_MSG_GID)
+			break;	/* unsupported message types */
+
+		switch (packet[2]) {
+		case FSP_GID_SP_UP:
+			vscroll =  1;
+			break;
+		case FSP_GID_SP_DOWN:
+			vscroll = -1;
+			break;
+		case FSP_GID_SP_LEFT:
+			hscroll =  1;
+			break;
+		case FSP_GID_SP_RIGHT:
+			hscroll = -1;
+			break;
+		default:
+			dev_dbg(&psmouse->ps2dev.serio->dev,
+				"GID 0x%x\n", packet[2]);
+			break;
+		}
+		input_report_rel(dev, REL_WHEEL, vscroll);
+		input_report_rel(dev, REL_HWHEEL, hscroll);
+		input_event(dev, EV_MSC, MSC_GESTURE, packet[2]);
+		break;
+
 	case FSP_PKT_TYPE_ABS:
+		if ((packet[0] == 0x48 || packet[0] == 0x49) &&
+		    packet[1] == 0 && packet[2] == 0) {
+			/*
+			 * filtering out erratic movement which will cause
+			 * unexpected cursor jumping to the top-left corner
+			 */
+			packet[3] &= 0xf0;
+		}
+		if (!IS_MFMC(packet[0]) && (packet[0] & (FSP_PB0_LBTN |
+		    FSP_PB0_PHY_BTN)) == FSP_PB0_LBTN) {
+			/*
+			 * On-pad click in SFAC mode should be handled
+			 * by userspace.  On-pad clicks in MFMC mode
+			 * are real clickpad clicks, and not ignored.
+			 */
+			packet[0] &= ~FSP_PB0_LBTN;
+		}
 		abs_x = GET_ABS_X(packet);
 		abs_y = GET_ABS_Y(packet);
 
-		if (packet[0] & FSP_PB0_MFMC) {
+		if (abs_x == 0 || abs_y == 0) {
 			/*
-			 * MFMC packet: assume that there are two fingers on
-			 * pad
+			 * workaround for buggy firmware which sends zero
+			 * coordinates even if there is finger on pad
 			 */
-			fgrs = 2;
-
-			/* MFMC packet */
-			if (packet[0] & FSP_PB0_MFMC_FGR2) {
-				/* 2nd finger */
-				if (ad->last_mt_fgr == 2) {
-					/*
-					 * workaround for buggy firmware
-					 * which doesn't clear MFMC bit if
-					 * the 1st finger is up
-					 */
-					fgrs = 1;
-					fsp_set_slot(dev, 0, false, 0, 0);
-				}
-				ad->last_mt_fgr = 2;
-
-				fsp_set_slot(dev, 1, fgrs == 2, abs_x, abs_y);
+			if (!IS_MFMC(packet[0])) {
+				/* don't report bad movement */
+				fgrs = 0;
 			} else {
-				/* 1st finger */
-				if (ad->last_mt_fgr == 1) {
-					/*
-					 * workaround for buggy firmware
-					 * which doesn't clear MFMC bit if
-					 * the 2nd finger is up
-					 */
-					fgrs = 1;
-					fsp_set_slot(dev, 1, false, 0, 0);
-				}
-				ad->last_mt_fgr = 1;
-				fsp_set_slot(dev, 0, fgrs != 0, abs_x, abs_y);
-			}
-		} else {
-			/* SFAC packet */
-			if ((packet[0] & (FSP_PB0_LBTN|FSP_PB0_PHY_BTN)) ==
-				FSP_PB0_LBTN) {
-				/* On-pad click in SFAC mode should be handled
-				 * by userspace.  On-pad clicks in MFMC mode
-				 * are real clickpad clicks, and not ignored.
+				/*
+				 * for zero 2nd MFMC packet, use saved
+				 * coordinate instead
 				 */
-				packet[0] &= ~FSP_PB0_LBTN;
+				abs_x = ad->mfmc_x1;
+				abs_y = ad->mfmc_y1;
+				fgrs = 2;
 			}
-
-			/* no multi-finger information */
-			ad->last_mt_fgr = 0;
-
-			if (abs_x != 0 && abs_y != 0)
+		} else {
+			/* finger(s) down */
+			if (!IS_MFMC(packet[0])) {
+				/* SFAC, MFMC CPB packet */
 				fgrs = 1;
 
-			fsp_set_slot(dev, 0, fgrs > 0, abs_x, abs_y);
-			fsp_set_slot(dev, 1, false, 0, 0);
+				fsp_set_slot(dev, 0, true, abs_x, abs_y);
+				fsp_set_slot(dev, 1, false, 0, 0);
+			} else if (IS_MFMC_FGR1(packet[0])) {
+				/* MFMC 1st finger */
+				fgrs = 2;
+
+				/*
+				 * accumulate the coordaintes and proceed to
+				 * the next run
+				 */
+				ad->mfmc_x1 = abs_x;
+				ad->mfmc_y1 = abs_y;
+			} else {
+				/* MFMC 2nd finger */
+				fgrs = 2;
+
+				/* report both fingers' coordinate */
+				fsp_set_slot(dev, 1, true, abs_x, abs_y);
+				abs_x = ad->mfmc_x1;
+				abs_y = ad->mfmc_y1;
+				fsp_set_slot(dev, 0, true, abs_x, abs_y);
+			}
 		}
 		if (fgrs > 0) {
 			input_report_abs(dev, ABS_X, abs_x);
@@ -916,7 +951,10 @@ static int fsp_activate_protocol(struct psmouse *psmouse)
 				  FSP_BIT_SWC1_EN_ABS_1F |
 				  FSP_BIT_SWC1_EN_ABS_2F |
 				  FSP_BIT_SWC1_EN_FUP_OUT |
-				  FSP_BIT_SWC1_EN_ABS_CON)) {
+				  FSP_BIT_SWC1_EN_ABS_CON |
+				  FSP_BIT_SWC1_EN_GID |
+				  FSP_BIT_SWC1_GST_GRP0 |
+				  FSP_BIT_SWC1_GST_GRP1)) {
 			psmouse_err(psmouse,
 				    "Unable to enable absolute coordinates output.\n");
 			return -EIO;
@@ -963,6 +1001,9 @@ static int fsp_set_input_params(struct psmouse *psmouse)
 		input_mt_init_slots(dev, 2);
 		input_set_abs_params(dev, ABS_MT_POSITION_X, 0, abs_x, 0, 0);
 		input_set_abs_params(dev, ABS_MT_POSITION_Y, 0, abs_y, 0, 0);
+
+		/* device generated gesture ID events */
+		input_set_capability(dev, EV_MSC, MSC_GESTURE);
 	}
 
 	return 0;
diff --git a/drivers/input/mouse/sentelic.h b/drivers/input/mouse/sentelic.h
index aa697ec..8a5cc59 100644
--- a/drivers/input/mouse/sentelic.h
+++ b/drivers/input/mouse/sentelic.h
@@ -90,6 +90,112 @@
 #define	FSP_PB0_MUST_SET	BIT(3)
 #define	FSP_PB0_PHY_BTN		BIT(4)
 #define	FSP_PB0_MFMC		BIT(5)
+/* packet type. See FSP_PKT_TYPE_XXX */
+#define	FSP_PB0_TYPE		(BIT(7) | BIT(6))
+#define	FSP_PB0_TYPE_REL	(0)
+#define	FSP_PB0_TYPE_ABS	BIT(6)
+#define	FSP_PB0_TYPE_NOTIFY	BIT(7)
+#define	FSP_PB0_TYPE_CUSTOM	(BIT(7) | BIT(6))
+
+/*
+ * physical clickpad button = MFMC packet without physical button.
+ */
+#define	IS_PHY_CPB(_pb0_)	\
+	(((_pb0_) & (FSP_PB0_TYPE | FSP_PB0_MFMC | FSP_PB0_PHY_BTN)) == \
+	(FSP_PB0_TYPE_ABS | FSP_PB0_MFMC))
+/*
+ * single-finger absolute coordinate
+ */
+#define	IS_SFAC(_pb0_)		\
+	(((_pb0_) & (FSP_PB0_TYPE | FSP_PB0_MFMC)) == FSP_PB0_TYPE_ABS)
+/*
+ * multi-finger multi-coordinate (physical clickpad button is excluded)
+ */
+#define	IS_MFMC(_pb0_)		\
+	(((_pb0_) & (FSP_PB0_TYPE | FSP_PB0_MFMC | FSP_PB0_PHY_BTN)) == \
+	(FSP_PB0_TYPE_ABS | FSP_PB0_MFMC | FSP_PB0_PHY_BTN))
+#define	IS_MFMC_FGR1(_pb0_)	\
+	(((_pb0_) & (FSP_PB0_TYPE | FSP_PB0_MFMC | FSP_PB0_PHY_BTN | \
+		FSP_PB0_MFMC_FGR2)) == (FSP_PB0_TYPE_ABS | \
+		FSP_PB0_MFMC | FSP_PB0_PHY_BTN))
+#define	IS_MFMC_FGR2(_pb0_)	\
+	(((_pb0_) & (FSP_PB0_TYPE | FSP_PB0_MFMC | FSP_PB0_PHY_BTN | \
+		FSP_PB0_MFMC_FGR2)) == (FSP_PB0_TYPE_ABS | \
+		FSP_PB0_MFMC | FSP_PB0_PHY_BTN | FSP_PB0_MFMC_FGR2))
+
+
+/* notification message types */
+#define	FSP_NOTIFY_MSG_GID	(0xba)
+#define	FSP_NOTIFY_MSG_HX_GST	(0xc0)
+
+/* gesture IDs */
+/** GID for 2F Straight Up             */
+#define	FSP_GID_SP1		(0x86)
+#define	FSP_GID_SP_UP		FSP_GID_SP1
+
+/** GID for 2F Straight Down           */
+#define	FSP_GID_SP5		(0x82)
+#define	FSP_GID_SP_DOWN		FSP_GID_SP5
+
+/** GID for 2F Straight Right          */
+#define	FSP_GID_SP2		(0x80)
+#define	FSP_GID_SP_RIGHT	FSP_GID_SP2
+
+/** GID for 2F Straight Left           */
+#define	FSP_GID_SP6		(0x84)
+#define	FSP_GID_SP_LEFT		FSP_GID_SP6
+
+/** GID for 2F Zoom In                 */
+#define	FSP_GID_SC6		(0x8F)
+#define	FSP_GID_SC_ZOOM_IN	FSP_GID_SC6
+
+/** GID for 2F Zoom Out                */
+#define	FSP_GID_SC3		(0x8B)
+#define	FSP_GID_SC_ZOOM_OUT	FSP_GID_SC3
+
+/** GID for 2F Curve CW                */
+#define	FSP_GID_DC1		(0xC4)
+#define	FSP_GID_DC_ROT_CW	FSP_GID_DC1
+
+/** GID for 2F Curve CCW               */
+#define	FSP_GID_DC2		(0xC0)
+#define	FSP_GID_DC_ROT_CCW	FSP_GID_DC2
+
+/** GID for 3F Straight Up             */
+#define	FSP_GID_TS4		(0x2E)
+#define	FSP_GID_TS_UP		FSP_GID_TS4
+
+/** GID for 3F Straight Down           */
+#define	FSP_GID_TS2		(0x2A)
+#define	FSP_GID_TS_DOWN		FSP_GID_TS2
+
+/** GID for 3F Straight Right          */
+#define	FSP_GID_TS1		(0x28)
+#define	FSP_GID_TS_RIGHT	FSP_GID_TS1
+
+/** GID for 3F Straight Left           */
+#define	FSP_GID_TS3		(0x2C)
+#define	FSP_GID_TS_LEFT		FSP_GID_TS3
+
+/** GID for 2F Click                   */
+#define	FSP_GID_DF1		(0x11)
+#define	FSP_GID_DF_CLICK	FSP_GID_DF1
+
+/** GID for 2F Double Click            */
+#define	FSP_GID_DF2		(0x12)
+#define	FSP_GID_DF_DBLCLICK	FSP_GID_DF2
+
+/** GID for 3F Click                   */
+#define	FSP_GID_TF1		(0x19)
+#define	FSP_GID_TF_CLICK	FSP_GID_TF1
+
+/** GID for 3F Double Click            */
+#define	FSP_GID_TF2		(0x1A)
+#define	FSP_GID_TF_DBLCLICK	FSP_GID_TF2
+
+/** GID for Palm                       */
+#define	FSP_GID_PM1		(0x38)
+#define	FSP_GID_PALM		FSP_GID_PM1
 
 /* hardware revisions */
 #define	FSP_VER_STL3888_A4	(0xC1)
@@ -116,7 +222,8 @@ struct fsp_data {
 
 	unsigned char	last_reg;	/* Last register we requested read from */
 	unsigned char	last_val;
-	unsigned int	last_mt_fgr;	/* Last seen finger(multitouch) */
+	unsigned short	mfmc_x1;	/* X coordinate for the 1st finger */
+	unsigned short	mfmc_y1;	/* Y coordinate for the 1st finger */
 };
 
 #ifdef CONFIG_MOUSE_PS2_SENTELIC

Re: [PATCH] Fix sentelic multi-touch driver

[Olivier Goffart]

On Friday 29 June 2012 11:03:12 Tai-hwa Liang wrote:
> On Thu, 28 Jun 2012, Olivier Goffart wrote:
> [...]
> 
> > - Touch the touchpad with one finger and move it: this moves the cursor
> > - Place a second finger: this allows you to scoll
> > - Release a finger: you should now be able to move the cursor again.  This
> > is the part that does not work in one of the patch
> 
>    Sorry, I didn't realise that one of my patch submitted a few months ago
> (namely, http://www.spinics.net/lists/linux-input/msg19443.html) isn't
> in the master tree, which is different from my local working copy.
> 
>    With the correct flag(FSP_BIT_SWC1_EN_GID) being set, the hardware
> clears MFMC bit during 2 -> 1 finger transition.  The attached patch is
> an aggregated version against Dmitry's master branch, which supposes to fix
> the testing case listed above.
> 
> Thanks,
> 
> Tai-hwa Liang

I am testing against the stable 3.4.y branch.
With that patch, scrolling is very slow, and it emits middle click sometimes 
while I want to scroll fast. 

Looking at the debug output, i see a lot of NOTIFY packets, for the same  
number of packets/seconds (77). So then there is 5 times less multi touch 
events. Maybe that's why.

Regards
-- 
Olivier

Re: [PATCH] Fix sentelic multi-touch driver

[Tai-hwa Liang]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 845 bytes --]

On Fri, 29 Jun 2012, Olivier Goffart wrote:
> On Friday 29 June 2012 11:03:12 Tai-hwa Liang wrote:
[...]
>>    With the correct flag(FSP_BIT_SWC1_EN_GID) being set, the hardware
>> clears MFMC bit during 2 -> 1 finger transition.  The attached patch is
>> an aggregated version against Dmitry's master branch, which supposes to fix
>> the testing case listed above.
>
> I am testing against the stable 3.4.y branch.
> With that patch, scrolling is very slow, and it emits middle click sometimes
> while I want to scroll fast.
>
> Looking at the debug output, i see a lot of NOTIFY packets, for the same
> number of packets/seconds (77). So then there is 5 times less multi touch
> events. Maybe that's why.

   I see. It appears to me that we'd better to revert back to the last_mt_fgr
workaround to avoid such delay.

-- 
Thanks,

Tai-hwa Liang

[-- Attachment #2: Type: TEXT/X-DIFF, Size: 6436 bytes --]

diff --git a/drivers/input/mouse/sentelic.c b/drivers/input/mouse/sentelic.c
index 3f5649f..995b395 100644
--- a/drivers/input/mouse/sentelic.c
+++ b/drivers/input/mouse/sentelic.c
@@ -707,7 +707,7 @@ static psmouse_ret_t fsp_process_byte(struct psmouse *psmouse)
 	struct fsp_data *ad = psmouse->private;
 	unsigned char *packet = psmouse->packet;
 	unsigned char button_status = 0, lscroll = 0, rscroll = 0;
-	unsigned short abs_x, abs_y, fgrs = 0;
+	unsigned short abs_x, abs_y, fgrs;
 	int rel_x, rel_y;
 
 	if (psmouse->pktcnt < 4)
@@ -721,64 +721,100 @@ static psmouse_ret_t fsp_process_byte(struct psmouse *psmouse)
 
 	switch (psmouse->packet[0] >> FSP_PKT_TYPE_SHIFT) {
 	case FSP_PKT_TYPE_ABS:
+		if ((packet[0] == 0x48 || packet[0] == 0x49) &&
+		    packet[1] == 0 && packet[2] == 0) {
+			/*
+			 * filtering out erratic movement which will cause
+			 * unexpected cursor jumping to the top-left corner
+			 */
+			packet[3] &= 0xf0;
+		}
 		abs_x = GET_ABS_X(packet);
 		abs_y = GET_ABS_Y(packet);
 
-		if (packet[0] & FSP_PB0_MFMC) {
+		if (!IS_MFMC(packet[0]) && (packet[0] & (FSP_PB0_LBTN |
+		    FSP_PB0_PHY_BTN)) == FSP_PB0_LBTN) {
 			/*
-			 * MFMC packet: assume that there are two fingers on
-			 * pad
+			 * On-pad click in SFAC mode should be handled
+			 * by userspace.  On-pad clicks in MFMC mode
+			 * are real clickpad clicks, and not ignored.
 			 */
-			fgrs = 2;
+			packet[0] &= ~FSP_PB0_LBTN;
+		}
+		if (abs_x == 0 || abs_y == 0) {
+			/*
+			 * workaround for buggy firmware which sends zero
+			 * coordinates even if there is finger on pad
+			 */
+			if (!IS_MFMC(packet[0])) {
+				/* don't report bad movement */
+				fgrs = 0;
+			} else {
+				/*
+				 * ignore finger up event for multiple finger
+				 * scenario
+				 */
+				return PSMOUSE_FULL_PACKET;
+			}
+		} else {
+			/* finger(s) down */
+			if (!IS_MFMC(packet[0])) {
+				/* SFAC, MFMC CPB packet */
+				fgrs = 1;
 
-			/* MFMC packet */
-			if (packet[0] & FSP_PB0_MFMC_FGR2) {
-				/* 2nd finger */
-				if (ad->last_mt_fgr == 2) {
+				/* no multi-finger information */
+				ad->last_mt_fgr = 0;
+
+				fsp_set_slot(dev, 0, true, abs_x, abs_y);
+				fsp_set_slot(dev, 1, false, 0, 0);
+			} else if (IS_MFMC_FGR1(packet[0])) {
+				/* MFMC 1st finger */
+				if (ad->last_mt_fgr == 1) {
 					/*
 					 * workaround for buggy firmware
 					 * which doesn't clear MFMC bit if
-					 * the 1st finger is up
+					 * the 2nd finger is up
 					 */
 					fgrs = 1;
-					fsp_set_slot(dev, 0, false, 0, 0);
-				}
-				ad->last_mt_fgr = 2;
+					fsp_set_slot(dev, 0, true,
+						     abs_x, abs_y);
+					fsp_set_slot(dev, 1, false, 0, 0);
+				} else {
+					fgrs = 2;
+					ad->last_mt_fgr = 1;
 
-				fsp_set_slot(dev, 1, fgrs == 2, abs_x, abs_y);
+					/*
+					 * accumulate the coordaintes and
+					 * proceed to the next run
+					 */
+					ad->mfmc_x1 = abs_x;
+					ad->mfmc_y1 = abs_y;
+				}
 			} else {
-				/* 1st finger */
-				if (ad->last_mt_fgr == 1) {
+				/* MFMC 2nd finger */
+				if (ad->last_mt_fgr == 2) {
 					/*
 					 * workaround for buggy firmware
 					 * which doesn't clear MFMC bit if
-					 * the 2nd finger is up
+					 * the 1st finger is up
 					 */
 					fgrs = 1;
-					fsp_set_slot(dev, 1, false, 0, 0);
+					fsp_set_slot(dev, 0, false, 0, 0);
+					fsp_set_slot(dev, 1, true,
+						     abs_x, abs_y);
+				} else {
+					fgrs = 2;
+					ad->last_mt_fgr = 2;
+
+					/* report both fingers' coordinate */
+					fsp_set_slot(dev, 1, true,
+						     abs_x, abs_y);
+					abs_x = ad->mfmc_x1;
+					abs_y = ad->mfmc_y1;
+					fsp_set_slot(dev, 0, true,
+						     abs_x, abs_y);
 				}
-				ad->last_mt_fgr = 1;
-				fsp_set_slot(dev, 0, fgrs != 0, abs_x, abs_y);
-			}
-		} else {
-			/* SFAC packet */
-			if ((packet[0] & (FSP_PB0_LBTN|FSP_PB0_PHY_BTN)) ==
-				FSP_PB0_LBTN) {
-				/* On-pad click in SFAC mode should be handled
-				 * by userspace.  On-pad clicks in MFMC mode
-				 * are real clickpad clicks, and not ignored.
-				 */
-				packet[0] &= ~FSP_PB0_LBTN;
 			}
-
-			/* no multi-finger information */
-			ad->last_mt_fgr = 0;
-
-			if (abs_x != 0 && abs_y != 0)
-				fgrs = 1;
-
-			fsp_set_slot(dev, 0, fgrs > 0, abs_x, abs_y);
-			fsp_set_slot(dev, 1, false, 0, 0);
 		}
 		if (fgrs > 0) {
 			input_report_abs(dev, ABS_X, abs_x);
diff --git a/drivers/input/mouse/sentelic.h b/drivers/input/mouse/sentelic.h
index aa697ec..0173fc5 100644
--- a/drivers/input/mouse/sentelic.h
+++ b/drivers/input/mouse/sentelic.h
@@ -90,6 +90,39 @@
 #define	FSP_PB0_MUST_SET	BIT(3)
 #define	FSP_PB0_PHY_BTN		BIT(4)
 #define	FSP_PB0_MFMC		BIT(5)
+/* packet type. See FSP_PKT_TYPE_XXX */
+#define	FSP_PB0_TYPE		(BIT(7) | BIT(6))
+#define	FSP_PB0_TYPE_REL	(0)
+#define	FSP_PB0_TYPE_ABS	BIT(6)
+#define	FSP_PB0_TYPE_NOTIFY	BIT(7)
+#define	FSP_PB0_TYPE_CUSTOM	(BIT(7) | BIT(6))
+
+/*
+ * physical clickpad button = MFMC packet without physical button.
+ */
+#define	IS_PHY_CPB(_pb0_)	\
+	(((_pb0_) & (FSP_PB0_TYPE | FSP_PB0_MFMC | FSP_PB0_PHY_BTN)) == \
+	(FSP_PB0_TYPE_ABS | FSP_PB0_MFMC))
+/*
+ * single-finger absolute coordinate
+ */
+#define	IS_SFAC(_pb0_)		\
+	(((_pb0_) & (FSP_PB0_TYPE | FSP_PB0_MFMC)) == FSP_PB0_TYPE_ABS)
+/*
+ * multi-finger multi-coordinate (physical clickpad button is excluded)
+ */
+#define	IS_MFMC(_pb0_)		\
+	(((_pb0_) & (FSP_PB0_TYPE | FSP_PB0_MFMC | FSP_PB0_PHY_BTN)) == \
+	(FSP_PB0_TYPE_ABS | FSP_PB0_MFMC | FSP_PB0_PHY_BTN))
+#define	IS_MFMC_FGR1(_pb0_)	\
+	(((_pb0_) & (FSP_PB0_TYPE | FSP_PB0_MFMC | FSP_PB0_PHY_BTN | \
+		FSP_PB0_MFMC_FGR2)) == (FSP_PB0_TYPE_ABS | \
+		FSP_PB0_MFMC | FSP_PB0_PHY_BTN))
+#define	IS_MFMC_FGR2(_pb0_)	\
+	(((_pb0_) & (FSP_PB0_TYPE | FSP_PB0_MFMC | FSP_PB0_PHY_BTN | \
+		FSP_PB0_MFMC_FGR2)) == (FSP_PB0_TYPE_ABS | \
+		FSP_PB0_MFMC | FSP_PB0_PHY_BTN | FSP_PB0_MFMC_FGR2))
+
 
 /* hardware revisions */
 #define	FSP_VER_STL3888_A4	(0xC1)
@@ -117,6 +150,8 @@ struct fsp_data {
 	unsigned char	last_reg;	/* Last register we requested read from */
 	unsigned char	last_val;
 	unsigned int	last_mt_fgr;	/* Last seen finger(multitouch) */
+	unsigned short	mfmc_x1;	/* X coordinate for the 1st finger */
+	unsigned short	mfmc_y1;	/* Y coordinate for the 1st finger */
 };
 
 #ifdef CONFIG_MOUSE_PS2_SENTELIC

Re: [PATCH] Fix sentelic multi-touch driver

[Olivier Goffart]

On Friday 29 June 2012 22:51:16 Tai-hwa Liang wrote:
> On Fri, 29 Jun 2012, Olivier Goffart wrote:
> > On Friday 29 June 2012 11:03:12 Tai-hwa Liang wrote:
> [...]
> 
> >>    With the correct flag(FSP_BIT_SWC1_EN_GID) being set, the hardware
> >> 
> >> clears MFMC bit during 2 -> 1 finger transition.  The attached patch is
> >> an aggregated version against Dmitry's master branch, which supposes to
> >> fix
> >> the testing case listed above.
> > 
> > I am testing against the stable 3.4.y branch.
> > With that patch, scrolling is very slow, and it emits middle click
> > sometimes while I want to scroll fast.
> > 
> > Looking at the debug output, i see a lot of NOTIFY packets, for the same
> > number of packets/seconds (77). So then there is 5 times less multi touch
> > events. Maybe that's why.
> 
>    I see. It appears to me that we'd better to revert back to the
> last_mt_fgr workaround to avoid such delay.

That last patch seems to work perfectly.

Thanks
-- 
Olivier

Re: [PATCH] Fix sentelic multi-touch driver

[Olivier Goffart]

On Friday 29 June 2012 17:00:39 Olivier Goffart wrote:
> On Friday 29 June 2012 22:51:16 Tai-hwa Liang wrote:
> > On Fri, 29 Jun 2012, Olivier Goffart wrote:
> > > On Friday 29 June 2012 11:03:12 Tai-hwa Liang wrote:
> > [...]
> > 
> > >>    With the correct flag(FSP_BIT_SWC1_EN_GID) being set, the hardware
> > >> 
> > >> clears MFMC bit during 2 -> 1 finger transition.  The attached patch is
> > >> an aggregated version against Dmitry's master branch, which supposes to
> > >> fix
> > >> the testing case listed above.
> > > 
> > > I am testing against the stable 3.4.y branch.
> > > With that patch, scrolling is very slow, and it emits middle click
> > > sometimes while I want to scroll fast.
> > > 
> > > Looking at the debug output, i see a lot of NOTIFY packets, for the same
> > > number of packets/seconds (77). So then there is 5 times less multi
> > > touch
> > > events. Maybe that's why.
> > > 
> >    I see. It appears to me that we'd better to revert back to the
> > 
> > last_mt_fgr workaround to avoid such delay.
> 
> That last patch seems to work perfectly.
> 
> Thanks

Hi,

Is there any news regarding this issue?  When is it going to be fixed 
upstream?
I am using that last patch and it is working very well.

-- 
Olivier