[REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)")

[REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)")

[Laura Abbott]

Hi,

Fedora got a bug report https://bugzilla.redhat.com/show_bug.cgi?id=1447327
of a touchpad failure on a Dell Latitude E7370. Testing showed that the
bad commit was

commit e7348396c6d51b57c95c6646c390cd078e038e19
Author: Masaki Ota <masaki.ota@jp.alps.com>
Date:   Fri Mar 17 14:10:57 2017 -0700

    Input: ALPS - fix V8+ protocol handling (73 03 28)
    
    Devices identified as E7="73 03 28" use slightly modified version of V8
    protocol, with lower count per electrode, different offsets, and different
    feature bits in OTP data.
    
    Fixes: aeaa881f9b17 ("Input: ALPS - set DualPoint flag for 74 03 28 devices")
    Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
    Acked-by: Pali Rohar <pali.rohar@gmail.com>
    Tested-by: Paul Donohue <linux-kernel@PaulSD.com>
    Tested-by: Nick Fletcher <nick.m.fletcher@gmail.com>
    Cc: stable@vger.kernel.org
    Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

I suspect this particular model needs special handling as well?

Thanks,
Laura

Re: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)")

[Paul Donohue]

This might be related to https://bugzilla.kernel.org/show_bug.cgi?id=195215

Could you have the user try this change? https://bugzilla.kernel.org/show_bug.cgi?id=195215#c12

On Fri, Jun 02, 2017 at 10:54:52AM -0700, Laura Abbott wrote:
> Hi,
> 
> Fedora got a bug report https://bugzilla.redhat.com/show_bug.cgi?id=1447327
> of a touchpad failure on a Dell Latitude E7370. Testing showed that the
> bad commit was
> 
> commit e7348396c6d51b57c95c6646c390cd078e038e19
> Author: Masaki Ota <masaki.ota@jp.alps.com>
> Date:   Fri Mar 17 14:10:57 2017 -0700
> 
>     Input: ALPS - fix V8+ protocol handling (73 03 28)
>     
>     Devices identified as E7="73 03 28" use slightly modified version of V8
>     protocol, with lower count per electrode, different offsets, and different
>     feature bits in OTP data.
>     
>     Fixes: aeaa881f9b17 ("Input: ALPS - set DualPoint flag for 74 03 28 devices")
>     Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
>     Acked-by: Pali Rohar <pali.rohar@gmail.com>
>     Tested-by: Paul Donohue <linux-kernel@PaulSD.com>
>     Tested-by: Nick Fletcher <nick.m.fletcher@gmail.com>
>     Cc: stable@vger.kernel.org
>     Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> I suspect this particular model needs special handling as well?
> 
> Thanks,
> Laura
> 

Re: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)")

[Laura Abbott]

On 06/02/2017 09:03 PM, Paul Donohue wrote:
> This might be related to https://bugzilla.kernel.org/show_bug.cgi?id=195215
> 
> Could you have the user try this change? https://bugzilla.kernel.org/show_bug.cgi?id=195215#c12
> 

https://bugzilla.redhat.com/show_bug.cgi?id=1447327#c13

"Cursor movement seems to work, but there are intermittent two-finger scrolling
issues on the right-hand side of the touchpad. There are no issues with cursor
movement or two-finger scrolling on the left-hand side of the touchpad."

> On Fri, Jun 02, 2017 at 10:54:52AM -0700, Laura Abbott wrote:
>> Hi,
>>
>> Fedora got a bug report https://bugzilla.redhat.com/show_bug.cgi?id=1447327
>> of a touchpad failure on a Dell Latitude E7370. Testing showed that the
>> bad commit was
>>
>> commit e7348396c6d51b57c95c6646c390cd078e038e19
>> Author: Masaki Ota <masaki.ota@jp.alps.com>
>> Date:   Fri Mar 17 14:10:57 2017 -0700
>>
>>     Input: ALPS - fix V8+ protocol handling (73 03 28)
>>     
>>     Devices identified as E7="73 03 28" use slightly modified version of V8
>>     protocol, with lower count per electrode, different offsets, and different
>>     feature bits in OTP data.
>>     
>>     Fixes: aeaa881f9b17 ("Input: ALPS - set DualPoint flag for 74 03 28 devices")
>>     Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
>>     Acked-by: Pali Rohar <pali.rohar@gmail.com>
>>     Tested-by: Paul Donohue <linux-kernel@PaulSD.com>
>>     Tested-by: Nick Fletcher <nick.m.fletcher@gmail.com>
>>     Cc: stable@vger.kernel.org
>>     Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>
>> I suspect this particular model needs special handling as well?
>>
>> Thanks,
>> Laura
>>

RE: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)")

[Masaki Ota]

Hi, Laura,

Could you try to check below modification?
https://bugzilla.kernel.org/show_bug.cgi?id=195215#c10

This thread person said, the issue was fixed by this change.
I guess it's a XY coordinate setting problem, though the code that before the patch is applied also has a problem.

Best Regards,
Masaki Ota
-----Original Message-----
From: Laura Abbott [mailto:labbott@redhat.com] 
Sent: Wednesday, June 07, 2017 1:59 AM
To: Paul Donohue <linux-kernel@PaulSD.com>
Cc: 太田 真喜 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 06/02/2017 09:03 PM, Paul Donohue wrote:
> This might be related to 
> https://bugzilla.kernel.org/show_bug.cgi?id=195215
> 
> Could you have the user try this change? 
> https://bugzilla.kernel.org/show_bug.cgi?id=195215#c12
> 

https://bugzilla.redhat.com/show_bug.cgi?id=1447327#c13

"Cursor movement seems to work, but there are intermittent two-finger scrolling issues on the right-hand side of the touchpad. There are no issues with cursor movement or two-finger scrolling on the left-hand side of the touchpad."

