From: Masaki Ota <masaki.ota@jp.alps.com>
To: Takashi Iwai <tiwai@suse.de>, Paul Donohue <linux-kernel@PaulSD.com>
CC: Laura Abbott <labbott@redhat.com>,
        Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        Pali Rohar <pali.rohar@gmail.com>,
        Nick Fletcher <nick.m.fletcher@gmail.com>,
        "linux-input@vger.kernel.org" <linux-input@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "scott.s.lowe@gmail.com" <scott.s.lowe@gmail.com>
Subject: RE: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS -
 fix V8+ protocol handling (73 03 28)")
Thread-Topic: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS -
 fix V8+ protocol handling (73 03 28)")
Thread-Index: AQHS28lZ6isdvv9kdkG/gMORaIsZVqIShTyAgAWPoYCACKbrMIAEC3aAgABT8qCAB4UPgIAAFhYAgAA3XACAIhV5gIAARVuAgAC9iICAAJo4gIAKgiQAgAGQgnA=
Date: Thu, 20 Jul 2017 09:20:20 +0000
Message-ID: <KAWPR01MB1092D56648C7759C1267DC77C7A70@KAWPR01MB1092.jpnprd01.prod.outlook.com>
References: <e841f28c-4d63-170a-482f-8ef1deda1900@redhat.com>
        <KAWPR01MB1092C3572946C54C2CC4DEFBC7CD0@KAWPR01MB1092.jpnprd01.prod.outlook.com>
        <75b84b97-5f36-a3be-bb4e-e43cd72d9921@redhat.com>
        <KAWPR01MB1092CFFDF4C4490669DCE92CC7C00@KAWPR01MB1092.jpnprd01.prod.outlook.com>
        <20170619184315.GF1855@TopQuark.net>
        <0995261b-8d19-64af-9b66-e1eab579a3d4@redhat.com>
        <20170619232026.GG1855@TopQuark.net>    <s5hwp7f3rdc.wl-tiwai@suse.de>
        <20170711195821.GG1855@TopQuark.net>    <s5hlgnuf7l0.wl-tiwai@suse.de>
        <20170712162841.GI1855@TopQuark.net> <s5hh8y8953z.wl-tiwai@suse.de>
In-Reply-To: <s5hh8y8953z.wl-tiwai@suse.de>
Accept-Language: ja-JP, en-US
Content-Language: ja-JP
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="iso-2022-jp"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Jul 2017 09:20:20.7842
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 57e76998-77bd-4b82-a424-198f46eb2254
X-MS-Exchange-Transport-CrossTenantHeadersStamped: KAWPR01MB1092
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id v6K9KbK2007742
Content-Length: 8323
Lines: 214

Hi, Takashi-san,

That's great! 

Actually, SS4 PLUS device is a little different from SS4 device.
I added SS4 PLUS code as below.
Could you try it?

And I still wonder why this issue does not happen on original code though X value is so large.

--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -1210,14 +1210,24 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
 
 	case SS4_PACKET_ID_TWO:
 		if (priv->flags & ALPS_BUTTONPAD) {
-			f->mt[0].x = SS4_BTL_MF_X_V2(p, 0);
+			if (IS_SS4PLUS_DEV(priv->dev_id)) {
+				f->mt[0].x = SS4_PLUS_BTL_MF_X_V2(p, 0);
+				f->mt[1].x = SS4_PLUS_BTL_MF_X_V2(p, 1);
+			} else {
+				f->mt[0].x = SS4_BTL_MF_X_V2(p, 0);
+				f->mt[1].x = SS4_BTL_MF_X_V2(p, 1);
+			}
 			f->mt[0].y = SS4_BTL_MF_Y_V2(p, 0);
-			f->mt[1].x = SS4_BTL_MF_X_V2(p, 1);
 			f->mt[1].y = SS4_BTL_MF_Y_V2(p, 1);
 		} else {
-			f->mt[0].x = SS4_STD_MF_X_V2(p, 0);
+			if (IS_SS4PLUS_DEV(priv->dev_id)) {
+				f->mt[0].x = SS4_PLUS_STD_MF_X_V2(p, 0);
+				f->mt[1].x = SS4_PLUS_STD_MF_X_V2(p, 1);
+			} else {
+				f->mt[0].x = SS4_STD_MF_X_V2(p, 0);
+				f->mt[1].x = SS4_STD_MF_X_V2(p, 1);
+			}
 			f->mt[0].y = SS4_STD_MF_Y_V2(p, 0);
-			f->mt[1].x = SS4_STD_MF_X_V2(p, 1);
 			f->mt[1].y = SS4_STD_MF_Y_V2(p, 1);
 		}
 		f->pressure = SS4_MF_Z_V2(p, 0) ? 0x30 : 0;
