diff mbox

Revert "Input: trackpoint - add new trackpoint firmware ID"

Message ID 20171230152213.GA2099@marax.lan.yath.de (mailing list archive)
State New, archived
Headers show

Commit Message

Sebastian Schmidt Dec. 30, 2017, 3:22 p.m. UTC 
This reverts commit ec667683c532c93fb41e100e5d61a518971060e2, which
breaks the Trackpoint on ThinkPad X1 Carbon Gen5 (Model 20HR). That
commit intended to add support for later firmware versions to the
trackpoint driver, however, the version is reported in the second byte
whereas the change was made to the magic byte preceding that version.
The update package linked by Lenovo suggests that 20HR models use an
ALPS Touchpad instead.

Signed-off-by: Sebastian Schmidt <yath@yath.de>
Acked-by: Greg KH <gregkh@linuxfoundation.org>
Cc: stable@vger.kernel.org
---
 drivers/input/mouse/trackpoint.c | 3 +--
 drivers/input/mouse/trackpoint.h | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

Comments

Greg Kroah-Hartman Dec. 30, 2017, 3:32 p.m. UTC | #1 
On Sat, Dec 30, 2017 at 04:22:13PM +0100, Sebastian Schmidt wrote:
> This reverts commit ec667683c532c93fb41e100e5d61a518971060e2, which
> breaks the Trackpoint on ThinkPad X1 Carbon Gen5 (Model 20HR). That
> commit intended to add support for later firmware versions to the
> trackpoint driver, however, the version is reported in the second byte
> whereas the change was made to the magic byte preceding that version.
> The update package linked by Lenovo suggests that 20HR models use an
> ALPS Touchpad instead.
> 
> Signed-off-by: Sebastian Schmidt <yath@yath.de>
> Acked-by: Greg KH <gregkh@linuxfoundation.org>

Um, I didn't ack this patch, don't ever put something like that on a
patch unless it is offered by the person.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sebastian Schmidt Dec. 30, 2017, 3:41 p.m. UTC | #2 
On Sat, Dec 30, 2017 at 04:32:13PM +0100, Greg KH wrote:
> On Sat, Dec 30, 2017 at 04:22:13PM +0100, Sebastian Schmidt wrote:
> > This reverts commit ec667683c532c93fb41e100e5d61a518971060e2, which
> > breaks the Trackpoint on ThinkPad X1 Carbon Gen5 (Model 20HR). That
> > commit intended to add support for later firmware versions to the
> > trackpoint driver, however, the version is reported in the second byte
> > whereas the change was made to the magic byte preceding that version.
> > The update package linked by Lenovo suggests that 20HR models use an
> > ALPS Touchpad instead.
> > 
> > Signed-off-by: Sebastian Schmidt <yath@yath.de>
> > Acked-by: Greg KH <gregkh@linuxfoundation.org>
> 
> Um, I didn't ack this patch, don't ever put something like that on a
> patch unless it is offered by the person.