> On Fri, Jun 02, 2017 at 10:54:52AM -0700, Laura Abbott wrote:
>> Hi,
>>
>> Fedora got a bug report 
>> https://bugzilla.redhat.com/show_bug.cgi?id=1447327
>> of a touchpad failure on a Dell Latitude E7370. Testing showed that 
>> the bad commit was
>>
>> commit e7348396c6d51b57c95c6646c390cd078e038e19
>> Author: Masaki Ota <masaki.ota@jp.alps.com>
>> Date:   Fri Mar 17 14:10:57 2017 -0700
>>
>>     Input: ALPS - fix V8+ protocol handling (73 03 28)
>>     
>>     Devices identified as E7="73 03 28" use slightly modified version of V8
>>     protocol, with lower count per electrode, different offsets, and different
>>     feature bits in OTP data.
>>     
>>     Fixes: aeaa881f9b17 ("Input: ALPS - set DualPoint flag for 74 03 28 devices")
>>     Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
>>     Acked-by: Pali Rohar <pali.rohar@gmail.com>
>>     Tested-by: Paul Donohue <linux-kernel@PaulSD.com>
>>     Tested-by: Nick Fletcher <nick.m.fletcher@gmail.com>
>>     Cc: stable@vger.kernel.org
>>     Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>
>> I suspect this particular model needs special handling as well?
>>
>> Thanks,
>> Laura
>>

Re: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)")

[Laura Abbott]

On 06/11/2017 10:25 PM, Masaki Ota wrote:
> Hi, Laura,
> 
> Could you try to check below modification?
> https://bugzilla.kernel.org/show_bug.cgi?id=195215#c10
> 
> This thread person said, the issue was fixed by this change.
> I guess it's a XY coordinate setting problem, though the code that before the patch is applied also has a problem.
> 

With the previous patch plus the part you suggested:

"it appears as if this resolves all remaining touchpad issues.
Cursor movement works as expected, both on the left-hand and right-hand
sides of the touchpad, and I did not see any issues with two-finger
scrolling on either side of the touchpad. Behavior with this test
build appeared to be identical to 4.10.5, the last "official" kernel
release to work with my touchpad."

So it sounds like both parts together fix the issue.

Thanks,
Laura

> Best Regards,
> Masaki Ota
> -----Original Message-----
> From: Laura Abbott [mailto:labbott@redhat.com] 
> Sent: Wednesday, June 07, 2017 1:59 AM
> To: Paul Donohue <linux-kernel@PaulSD.com>
> Cc: 太田 真喜 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 06/02/2017 09:03 PM, Paul Donohue wrote:
>> This might be related to 
>> https://bugzilla.kernel.org/show_bug.cgi?id=195215
>>
>> Could you have the user try this change? 
>> https://bugzilla.kernel.org/show_bug.cgi?id=195215#c12
>>
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1447327#c13
> 
> "Cursor movement seems to work, but there are intermittent two-finger scrolling issues on the right-hand side of the touchpad. There are no issues with cursor movement or two-finger scrolling on the left-hand side of the touchpad."
> 
>> On Fri, Jun 02, 2017 at 10:54:52AM -0700, Laura Abbott wrote:
>>> Hi,
>>>
>>> Fedora got a bug report 
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1447327
>>> of a touchpad failure on a Dell Latitude E7370. Testing showed that 
>>> the bad commit was
>>>
>>> commit e7348396c6d51b57c95c6646c390cd078e038e19
>>> Author: Masaki Ota <masaki.ota@jp.alps.com>
>>> Date:   Fri Mar 17 14:10:57 2017 -0700
>>>
>>>     Input: ALPS - fix V8+ protocol handling (73 03 28)
>>>     
>>>     Devices identified as E7="73 03 28" use slightly modified version of V8
>>>     protocol, with lower count per electrode, different offsets, and different
>>>     feature bits in OTP data.
>>>     
>>>     Fixes: aeaa881f9b17 ("Input: ALPS - set DualPoint flag for 74 03 28 devices")
>>>     Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
>>>     Acked-by: Pali Rohar <pali.rohar@gmail.com>
>>>     Tested-by: Paul Donohue <linux-kernel@PaulSD.com>
>>>     Tested-by: Nick Fletcher <nick.m.fletcher@gmail.com>
>>>     Cc: stable@vger.kernel.org
>>>     Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>
>>> I suspect this particular model needs special handling as well?
>>>
>>> Thanks,
>>> Laura
>>>
> 

RE: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)")

[Masaki Ota]

Hi, Paul, Dmitry,

About Laura's test result, it seems like this issue has to do with x_max, y_max, x_res, y_res.
This values are set as following code.
	input_set_abs_params(dev1, ABS_MT_POSITION_X, 0, priv->x_max, 0, 0);
	input_set_abs_params(dev1, ABS_MT_POSITION_Y, 0, priv->y_max, 0, 0);

	input_abs_set_res(dev1, ABS_MT_POSITION_X, priv->x_res);
	input_abs_set_res(dev1, ABS_MT_POSITION_Y, priv->y_res);

For testing this code, I assigned an abnormal value to x_max, y_max , and it seems to effect only cursor speed.
About x_res, y_res , there is no effect, even if I set an abnormal value.(need this code?)
I don't understand why these values have to do with this issue.

Can you guess the root cause of this issue?

Best Regards,
Masaki Ota
-----Original Message-----
From: Laura Abbott [mailto:labbott@redhat.com] 
Sent: Thursday, June 15, 2017 3:53 AM
To: 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>; Paul Donohue <linux-kernel@PaulSD.com>
Cc: 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 06/11/2017 10:25 PM, Masaki Ota wrote:
> Hi, Laura,
> 
> Could you try to check below modification?
> https://bugzilla.kernel.org/show_bug.cgi?id=195215#c10
> 
> This thread person said, the issue was fixed by this change.
> I guess it's a XY coordinate setting problem, though the code that before the patch is applied also has a problem.
> 

With the previous patch plus the part you suggested:

"it appears as if this resolves all remaining touchpad issues.
Cursor movement works as expected, both on the left-hand and right-hand sides of the touchpad, and I did not see any issues with two-finger scrolling on either side of the touchpad. Behavior with this test build appeared to be identical to 4.10.5, the last "official" kernel release to work with my touchpad."

So it sounds like both parts together fix the issue.

Thanks,
Laura