@@ -1234,16 +1244,27 @@ static int alps_decode_ss4_v2(struct alps_fields *f,
 
 	case SS4_PACKET_ID_MULTI:
 		if (priv->flags & ALPS_BUTTONPAD) {
-			f->mt[2].x = SS4_BTL_MF_X_V2(p, 0);
+			if (IS_SS4PLUS_DEV(priv->dev_id)) {
+				f->mt[0].x = SS4_PLUS_BTL_MF_X_V2(p, 0);
+				f->mt[1].x = SS4_PLUS_BTL_MF_X_V2(p, 1);
+			} else {
+				f->mt[2].x = SS4_BTL_MF_X_V2(p, 0);
+				f->mt[3].x = SS4_BTL_MF_X_V2(p, 1);
+			}
+
 			f->mt[2].y = SS4_BTL_MF_Y_V2(p, 0);
-			f->mt[3].x = SS4_BTL_MF_X_V2(p, 1);
 			f->mt[3].y = SS4_BTL_MF_Y_V2(p, 1);
 			no_data_x = SS4_MFPACKET_NO_AX_BL;
 			no_data_y = SS4_MFPACKET_NO_AY_BL;
 		} else {
-			f->mt[2].x = SS4_STD_MF_X_V2(p, 0);
+			if (IS_SS4PLUS_DEV(priv->dev_id)) {
+				f->mt[0].x = SS4_PLUS_STD_MF_X_V2(p, 0);
+				f->mt[1].x = SS4_PLUS_STD_MF_X_V2(p, 1);
+			} else {
+				f->mt[0].x = SS4_STD_MF_X_V2(p, 0);
+				f->mt[1].x = SS4_STD_MF_X_V2(p, 1);
+			}
 			f->mt[2].y = SS4_STD_MF_Y_V2(p, 0);
-			f->mt[3].x = SS4_STD_MF_X_V2(p, 1);
 			f->mt[3].y = SS4_STD_MF_Y_V2(p, 1);
 			no_data_x = SS4_MFPACKET_NO_AX;
 			no_data_y = SS4_MFPACKET_NO_AY;
@@ -2536,8 +2557,8 @@ static int alps_set_defaults_ss4_v2(struct psmouse *psmouse,
 
 	memset(otp, 0, sizeof(otp));
 
-	if (alps_get_otp_values_ss4_v2(psmouse, 0, &otp[0][0]) ||
-	    alps_get_otp_values_ss4_v2(psmouse, 1, &otp[1][0]))
+	if (alps_get_otp_values_ss4_v2(psmouse, 1, &otp[1][0]) ||
+	    alps_get_otp_values_ss4_v2(psmouse, 0, &otp[0][0]))
 		return -1;
 
 	alps_update_device_area_ss4_v2(otp, priv);
diff --git a/drivers/input/mouse/alps.h b/drivers/input/mouse/alps.h
index 4334f2805d93..75542199edda 100644
--- a/drivers/input/mouse/alps.h
+++ b/drivers/input/mouse/alps.h
@@ -99,6 +99,10 @@ enum SS4_PACKET_ID {
 				 ((_b[1 + _i * 3]  << 5) & 0x1F00)	\
 				)
 
+#define SS4_PLUS_STD_MF_X_V2(_b, _i) (((_b[0 + (_i) * 3] << 4) & 0x0070) | \
+				 ((_b[1 + (_i) * 3]  << 4) & 0x0F80)	\
+				)
+
 #define SS4_STD_MF_Y_V2(_b, _i)	(((_b[1 + (_i) * 3] << 3) & 0x0010) |	\
 				 ((_b[2 + (_i) * 3] << 5) & 0x01E0) |	\
 				 ((_b[2 + (_i) * 3] << 4) & 0x0E00)	\
@@ -108,6 +112,10 @@ enum SS4_PACKET_ID {
 				 ((_b[0 + (_i) * 3] >> 3) & 0x0010)	\
 				)
 
+#define SS4_PLUS_BTL_MF_X_V2(_b, _i) (SS4_PLUS_STD_MF_X_V2(_b, _i) |	\
+				 ((_b[0 + (_i) * 3] >> 4) & 0x0008)	\
+				)
+
 #define SS4_BTL_MF_Y_V2(_b, _i)	(SS4_STD_MF_Y_V2(_b, _i) | \
 				 ((_b[0 + (_i) * 3] >> 3) & 0x0008)	\
 				)
--

Best Regards,
Masaki Ota
-----Original Message-----
From: Takashi Iwai [mailto:tiwai@suse.de] 
Sent: Wednesday, July 19, 2017 5:57 PM
To: Paul Donohue <linux-kernel@PaulSD.com>
Cc: Laura Abbott <labbott@redhat.com>; 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>; Dmitry Torokhov <dmitry.torokhov@gmail.com>; Pali Rohar <pali.rohar@gmail.com>; Nick Fletcher <nick.m.fletcher@gmail.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; scott.s.lowe@gmail.com
Subject: Re: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)")

