[SRU][B/C/D/OEM-OSP1-B/E][PATCH 1/1] Input: alps - don't handle ALPS cs19 trackpoint-only device

[Hui Wang]

On 2019/7/19 上午5:01, Andy Whitcroft wrote:
> On Tue, Jul 16, 2019 at 10:12:57PM +0800, Hui Wang wrote:
>> BugLink: https://bugs.launchpad.net/bugs/1836752
>>
>> On a latest Lenovo laptop, the trackpoint and 3 buttons below it
>> don't work at all, when we move the trackpoint or press those 3
>> buttons, the kernel will print out:
>> "Rejected trackstick packet from non DualPoint device"
>>
>> This device is identified as an alps touchpad but the packet has
>> trackpoint format, so the alps.c drops the packet and prints out
>> the message above.
>>
>> According to XiaoXiao's explanation, this device is named cs19 and
>> is trackpoint-only device, its firmware is only for trackpoint, it
>> is independent of touchpad and is a device completely different from
>> DualPoint ones.
>>
>> To drive this device with mininal changes to the existing driver, we
>> just let the alps driver not handle this device, then the trackpoint.c
>> will be the driver of this device if the trackpoint driver is enabled.
>> (if not, this device will fallback to a bare PS/2 device)
>>
>> With the trackpoint.c, this trackpoint and 3 buttons all work well,
>> they have all features that the trackpoint should have, like
>> scrolling-screen, drag-and-drop and frame-selection.
>>
>> Signed-off-by: XiaoXiao Liu <sliuuxiaonxiao at gmail.com>
>> Signed-off-by: Hui Wang <hui.wang at canonical.com>
>> Reviewed-by: Pali Rohár <pali.rohar at gmail.com>
>> Cc: stable at vger.kernel.org
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov at gmail.com>
>> (cherry picked from commit 7e4935ccc3236751e5fe4bd6846f86e46bb2e427
>> git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git)
>> Signed-off-by: Hui Wang <hui.wang at canonical.com>
>> ---
>>   drivers/input/mouse/alps.c | 32 ++++++++++++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
>>
>> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
>> index 0a6f7ca883e7..11a4363c3b60 100644
>> --- a/drivers/input/mouse/alps.c
>> +++ b/drivers/input/mouse/alps.c
>> @@ -24,6 +24,7 @@
>>   
>>   #include "psmouse.h"
>>   #include "alps.h"
>> +#include "trackpoint.h"
>>   
>>   /*
>>    * Definitions for ALPS version 3 and 4 command mode protocol
>> @@ -2864,6 +2865,23 @@ static const struct alps_protocol_info *alps_match_table(unsigned char *e7,
>>   	return NULL;
>>   }
>>   
>> +static bool alps_is_cs19_trackpoint(struct psmouse *psmouse)
>> +{
>> +	u8 param[2] = { 0 };
>> +
>> +	if (ps2_command(&psmouse->ps2dev,
>> +			param, MAKE_PS2_CMD(0, 2, TP_READ_ID)))
>> +		return false;
>> +
>> +	/*
>> +	 * param[0] contains the trackpoint device variant_id while
>> +	 * param[1] contains the firmware_id. So far all alps
>> +	 * trackpoint-only devices have their variant_ids equal
>> +	 * TP_VARIANT_ALPS and their firmware_ids are in 0x20~0x2f range.
>> +	 */
>> +	return param[0] == TP_VARIANT_ALPS && (param[1] & 0x20);
> I don't think the comment is consistent with the code.  I think the last
> conditional should be something like the below to match the comment:
>
>      ((param[1] | 0x0f) == 0x2f)
>
> I have no idea if the comment is right, or the code is right.

You are right,  if param[1] is 0x30, 0x60..., the condition will be true 
unexpectedly. This is an issue, and I will send an incremental patch to 
fix it.

And in practice, it will not introduce any issue since the alps told me 
the param[1]  only has 0x0~0x1f in the old devices, and in the 
trackpoint-only device, it has 0x20~0x2f, the value higher than 0x2f has 
not been used yet. so it is safe to merge this patch temporarily. And I 
will submit an incremental SRU after the incremental patch is upstreamed.


Thanks.



>
>> +}
>> +
>>   static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
>>   {
>>   	const struct alps_protocol_info *protocol;
>> @@ -3164,6 +3182,20 @@ int alps_detect(struct psmouse *psmouse, bool set_properties)
>>   	if (error)
>>   		return error;
>>   
>> +	/*
>> +	 * ALPS cs19 is a trackpoint-only device, and uses different
>> +	 * protocol than DualPoint ones, so we return -EINVAL here and let
>> +	 * trackpoint.c drive this device. If the trackpoint driver is not
>> +	 * enabled, the device will fall back to a bare PS/2 mouse.
>> +	 * If ps2_command() fails here, we depend on the immediately
>> +	 * followed psmouse_reset() to reset the device to normal state.
>> +	 */
>> +	if (alps_is_cs19_trackpoint(psmouse)) {
>> +		psmouse_dbg(psmouse,
>> +			    "ALPS CS19 trackpoint-only device detected, ignoring\n");
>> +		return -EINVAL;
>> +	}
>> +
>>   	/*
>>   	 * Reset the device to make sure it is fully operational:
>>   	 * on some laptops, like certain Dell Latitudes, we may
>> -- 
>> 2.17.1
>>
>>
>> -- 
>> kernel-team mailing list
>> kernel-team at lists.ubuntu.com
>> https://lists.ubuntu.com/mailman/listinfo/kernel-team