> Best Regards,
> Masaki Ota
> -----Original Message-----
> From: Laura Abbott [mailto:labbott@redhat.com]
> Sent: Wednesday, June 07, 2017 1:59 AM
> To: Paul Donohue <linux-kernel@PaulSD.com>
> Cc: 太田 真喜 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 06/02/2017 09:03 PM, Paul Donohue wrote:
>> This might be related to
>> https://bugzilla.kernel.org/show_bug.cgi?id=195215
>>
>> Could you have the user try this change? 
>> https://bugzilla.kernel.org/show_bug.cgi?id=195215#c12
>>
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1447327#c13
> 
> "Cursor movement seems to work, but there are intermittent two-finger scrolling issues on the right-hand side of the touchpad. There are no issues with cursor movement or two-finger scrolling on the left-hand side of the touchpad."
> 
>> On Fri, Jun 02, 2017 at 10:54:52AM -0700, Laura Abbott wrote:
>>> Hi,
>>>
>>> Fedora got a bug report
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1447327
>>> of a touchpad failure on a Dell Latitude E7370. Testing showed that 
>>> the bad commit was
>>>
>>> commit e7348396c6d51b57c95c6646c390cd078e038e19
>>> Author: Masaki Ota <masaki.ota@jp.alps.com>
>>> Date:   Fri Mar 17 14:10:57 2017 -0700
>>>
>>>     Input: ALPS - fix V8+ protocol handling (73 03 28)
>>>     
>>>     Devices identified as E7="73 03 28" use slightly modified version of V8
>>>     protocol, with lower count per electrode, different offsets, and different
>>>     feature bits in OTP data.
>>>     
>>>     Fixes: aeaa881f9b17 ("Input: ALPS - set DualPoint flag for 74 03 28 devices")
>>>     Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
>>>     Acked-by: Pali Rohar <pali.rohar@gmail.com>
>>>     Tested-by: Paul Donohue <linux-kernel@PaulSD.com>
>>>     Tested-by: Nick Fletcher <nick.m.fletcher@gmail.com>
>>>     Cc: stable@vger.kernel.org
>>>     Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>
>>> I suspect this particular model needs special handling as well?
>>>
>>> Thanks,
>>> Laura
>>>
> 

Re: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)")

[Paul Donohue]

I get the same results as you - x_max and y_max affect cursor speed, and x_res and y_res seem to have no effect.  I can't seem to come up with any values that cause intermittent cursor issues on one side of the touchpad.
Both max and res do get passed up to the X driver, and I do see references to both max and resolution in xf86-input-evdev, although I haven't actually traced it through to see if/where each value is actually consumed with my setup.

Maybe we should ask the user to try a few more tests?
1) Using the original code (without the modifications from bug 195215), add the following before 'return 0' at the end of alps_update_device_area_ss4_v2(), then run `dmesg | grep num-electrodes` after loading the alps kernel module to get the output.  This should tell us what values the user is actually reading from the hardware:
psmouse_err(psmouse,
    "pitch %dx%d num-electrodes %dx%d physical size %dx%dmm res %dx%d max %dx%d\n",
    x_pitch, y_pitch, num_x_electrode, num_y_electrode,
    x_phys / 10, y_phys / 10, priv->x_res, priv->y_res, priv->x_max, priv->y_max);
2) Use the old electrode count code but the new pitch code:
	if (IS_SS4PLUS_DEV(priv->dev_id)) {
		num_x_electrode =
			SS4_NUMSENSOR_XOFFSET + (otp[1][0] & 0x0F);
		num_y_electrode =
			SS4_NUMSENSOR_YOFFSET + ((otp[1][0] >> 4) & 0x0F);

		priv->x_max =
			(num_x_electrode - 1) * SS4_COUNT_PER_ELECTRODE;
		priv->y_max =
			(num_y_electrode - 1) * SS4_COUNT_PER_ELECTRODE;

		x_pitch = (otp[0][1] & 0x0F) + SS4PLUS_MIN_PITCH_MM;
		y_pitch = ((otp[0][1] >> 4) & 0x0F) + SS4PLUS_MIN_PITCH_MM;

	} else {
3) Use the new electrode count code but the old pitch code:
	if (IS_SS4PLUS_DEV(priv->dev_id)) {
		num_x_electrode =
			SS4PLUS_NUMSENSOR_XOFFSET + (otp[0][2] & 0x0F);
		num_y_electrode =
			SS4PLUS_NUMSENSOR_YOFFSET + ((otp[0][2] >> 4) & 0x0F);

		priv->x_max =
			(num_x_electrode - 1) * SS4PLUS_COUNT_PER_ELECTRODE;
		priv->y_max =
			(num_y_electrode - 1) * SS4PLUS_COUNT_PER_ELECTRODE;

		x_pitch = ((otp[1][2] >> 2) & 0x07) + SS4_MIN_PITCH_MM;
		y_pitch = ((otp[1][2] >> 5) & 0x07) + SS4_MIN_PITCH_MM;

	} else {

On Thu, Jun 15, 2017 at 12:33:29AM +0000, Masaki Ota wrote:
> Hi, Paul, Dmitry,
> 
> About Laura's test result, it seems like this issue has to do with x_max, y_max, x_res, y_res.
> This values are set as following code.
> 	input_set_abs_params(dev1, ABS_MT_POSITION_X, 0, priv->x_max, 0, 0);
> 	input_set_abs_params(dev1, ABS_MT_POSITION_Y, 0, priv->y_max, 0, 0);
> 
> 	input_abs_set_res(dev1, ABS_MT_POSITION_X, priv->x_res);
> 	input_abs_set_res(dev1, ABS_MT_POSITION_Y, priv->y_res);
> 
> For testing this code, I assigned an abnormal value to x_max, y_max , and it seems to effect only cursor speed.
> About x_res, y_res , there is no effect, even if I set an abnormal value.(need this code?)
> I don't understand why these values have to do with this issue.
> 
> Can you guess the root cause of this issue?
> 
> Best Regards,
> Masaki Ota
> -----Original Message-----
> From: Laura Abbott [mailto:labbott@redhat.com] 
> Sent: Thursday, June 15, 2017 3:53 AM
> To: 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>; Paul Donohue <linux-kernel@PaulSD.com>
> Cc: 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 06/11/2017 10:25 PM, Masaki Ota wrote:
> > Hi, Laura,
> > 
> > Could you try to check below modification?
> > https://bugzilla.kernel.org/show_bug.cgi?id=195215#c10
> > 
> > This thread person said, the issue was fixed by this change.
> > I guess it's a XY coordinate setting problem, though the code that before the patch is applied also has a problem.
> > 
> 
> With the previous patch plus the part you suggested:
> 
> "it appears as if this resolves all remaining touchpad issues.
> Cursor movement works as expected, both on the left-hand and right-hand sides of the touchpad, and I did not see any issues with two-finger scrolling on either side of the touchpad. Behavior with this test build appeared to be identical to 4.10.5, the last "official" kernel release to work with my touchpad."
> 
> So it sounds like both parts together fix the issue.
> 
> Thanks,
> Laura
> 
> > Best Regards,
> > Masaki Ota
> > -----Original Message-----
> > From: Laura Abbott [mailto:labbott@redhat.com]
> > Sent: Wednesday, June 07, 2017 1:59 AM
> > To: Paul Donohue <linux-kernel@PaulSD.com>
> > Cc: 太田 真喜 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 06/02/2017 09:03 PM, Paul Donohue wrote:
> >> This might be related to
> >> https://bugzilla.kernel.org/show_bug.cgi?id=195215
> >>
> >> Could you have the user try this change? 
> >> https://bugzilla.kernel.org/show_bug.cgi?id=195215#c12
> >>
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1447327#c13
> > 
> > "Cursor movement seems to work, but there are intermittent two-finger scrolling issues on the right-hand side of the touchpad. There are no issues with cursor movement or two-finger scrolling on the left-hand side of the touchpad."
> > 
> >> On Fri, Jun 02, 2017 at 10:54:52AM -0700, Laura Abbott wrote:
> >>> Hi,
> >>>
> >>> Fedora got a bug report
> >>> https://bugzilla.redhat.com/show_bug.cgi?id=1447327
> >>> of a touchpad failure on a Dell Latitude E7370. Testing showed that 
> >>> the bad commit was
> >>>
> >>> commit e7348396c6d51b57c95c6646c390cd078e038e19
> >>> Author: Masaki Ota <masaki.ota@jp.alps.com>
> >>> Date:   Fri Mar 17 14:10:57 2017 -0700
> >>>
> >>>     Input: ALPS - fix V8+ protocol handling (73 03 28)
> >>>     
> >>>     Devices identified as E7="73 03 28" use slightly modified version of V8
> >>>     protocol, with lower count per electrode, different offsets, and different
> >>>     feature bits in OTP data.
> >>>     
> >>>     Fixes: aeaa881f9b17 ("Input: ALPS - set DualPoint flag for 74 03 28 devices")
> >>>     Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
> >>>     Acked-by: Pali Rohar <pali.rohar@gmail.com>
> >>>     Tested-by: Paul Donohue <linux-kernel@PaulSD.com>
> >>>     Tested-by: Nick Fletcher <nick.m.fletcher@gmail.com>
> >>>     Cc: stable@vger.kernel.org
> >>>     Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >>>
> >>> I suspect this particular model needs special handling as well?
> >>>
> >>> Thanks,
> >>> Laura
> >>>
> > 
> 

Re: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)")