On Wed, 12 Jul 2017 18:28:41 +0200,
Paul Donohue wrote:
> 
> On Wed, Jul 12, 2017 at 09:16:43AM +0200, Takashi Iwai wrote:
> > On Tue, 11 Jul 2017 21:58:21 +0200,
> > Paul Donohue wrote:
> > > 
> > > On Tue, Jul 11, 2017 at 05:50:07PM +0200, Takashi Iwai wrote:
> > > > Hi, just joining to the party in the middle, as I'm also facing 
> > > > the same problem on Dell E7270 laptop.  Has this issue already 
> > > > been addressed?
> > > > 
> > > > If not, the following was my result:
> > > > 
> > > > - the first patch slowed the pointer movement a lot, it's even slower
> > > >   than the old kernel (e.g. 4.4.x).
> > > >   The two finger scroll works fine on all touchpad area now.
> > > > 
> > > > - the second patch made the pointer movement even faster than now (as
> > > >   I feel, not quite sure).  The two finger scroll doesn't work at the
> > > >   right side of the touchpad.
> > > > 
> > > > 
> > > > The kernel output from the first patch is below:
> > > >   psmouse serio1: alps: test1 pitch 37x37 num-electrodes 8x7 
> > > > physical size 25x22mm res 69x69 max 1792x1536
> > > > 
> > > > Let me know if you have any further test.
> > > > 
> > > > 
> > > > thanks,
> > > > 
> > > > Takashi
> > > 
> > > Do you have the kernel output from the second patch?
> > 
> > Here it is:
> > 
> > psmouse serio1: alps: test2 pitch 50x54 num-electrodes 20x11 
> > physical size 95x54mm res 25x23 max 2432x1280
> 
> Thanks!
> 
> Wow, this is weird ... Values that cause scrolling issues for you work fine for me.
> 
> It looks like the priv->x_max value that is causing your problem, but perhaps you can confirm this with another test:
> Revert the patches above, then just before the 'return' at the end of alps_update_device_area_ss4_v2() add the following line:
> priv->x_max = 1792;
> I suspect this line will make two-finger scrolling work again.
> 
> It might also help if you could experiment with other x_max values.  2432 is the correct value that causes problems for you..  Does a small value like 500 or a large value like 5000 change the behavior?
> 
> Another test you can try is to add a print statement in the 'case SS4_PACKET_ID_TWO' section of alps_decode_ss4_v2() and print out the f->mt[0].x and f->mt[1].x values.  With x_max set to 2432, what range of x values does this print when scrolling works, and what range of values are printed when scrolling doesn't work?

Sorry for the late reply.

I checked your suggestion, and noticed that actually x_max is a red herring.  Then I started looking at the output of evtest (should have done from the beginning!), and the problem became obvious: the reported X values for MT with finger > 1 are doubled, e.g. it reports
4800 instead of 2400.  The MT_Y coordinate looks OK, so it's only about MT_X.  And finger=1 reports correctly.

After reading the code, I ended up the patch like below.  This seems working through a quick test.  Could anyone check the patch to see whether it works / breaks anything?

The evtest showed that the MT_X for finger 3 and 4 are also bogus, and the patch should address it, too.

My only concern so far is whether correcting this globally is right, or it's specific to some certain models.  This needs the confirmation from ALPS people...


thanks,

Takashi


--- a/drivers/input/mouse/alps.h
+++ b/drivers/input/mouse/alps.h
@@ -96,8 +96,8 @@ enum SS4_PACKET_ID {
 
 #define SS4_BTN_V2(_b)		((_b[0] >> 5) & SS4_MASK_NORMAL_BUTTONS)
 
-#define SS4_STD_MF_X_V2(_b, _i)	(((_b[0 + (_i) * 3] << 5) & 0x00E0) |	\
-				 ((_b[1 + _i * 3]  << 5) & 0x1F00)	\
+#define SS4_STD_MF_X_V2(_b, _i)	(((_b[0 + (_i) * 3] << 4) & 0x0070) |	\
+				 ((_b[1 + (_i) * 3] << 4) & 0x0F80)	\
 				)
 
 #define SS4_STD_MF_Y_V2(_b, _i)	(((_b[1 + (_i) * 3] << 3) & 0x0010) |	\
@@ -106,7 +106,7 @@ enum SS4_PACKET_ID {
 				)
 
 #define SS4_BTL_MF_X_V2(_b, _i)	(SS4_STD_MF_X_V2(_b, _i) |		\
-				 ((_b[0 + (_i) * 3] >> 3) & 0x0010)	\
+				 ((_b[0 + (_i) * 3] >> 2) & 0x0008)	\
 				)
 
 #define SS4_BTL_MF_Y_V2(_b, _i)	(SS4_STD_MF_Y_V2(_b, _i) | \