Sorry. :( Should I re-send the amended patch?

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aaron Ma Dec. 31, 2017, 4:37 a.m. UTC | #3 
Please refer to the Lenovo forum:

https://forums.lenovo.com/t5/forums/v3_1/forumtopicpage/board-id/Special_Interest_Linux/thread-id/9645/page/23

The patch you tried to revert solved many people's issue.
Also the people who used the trackpointdetect.exe to identify their ID
as (Elan (ID:03).

The elan driver can *not* work on their trackpoint, neither does ALPS.
They didn't match the ID tables, commands on ALPS or Elan driver.

The error log of Windows firmware update tools maybe wrong, so do NOT
based on it to guess what device it is.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greg Kroah-Hartman Dec. 31, 2017, 8:26 a.m. UTC | #4 
On Sun, Dec 31, 2017 at 12:37:56PM +0800, Aaron Ma wrote:
> Please refer to the Lenovo forum:

Please always include the proper email context, I have no idea what
you are talking about here.  Remember, some people get hundreds, if not
thousands, of emails a day...

thnaks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aaron Ma Dec. 31, 2017, 8:51 a.m. UTC | #5 
1, Commit ec667683c53 addes support of the new trackpoint.
The trackpoint can be identified as "TPPS/2 IBM TrackPoint" and set
properties OK. Trackpoint enabled some enhanced features.
Without this commit there will be no scroll mode when xf86-input-evdev
is used.

2, trackpoint.c export a speed of sysfs,
the speed sys file can't be set right value on X1 Carbon 5th. This issue
is caused by trackpoint firmware.
Workaround is to use xinput/xorg/udev/GUI to set trackpoint speed.

3, Windowds tool Trackpointdetect.exe shows it is ELAN (ID:03) in error log.
After debugging psmouse driver on it, the IDs and FW/hardware version is
not ELAN. Forcing to load ELAN driver will not make it work.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Torokhov Jan. 2, 2018, 7:08 a.m. UTC | #6 
Hi Aaron,

On Sun, Dec 31, 2017 at 04:51:09PM +0800, Aaron Ma wrote:
> 1, Commit ec667683c53 addes support of the new trackpoint.
> The trackpoint can be identified as "TPPS/2 IBM TrackPoint" and set
> properties OK. Trackpoint enabled some enhanced features.
> Without this commit there will be no scroll mode when xf86-input-evdev
> is used.
> 
> 2, trackpoint.c export a speed of sysfs,
> the speed sys file can't be set right value on X1 Carbon 5th. This issue
> is caused by trackpoint firmware.

No, I do not believe that this is an issue with the firmware. Rather, it
seems that Lenovo is multi-sourcing trackpoint for their devices, and
not all of them implement the IBM trackpoint protocol. We will need to
contact Elan to figure out what capabilities their trackpoints have.

> Workaround is to use xinput/xorg/udev/GUI to set trackpoint speed.

I'd rather we did not force users to do that.

> 
> 3, Windowds tool Trackpointdetect.exe shows it is ELAN (ID:03) in error log.
> After debugging psmouse driver on it, the IDs and FW/hardware version is
> not ELAN. Forcing to load ELAN driver will not make it work.

We do not have a special driver for Elan trackpoints and if they do not
fully implement IBM trackpoint protocol, then standard PS/2 mode should
be used, at least until someone creates a proper Elan trackpoint driver.

The original "magic ID" for the IBM trcakpoints was 0x1. This value was
described in the original trackpoint spec and has not changed since late
90th. Now we have 0x2 and 0x3. Could they indicate ALPS and Elan
trackpoints respectively? What ID does your Yoga report? Does it support
all features exported by the trackpoint driver (speed, sensitivity,
inertia, etc)?

Thanks.
Aaron Ma Jan. 2, 2018, 1:57 p.m. UTC | #7 
No, it is not a regression of this commit.

ThinkPad X1 Yoga 2nd:
   trackpoint (ID: 01)

ThinkPad X1 Yoga 3rd:
   trackpoint (ID: 03)

Both laptop's trackpoints have the same behavior.
Writing "speed" of sysfs is failed.

Override the ID and force loading drivers/input/mouse/elantech.c,
it causes too many failure and trackpoint stops work.

The ID of "2.4.18 READ SECONDARY ID (x"E1")" in TrackPoint specification
does not indicate any other vendors but only trackpoint.
Elantech uses 0x03e9.
ALPS uses 0x00e6/0x00e7/0x00ec.

Maybe the windows tool's is wrong like Linux driver before.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Torokhov Jan. 5, 2018, 12:56 a.m. UTC | #8 
Hi Aaron,

On Tue, Jan 02, 2018 at 09:57:55PM +0800, Aaron Ma wrote:
> No, it is not a regression of this commit.
> 
> ThinkPad X1 Yoga 2nd:
>    trackpoint (ID: 01)
> 
> ThinkPad X1 Yoga 3rd:
>    trackpoint (ID: 03)
> 
> Both laptop's trackpoints have the same behavior.
> Writing "speed" of sysfs is failed.
> 
> Override the ID and force loading drivers/input/mouse/elantech.c,
> it causes too many failure and trackpoint stops work.

Right, because it does not support Elantech *touchpad* protocol, that is
not a surprise.

> 
> The ID of "2.4.18 READ SECONDARY ID (x"E1")" in TrackPoint specification
> does not indicate any other vendors but only trackpoint.

Exactly. If ID does not match, it is not an IBM trackpoint device.

> Elantech uses 0x03e9.
> ALPS uses 0x00e6/0x00e7/0x00ec.
> 
> Maybe the windows tool's is wrong like Linux driver before.

I am not sure what you mean by that.

Anyway, I played with my Carbons a bit, and it seems that the patch
should indeed be reverted. I believe that neither the Elantech nor ALPS
trackpoints support the IBM trackpoint protocol; none of the extended
features (sensitivity, inertia, etc) work when we register them as
TTPS/2 devices. They should continue to be registered as "Generic PS/2"
as that's that they support.

I understand that you want scroll mode working with trackpoints, but
forcing them to pretend that they are TTPS/2 devices is not the proper
way of doing that. Write udev rules that would set
ID_INPUT_POINTINGSTICK property on all input devices connected to a
pass-through serio ports on LENOVO devices, and you should be set (just
make sure you cover both PS/2 pass-through and RMI pass-through
options).

Thanks.
Aaron Ma Jan. 5, 2018, 1:29 p.m. UTC | #9 
Hi Dmitry:

Got the official info from Lenovo:
Lenovo introduced new TrackPoint compatible sticks ( ELAN/Alps/NXP
sticks) from 2016.
These new devices only support the minimum commands described in the
spec, which has been used in the current Windows driver.

Legacy TrackPoint: 0101 – 0E01
ALPS: 0102 – FF02
ELAN:0103 – FF03
NXP: 0104 – FF04

2.4.18 READ SECONDARY ID (x"E1")
This command will read the secondary device ID of the pointing device (2
bytes).  The least significant byte is sent first.  For the first byte,
the legacy TrackPoint controller from IBM will always return x"01", the
pointing stick from ALPS will always return x"02", the pointing stick
from Elan will always return x"03”, and the pointing stick from NXP will
always return 0x”04".  And a second byte which denotes a specific set of
functional specifications.  Differing ROM versions are used to denote
changes within a given functional set.

The new devices (include Legacy ID:01) will not support the sysfs like
speed.

So it is not right to revert the commit, it is about to add another 0x04
ID in it.

Old sysfs could be stayed for old legacy device ID:01 or removed.

Aaron
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Torokhov Jan. 5, 2018, 4:23 p.m. UTC | #10 
Hi Aaron,

On Fri, Jan 05, 2018 at 09:29:26PM +0800, Aaron Ma wrote:
> Hi Dmitry:
> 
> Got the official info from Lenovo:
> Lenovo introduced new TrackPoint compatible sticks ( ELAN/Alps/NXP
> sticks) from 2016.
> These new devices only support the minimum commands described in the
> spec, which has been used in the current Windows driver.

What is the exact list of the commands supported by each variant?

> 
> Legacy TrackPoint: 0101 – 0E01
> ALPS: 0102 – FF02
> ELAN:0103 – FF03
> NXP: 0104 – FF04
> 
> 2.4.18 READ SECONDARY ID (x"E1")
> This command will read the secondary device ID of the pointing device (2
> bytes).  The least significant byte is sent first.  For the first byte,
> the legacy TrackPoint controller from IBM will always return x"01", the
> pointing stick from ALPS will always return x"02", the pointing stick
> from Elan will always return x"03”, and the pointing stick from NXP will
> always return 0x”04".  And a second byte which denotes a specific set of
> functional specifications.  Differing ROM versions are used to denote
> changes within a given functional set.

Can you/Lenovo share the updated spec?

> 
> The new devices (include Legacy ID:01) will not support the sysfs like
> speed.
> 
> So it is not right to revert the commit, it is about to add another 0x04
> ID in it.
> 
> Old sysfs could be stayed for old legacy device ID:01 or removed.

No, because there are devices that have trackpoints properly
implementing the protocol, before Lenovo started their "innovation".

Do we have any way to distinguish between properly implemented
trackpoints and Lenovo "improved" ones? I played with gen3 Carbon, and
while it does not error out on "speed" attribute, unlike gen5, it still
has no visible effects. Additionally, the "press to select"
functionality seems to be disabled, and trying to enable it via sysfs
results in register content being reverted to the original "disabled"
setting in a second or two. Setting to swap X and Y axes does not work
either, not sure about other bits of that control register.
"sensitivity" does work though, again unlike my gen5.

Thanks.
diff mbox

Patch

diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
index 0871010f18d5..20b5b21c1bba 100644
--- a/drivers/input/mouse/trackpoint.c
+++ b/drivers/input/mouse/trackpoint.c
@@ -265,8 +265,7 @@  static int trackpoint_start_protocol(struct psmouse *psmouse, unsigned char *fir
 	if (ps2_command(&psmouse->ps2dev, param, MAKE_PS2_CMD(0, 2, TP_READ_ID)))
 		return -1;
 
-	/* add new TP ID. */
-	if (!(param[0] & TP_MAGIC_IDENT))
+	if (param[0] != TP_MAGIC_IDENT)
 		return -1;
 
 	if (firmware_id)
diff --git a/drivers/input/mouse/trackpoint.h b/drivers/input/mouse/trackpoint.h
index 88055755f82e..5617ed3a7d7a 100644
--- a/drivers/input/mouse/trackpoint.h
+++ b/drivers/input/mouse/trackpoint.h
@@ -21,9 +21,8 @@ 
 #define TP_COMMAND		0xE2	/* Commands start with this */
 
 #define TP_READ_ID		0xE1	/* Sent for device identification */
-#define TP_MAGIC_IDENT		0x03	/* Sent after a TP_READ_ID followed */
+#define TP_MAGIC_IDENT		0x01	/* Sent after a TP_READ_ID followed */
 					/* by the firmware ID */
-					/* Firmware ID includes 0x1, 0x2, 0x3 */
 
 
 /*