[Laura Abbott]

On 06/19/2017 11:43 AM, Paul Donohue wrote:
> I get the same results as you - x_max and y_max affect cursor speed, and x_res and y_res seem to have no effect.  I can't seem to come up with any values that cause intermittent cursor issues on one side of the touchpad.
> Both max and res do get passed up to the X driver, and I do see references to both max and resolution in xf86-input-evdev, although I haven't actually traced it through to see if/where each value is actually consumed with my setup.
> 
> Maybe we should ask the user to try a few more tests?
> 1) Using the original code (without the modifications from bug 195215), add the following before 'return 0' at the end of alps_update_device_area_ss4_v2(), then run `dmesg | grep num-electrodes` after loading the alps kernel module to get the output.  This should tell us what values the user is actually reading from the hardware:
> psmouse_err(psmouse,
>     "pitch %dx%d num-electrodes %dx%d physical size %dx%dmm res %dx%d max %dx%d\n",
>     x_pitch, y_pitch, num_x_electrode, num_y_electrode,
>     x_phys / 10, y_phys / 10, priv->x_res, priv->y_res, priv->x_max, priv->y_max);
> 2) Use the old electrode count code but the new pitch code:
> 	if (IS_SS4PLUS_DEV(priv->dev_id)) {
> 		num_x_electrode =
> 			SS4_NUMSENSOR_XOFFSET + (otp[1][0] & 0x0F);
> 		num_y_electrode =
> 			SS4_NUMSENSOR_YOFFSET + ((otp[1][0] >> 4) & 0x0F);
> 
> 		priv->x_max =
> 			(num_x_electrode - 1) * SS4_COUNT_PER_ELECTRODE;
> 		priv->y_max =
> 			(num_y_electrode - 1) * SS4_COUNT_PER_ELECTRODE;
> 
> 		x_pitch = (otp[0][1] & 0x0F) + SS4PLUS_MIN_PITCH_MM;
> 		y_pitch = ((otp[0][1] >> 4) & 0x0F) + SS4PLUS_MIN_PITCH_MM;
> 
> 	} else {
> 3) Use the new electrode count code but the old pitch code:
> 	if (IS_SS4PLUS_DEV(priv->dev_id)) {
> 		num_x_electrode =
> 			SS4PLUS_NUMSENSOR_XOFFSET + (otp[0][2] & 0x0F);
> 		num_y_electrode =
> 			SS4PLUS_NUMSENSOR_YOFFSET + ((otp[0][2] >> 4) & 0x0F);
> 
> 		priv->x_max =
> 			(num_x_electrode - 1) * SS4PLUS_COUNT_PER_ELECTRODE;
> 		priv->y_max =
> 			(num_y_electrode - 1) * SS4PLUS_COUNT_PER_ELECTRODE;
> 
> 		x_pitch = ((otp[1][2] >> 2) & 0x07) + SS4_MIN_PITCH_MM;
> 		y_pitch = ((otp[1][2] >> 5) & 0x07) + SS4_MIN_PITCH_MM;
> 
> 	} else {
> 

Can you produce patches for these test cases?

> On Thu, Jun 15, 2017 at 12:33:29AM +0000, Masaki Ota wrote:
>> Hi, Paul, Dmitry,
>>
>> About Laura's test result, it seems like this issue has to do with x_max, y_max, x_res, y_res.
>> This values are set as following code.
>> 	input_set_abs_params(dev1, ABS_MT_POSITION_X, 0, priv->x_max, 0, 0);
>> 	input_set_abs_params(dev1, ABS_MT_POSITION_Y, 0, priv->y_max, 0, 0);
>>
>> 	input_abs_set_res(dev1, ABS_MT_POSITION_X, priv->x_res);
>> 	input_abs_set_res(dev1, ABS_MT_POSITION_Y, priv->y_res);
>>
>> For testing this code, I assigned an abnormal value to x_max, y_max , and it seems to effect only cursor speed.
>> About x_res, y_res , there is no effect, even if I set an abnormal value.(need this code?)
>> I don't understand why these values have to do with this issue.
>>
>> Can you guess the root cause of this issue?
>>
>> Best Regards,
>> Masaki Ota
>> -----Original Message-----
>> From: Laura Abbott [mailto:labbott@redhat.com] 
>> Sent: Thursday, June 15, 2017 3:53 AM
>> To: 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>; Paul Donohue <linux-kernel@PaulSD.com>
>> Cc: 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 06/11/2017 10:25 PM, Masaki Ota wrote:
>>> Hi, Laura,
>>>
>>> Could you try to check below modification?
>>> https://bugzilla.kernel.org/show_bug.cgi?id=195215#c10
>>>
>>> This thread person said, the issue was fixed by this change.
>>> I guess it's a XY coordinate setting problem, though the code that before the patch is applied also has a problem.
>>>
>>
>> With the previous patch plus the part you suggested:
>>
>> "it appears as if this resolves all remaining touchpad issues.
>> Cursor movement works as expected, both on the left-hand and right-hand sides of the touchpad, and I did not see any issues with two-finger scrolling on either side of the touchpad. Behavior with this test build appeared to be identical to 4.10.5, the last "official" kernel release to work with my touchpad."
>>
>> So it sounds like both parts together fix the issue.
>>
>> Thanks,
>> Laura
>>
>>> Best Regards,
>>> Masaki Ota
>>> -----Original Message-----
>>> From: Laura Abbott [mailto:labbott@redhat.com]
>>> Sent: Wednesday, June 07, 2017 1:59 AM
>>> To: Paul Donohue <linux-kernel@PaulSD.com>
>>> Cc: 太田 真喜 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 06/02/2017 09:03 PM, Paul Donohue wrote:
>>>> This might be related to
>>>> https://bugzilla.kernel.org/show_bug.cgi?id=195215
>>>>
>>>> Could you have the user try this change? 
>>>> https://bugzilla.kernel.org/show_bug.cgi?id=195215#c12
>>>>
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1447327#c13
>>>
>>> "Cursor movement seems to work, but there are intermittent two-finger scrolling issues on the right-hand side of the touchpad. There are no issues with cursor movement or two-finger scrolling on the left-hand side of the touchpad."
>>>
>>>> On Fri, Jun 02, 2017 at 10:54:52AM -0700, Laura Abbott wrote:
>>>>> Hi,
>>>>>
>>>>> Fedora got a bug report
>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1447327
>>>>> of a touchpad failure on a Dell Latitude E7370. Testing showed that 
>>>>> the bad commit was
>>>>>
>>>>> commit e7348396c6d51b57c95c6646c390cd078e038e19
>>>>> Author: Masaki Ota <masaki.ota@jp.alps.com>
>>>>> Date:   Fri Mar 17 14:10:57 2017 -0700
>>>>>
>>>>>     Input: ALPS - fix V8+ protocol handling (73 03 28)
>>>>>     
>>>>>     Devices identified as E7="73 03 28" use slightly modified version of V8
>>>>>     protocol, with lower count per electrode, different offsets, and different
>>>>>     feature bits in OTP data.
>>>>>     
>>>>>     Fixes: aeaa881f9b17 ("Input: ALPS - set DualPoint flag for 74 03 28 devices")
>>>>>     Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com>
>>>>>     Acked-by: Pali Rohar <pali.rohar@gmail.com>
>>>>>     Tested-by: Paul Donohue <linux-kernel@PaulSD.com>
>>>>>     Tested-by: Nick Fletcher <nick.m.fletcher@gmail.com>
>>>>>     Cc: stable@vger.kernel.org
>>>>>     Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>>>
>>>>> I suspect this particular model needs special handling as well?
>>>>>
>>>>> Thanks,
>>>>> Laura
>>>>>
>>>
>>

Re: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)")

[Paul Donohue]

[-- Attachment #1: Type: text/plain, Size: 2491 bytes --]

On Mon, Jun 19, 2017 at 01:02:18PM -0700, Laura Abbott wrote:
> On 06/19/2017 11:43 AM, Paul Donohue wrote:
> > I get the same results as you - x_max and y_max affect cursor speed, and x_res and y_res seem to have no effect.  I can't seem to come up with any values that cause intermittent cursor issues on one side of the touchpad.
> > Both max and res do get passed up to the X driver, and I do see references to both max and resolution in xf86-input-evdev, although I haven't actually traced it through to see if/where each value is actually consumed with my setup.
> > 
> > Maybe we should ask the user to try a few more tests?
> > 1) Using the original code (without the modifications from bug 195215), add the following before 'return 0' at the end of alps_update_device_area_ss4_v2(), then run `dmesg | grep num-electrodes` after loading the alps kernel module to get the output.  This should tell us what values the user is actually reading from the hardware:
> > psmouse_err(psmouse,
> >     "pitch %dx%d num-electrodes %dx%d physical size %dx%dmm res %dx%d max %dx%d\n",
> >     x_pitch, y_pitch, num_x_electrode, num_y_electrode,
> >     x_phys / 10, y_phys / 10, priv->x_res, priv->y_res, priv->x_max, priv->y_max);
> > 2) Use the old electrode count code but the new pitch code:
> > 	if (IS_SS4PLUS_DEV(priv->dev_id)) {
> > 		num_x_electrode =
> > 			SS4_NUMSENSOR_XOFFSET + (otp[1][0] & 0x0F);
> > 		num_y_electrode =
> > 			SS4_NUMSENSOR_YOFFSET + ((otp[1][0] >> 4) & 0x0F);
> > 
> > 		priv->x_max =
> > 			(num_x_electrode - 1) * SS4_COUNT_PER_ELECTRODE;
> > 		priv->y_max =
> > 			(num_y_electrode - 1) * SS4_COUNT_PER_ELECTRODE;
> > 
> > 		x_pitch = (otp[0][1] & 0x0F) + SS4PLUS_MIN_PITCH_MM;
> > 		y_pitch = ((otp[0][1] >> 4) & 0x0F) + SS4PLUS_MIN_PITCH_MM;
> > 
> > 	} else {
> > 3) Use the new electrode count code but the old pitch code:
> > 	if (IS_SS4PLUS_DEV(priv->dev_id)) {
> > 		num_x_electrode =
> > 			SS4PLUS_NUMSENSOR_XOFFSET + (otp[0][2] & 0x0F);
> > 		num_y_electrode =
> > 			SS4PLUS_NUMSENSOR_YOFFSET + ((otp[0][2] >> 4) & 0x0F);
> > 
> > 		priv->x_max =
> > 			(num_x_electrode - 1) * SS4PLUS_COUNT_PER_ELECTRODE;
> > 		priv->y_max =
> > 			(num_y_electrode - 1) * SS4PLUS_COUNT_PER_ELECTRODE;
> > 
> > 		x_pitch = ((otp[1][2] >> 2) & 0x07) + SS4_MIN_PITCH_MM;
> > 		y_pitch = ((otp[1][2] >> 5) & 0x07) + SS4_MIN_PITCH_MM;
> > 
> > 	} else {
> > 
> 
> Can you produce patches for these test cases?

I've reduced it to two test cases.  Patches attached.


[-- Attachment #2: patch1 --]
[-- Type: text/plain, Size: 1429 bytes --]

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 262d1057c1da..341a6c3bf907 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -2467,14 +2467,14 @@ static int alps_update_device_area_ss4_v2(unsigned char otp[][4],
 
 	if (IS_SS4PLUS_DEV(priv->dev_id)) {
 		num_x_electrode =
-			SS4PLUS_NUMSENSOR_XOFFSET + (otp[0][2] & 0x0F);
+			SS4_NUMSENSOR_XOFFSET + (otp[1][0] & 0x0F);
 		num_y_electrode =
-			SS4PLUS_NUMSENSOR_YOFFSET + ((otp[0][2] >> 4) & 0x0F);
+			SS4_NUMSENSOR_YOFFSET + ((otp[1][0] >> 4) & 0x0F);
 
 		priv->x_max =
-			(num_x_electrode - 1) * SS4PLUS_COUNT_PER_ELECTRODE;
+			(num_x_electrode - 1) * SS4_COUNT_PER_ELECTRODE;
 		priv->y_max =
-			(num_y_electrode - 1) * SS4PLUS_COUNT_PER_ELECTRODE;
+			(num_y_electrode - 1) * SS4_COUNT_PER_ELECTRODE;
 
 		x_pitch = (otp[0][1] & 0x0F) + SS4PLUS_MIN_PITCH_MM;
 		y_pitch = ((otp[0][1] >> 4) & 0x0F) + SS4PLUS_MIN_PITCH_MM;
@@ -2500,6 +2500,11 @@ static int alps_update_device_area_ss4_v2(unsigned char otp[][4],
 	priv->x_res = priv->x_max * 10 / x_phys; /* units / mm */
 	priv->y_res = priv->y_max * 10 / y_phys; /* units / mm */
 
+	psmouse_err(psmouse,
+		    "test1 pitch %dx%d num-electrodes %dx%d physical size %dx%dmm res %dx%d max %dx%d\n",
+		    x_pitch, y_pitch, num_x_electrode, num_y_electrode,
+		    x_phys / 10, y_phys / 10, priv->x_res, priv->y_res, priv->x_max, priv->y_max);
+
 	return 0;
 }
 

[-- Attachment #3: patch2 --]
[-- Type: text/plain, Size: 1101 bytes --]

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 262d1057c1da..f57dca5cecd5 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -2476,8 +2476,8 @@ static int alps_update_device_area_ss4_v2(unsigned char otp[][4],
 		priv->y_max =
 			(num_y_electrode - 1) * SS4PLUS_COUNT_PER_ELECTRODE;
 
-		x_pitch = (otp[0][1] & 0x0F) + SS4PLUS_MIN_PITCH_MM;
-		y_pitch = ((otp[0][1] >> 4) & 0x0F) + SS4PLUS_MIN_PITCH_MM;
+		x_pitch = ((otp[1][2] >> 2) & 0x07) + SS4_MIN_PITCH_MM;
+		y_pitch = ((otp[1][2] >> 5) & 0x07) + SS4_MIN_PITCH_MM;
 
 	} else {
 		num_x_electrode =
@@ -2500,6 +2500,11 @@ static int alps_update_device_area_ss4_v2(unsigned char otp[][4],
 	priv->x_res = priv->x_max * 10 / x_phys; /* units / mm */
 	priv->y_res = priv->y_max * 10 / y_phys; /* units / mm */
 
+	psmouse_err(psmouse,
+		    "test2 pitch %dx%d num-electrodes %dx%d physical size %dx%dmm res %dx%d max %dx%d\n",
+		    x_pitch, y_pitch, num_x_electrode, num_y_electrode,
+		    x_phys / 10, y_phys / 10, priv->x_res, priv->y_res, priv->x_max, priv->y_max);
+
 	return 0;
 }
 

Re: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)")

[Takashi Iwai]

On Tue, 20 Jun 2017 01:20:26 +0200,
Paul Donohue wrote:
> 
> On Mon, Jun 19, 2017 at 01:02:18PM -0700, Laura Abbott wrote:
> > On 06/19/2017 11:43 AM, Paul Donohue wrote:
> > > I get the same results as you - x_max and y_max affect cursor speed, and x_res and y_res seem to have no effect.  I can't seem to come up with any values that cause intermittent cursor issues on one side of the touchpad.
> > > Both max and res do get passed up to the X driver, and I do see references to both max and resolution in xf86-input-evdev, although I haven't actually traced it through to see if/where each value is actually consumed with my setup.
> > > 
> > > Maybe we should ask the user to try a few more tests?
> > > 1) Using the original code (without the modifications from bug 195215), add the following before 'return 0' at the end of alps_update_device_area_ss4_v2(), then run `dmesg | grep num-electrodes` after loading the alps kernel module to get the output.  This should tell us what values the user is actually reading from the hardware:
> > > psmouse_err(psmouse,
> > >     "pitch %dx%d num-electrodes %dx%d physical size %dx%dmm res %dx%d max %dx%d\n",
> > >     x_pitch, y_pitch, num_x_electrode, num_y_electrode,
> > >     x_phys / 10, y_phys / 10, priv->x_res, priv->y_res, priv->x_max, priv->y_max);
> > > 2) Use the old electrode count code but the new pitch code:
> > > 	if (IS_SS4PLUS_DEV(priv->dev_id)) {
> > > 		num_x_electrode =
> > > 			SS4_NUMSENSOR_XOFFSET + (otp[1][0] & 0x0F);
> > > 		num_y_electrode =
> > > 			SS4_NUMSENSOR_YOFFSET + ((otp[1][0] >> 4) & 0x0F);
> > > 
> > > 		priv->x_max =
> > > 			(num_x_electrode - 1) * SS4_COUNT_PER_ELECTRODE;
> > > 		priv->y_max =
> > > 			(num_y_electrode - 1) * SS4_COUNT_PER_ELECTRODE;
> > > 
> > > 		x_pitch = (otp[0][1] & 0x0F) + SS4PLUS_MIN_PITCH_MM;
> > > 		y_pitch = ((otp[0][1] >> 4) & 0x0F) + SS4PLUS_MIN_PITCH_MM;
> > > 
> > > 	} else {
> > > 3) Use the new electrode count code but the old pitch code:
> > > 	if (IS_SS4PLUS_DEV(priv->dev_id)) {
> > > 		num_x_electrode =
> > > 			SS4PLUS_NUMSENSOR_XOFFSET + (otp[0][2] & 0x0F);
> > > 		num_y_electrode =
> > > 			SS4PLUS_NUMSENSOR_YOFFSET + ((otp[0][2] >> 4) & 0x0F);
> > > 
> > > 		priv->x_max =
> > > 			(num_x_electrode - 1) * SS4PLUS_COUNT_PER_ELECTRODE;
> > > 		priv->y_max =
> > > 			(num_y_electrode - 1) * SS4PLUS_COUNT_PER_ELECTRODE;
> > > 
> > > 		x_pitch = ((otp[1][2] >> 2) & 0x07) + SS4_MIN_PITCH_MM;
> > > 		y_pitch = ((otp[1][2] >> 5) & 0x07) + SS4_MIN_PITCH_MM;
> > > 
> > > 	} else {
> > > 
> > 
> > Can you produce patches for these test cases?
> 
> I've reduced it to two test cases.  Patches attached.

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

Re: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)")

[Paul Donohue]

On Tue, Jul 11, 2017 at 05:50:07PM +0200, Takashi Iwai wrote:
> On Tue, 20 Jun 2017 01:20:26 +0200,
> Paul Donohue wrote:
> > 
> > On Mon, Jun 19, 2017 at 01:02:18PM -0700, Laura Abbott wrote:
> > > On 06/19/2017 11:43 AM, Paul Donohue wrote:
> > > > I get the same results as you - x_max and y_max affect cursor speed, and x_res and y_res seem to have no effect.  I can't seem to come up with any values that cause intermittent cursor issues on one side of the touchpad.
> > > > Both max and res do get passed up to the X driver, and I do see references to both max and resolution in xf86-input-evdev, although I haven't actually traced it through to see if/where each value is actually consumed with my setup.
> > > > 
> > > > Maybe we should ask the user to try a few more tests?
> > > > 1) Using the original code (without the modifications from bug 195215), add the following before 'return 0' at the end of alps_update_device_area_ss4_v2(), then run `dmesg | grep num-electrodes` after loading the alps kernel module to get the output.  This should tell us what values the user is actually reading from the hardware:
> > > > psmouse_err(psmouse,
> > > >     "pitch %dx%d num-electrodes %dx%d physical size %dx%dmm res %dx%d max %dx%d\n",
> > > >     x_pitch, y_pitch, num_x_electrode, num_y_electrode,
> > > >     x_phys / 10, y_phys / 10, priv->x_res, priv->y_res, priv->x_max, priv->y_max);
> > > > 2) Use the old electrode count code but the new pitch code:
> > > > 	if (IS_SS4PLUS_DEV(priv->dev_id)) {
> > > > 		num_x_electrode =
> > > > 			SS4_NUMSENSOR_XOFFSET + (otp[1][0] & 0x0F);
> > > > 		num_y_electrode =
> > > > 			SS4_NUMSENSOR_YOFFSET + ((otp[1][0] >> 4) & 0x0F);
> > > > 
> > > > 		priv->x_max =
> > > > 			(num_x_electrode - 1) * SS4_COUNT_PER_ELECTRODE;
> > > > 		priv->y_max =
> > > > 			(num_y_electrode - 1) * SS4_COUNT_PER_ELECTRODE;
> > > > 
> > > > 		x_pitch = (otp[0][1] & 0x0F) + SS4PLUS_MIN_PITCH_MM;
> > > > 		y_pitch = ((otp[0][1] >> 4) & 0x0F) + SS4PLUS_MIN_PITCH_MM;
> > > > 
> > > > 	} else {
> > > > 3) Use the new electrode count code but the old pitch code:
> > > > 	if (IS_SS4PLUS_DEV(priv->dev_id)) {
> > > > 		num_x_electrode =
> > > > 			SS4PLUS_NUMSENSOR_XOFFSET + (otp[0][2] & 0x0F);
> > > > 		num_y_electrode =
> > > > 			SS4PLUS_NUMSENSOR_YOFFSET + ((otp[0][2] >> 4) & 0x0F);
> > > > 
> > > > 		priv->x_max =
> > > > 			(num_x_electrode - 1) * SS4PLUS_COUNT_PER_ELECTRODE;
> > > > 		priv->y_max =
> > > > 			(num_y_electrode - 1) * SS4PLUS_COUNT_PER_ELECTRODE;
> > > > 
> > > > 		x_pitch = ((otp[1][2] >> 2) & 0x07) + SS4_MIN_PITCH_MM;
> > > > 		y_pitch = ((otp[1][2] >> 5) & 0x07) + SS4_MIN_PITCH_MM;
> > > > 
> > > > 	} else {
> > > > 
> > > 
> > > Can you produce patches for these test cases?
> > 
> > I've reduced it to two test cases.  Patches attached.
> 
> 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?

Thanks!
-Paul

Re: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)")

[Takashi Iwai]

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


Takashi

Re: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)")

[Paul Donohue]

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?

Thanks,
-Paul

Re: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)")

[Takashi Iwai]

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) | \

RE: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)")

[Masaki Ota]

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) | \

Re: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)")

[Takashi Iwai]

On Thu, 20 Jul 2017 11:20:20 +0200,
Masaki Ota wrote:
> 
> 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?

Yes, it works as expected.  Feel free to take:
  Tested-by: Takashi Iwai <tiwai@suse.de>

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

Which original code are you referring to?
The bug manifests itself only for decoding multi-touch events.


thanks,

Takashi

> 
> --- 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) | \
> 

RE: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)")

[Masaki Ota]

Hi, 

Some user reported that this issue happened from kernel 4.10.7.
Because the below patch was applied on it.
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=0186e6a4e501d39f5f90dd7e5887bc668aef06c4

So, I thought the cause of this issue is this patch.
Original code meaning is Kernel 4.10.6, it seems not to have this issue.

Best Regards,
Masaki Ota
-----Original Message-----
From: Takashi Iwai [mailto:tiwai@suse.de] 
Sent: Thursday, July 20, 2017 6:35 PM
To: 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>
Cc: Paul Donohue <linux-kernel@PaulSD.com>; 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-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 Thu, 20 Jul 2017 11:20:20 +0200,
Masaki Ota wrote:
> 
> 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?

Yes, it works as expected.  Feel free to take:
  Tested-by: Takashi Iwai <tiwai@suse.de>

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

Which original code are you referring to?
The bug manifests itself only for decoding multi-touch events.


thanks,

Takashi

> 
> --- 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) | \
> 

Re: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)")

[Takashi Iwai]

On Thu, 20 Jul 2017 12:02:10 +0200,
Masaki Ota wrote:
> 
> Hi, 
> 
> Some user reported that this issue happened from kernel 4.10.7.
> Because the below patch was applied on it.
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=0186e6a4e501d39f5f90dd7e5887bc668aef06c4
> 
> So, I thought the cause of this issue is this patch.
> Original code meaning is Kernel 4.10.6, it seems not to have this issue.

The bug has been present from the beginning, but it didn't surface
until another commit to change x_max and co because MT feature is
mostly used only for scrolling and such simple behavior.  For
scrolling, even the bogus MT_X value could work casually with the
certain x_max value used in the original commit.
Actually, by fiddling with x_max (either less than 2200 or greater
than 3000), the two-finger scroll "works", although it shouldn't.


Takashi

> 
> Best Regards,
> Masaki Ota
> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de] 
> Sent: Thursday, July 20, 2017 6:35 PM
> To: 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>
> Cc: Paul Donohue <linux-kernel@PaulSD.com>; 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-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 Thu, 20 Jul 2017 11:20:20 +0200,
> Masaki Ota wrote:
> > 
> > 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?
> 
> Yes, it works as expected.  Feel free to take:
>   Tested-by: Takashi Iwai <tiwai@suse.de>
> 
> > And I still wonder why this issue does not happen on original code though X value is so large.
> 
> Which original code are you referring to?
> The bug manifests itself only for decoding multi-touch events.
> 
> 
> thanks,
> 
> Takashi
> 
> > 
> > --- 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) | \
> > 

Re: [REGRESSION] Touchpad failure after e7348396c6d5 ("Input: ALPS - fix V8+ protocol handling (73 03 28)")

[Paul Donohue]

Hmm... I see the same doubled X values in evtest, but everything works fine regardless of what value I set x_max to.
Maybe the issue is related to the window manager?  I've tried it under both Compiz and Xfwm with the same results.
Regardless, the latest patch works for me.

Thanks!

-Paul

On Thu, Jul 20, 2017 at 12:43:10PM +0200, Takashi Iwai wrote:
> On Thu, 20 Jul 2017 12:02:10 +0200,
> Masaki Ota wrote:
> > 
> > Hi, 
> > 
> > Some user reported that this issue happened from kernel 4.10.7.
> > Because the below patch was applied on it.
> > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=0186e6a4e501d39f5f90dd7e5887bc668aef06c4
> > 
> > So, I thought the cause of this issue is this patch.
> > Original code meaning is Kernel 4.10.6, it seems not to have this issue.
> 
> The bug has been present from the beginning, but it didn't surface
> until another commit to change x_max and co because MT feature is
> mostly used only for scrolling and such simple behavior.  For
> scrolling, even the bogus MT_X value could work casually with the
> certain x_max value used in the original commit.
> Actually, by fiddling with x_max (either less than 2200 or greater
> than 3000), the two-finger scroll "works", although it shouldn't.
> 
> 
> Takashi
> 
> > 
> > Best Regards,
> > Masaki Ota
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de] 
> > Sent: Thursday, July 20, 2017 6:35 PM
> > To: 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>
> > Cc: Paul Donohue <linux-kernel@PaulSD.com>; 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-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 Thu, 20 Jul 2017 11:20:20 +0200,
> > Masaki Ota wrote:
> > > 
> > > 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?
> > 
> > Yes, it works as expected.  Feel free to take:
> >   Tested-by: Takashi Iwai <tiwai@suse.de>
> > 
> > > And I still wonder why this issue does not happen on original code though X value is so large.
> > 
> > Which original code are you referring to?
> > The bug manifests itself only for decoding multi-touch events.
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > > 
> > > --- 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) | \
> > > 
>