Input: alps - Don't handle ALPS cs19 trackpoint-only device

Commit Message

Hui Wang June 12, 2019, 7:05 a.m. UTC

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 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 completely different device 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.

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 <[e-mail]>
Signed-off-by: Hui Wang <[e-mail]>
---
 drivers/input/mouse/alps.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

Pali Rohár June 12, 2019, 7:38 a.m. UTC | #1

On Wednesday 12 June 2019 15:05:17 Hui Wang wrote:
> 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 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 completely different device 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.
> 
> 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 <[e-mail]>
> Signed-off-by: Hui Wang <[e-mail]>
> ---
>  drivers/input/mouse/alps.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 0a6f7ca883e7..ff522cd980a0 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,24 @@ 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 };
> +	int error;
> +
> +	error = ps2_command(&psmouse->ps2dev,
> +			    param, MAKE_PS2_CMD(0, 2, TP_READ_ID));
> +	if (error)
> +		return false;
> +
> +	if (param[0] == TP_VARIANT_ALPS && param[1] & 0x20) {

Hi!

Do we need to check firmware id? Is not check for "any alps trackpoint"
enough? If in future there would be more alps trackpoint-only devices it
probably have different firmware id.

Also you need to put param[1] & 0x20 into parenthesis due to priority of
& and && operators.

Also, what about making trackpoint_start_protocol() function non-static
and use it in alps_is_c19_trackpoint implementation? It is doing exactly
same thing.

> +		psmouse_dbg(psmouse, "It is an ALPS trackpoint-only device (CS19)\n");
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
>  static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
>  {
>  	const struct alps_protocol_info *protocol;
> @@ -2883,6 +2902,15 @@ static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
>  	if ((e6[0] & 0xf8) != 0 || e6[1] != 0 || (e6[2] != 10 && e6[2] != 100))
>  		return -EINVAL;
>  
> +	/*
> +	 * ALPS cs19 is a trackpoint-only device, it is completely independent
> +	 * of touchpad. So it is a different device from DualPoint ones, if it
> +	 * is identified as a cs19 trackpoint device, we return -EINVAL here and
> +	 * let trackpoint.c drive this device.
> +	 */
> +	if (alps_is_cs19_trackpoint(psmouse))
> +		return -EINVAL;
> +

This change is not ideal as this function would be called two times, see
alps_detect(). I would suggest to think more about detection and come up
with better solution so above trackpoint check would called only once
during PS/2 device detection.

Calling that trackpoint check two times is useless and just increase
detection time of PS/2 devices.

>  	/*
>  	 * Now get the "E7" and "EC" reports.  These will uniquely identify
>  	 * most ALPS touchpads.

Hui Wang June 13, 2019, 2:09 a.m. UTC | #2

On 2019/6/12 下午3:38, Pali Rohár wrote:
> On Wednesday 12 June 2019 15:05:17 Hui Wang wrote:
>> 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 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 completely different device 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.
>>
>> 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 <[e-mail]>
>> Signed-off-by: Hui Wang <[e-mail]>
>> ---
>>   drivers/input/mouse/alps.c | 28 ++++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
>> index 0a6f7ca883e7..ff522cd980a0 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,24 @@ 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 };
>> +	int error;
>> +
>> +	error = ps2_command(&psmouse->ps2dev,
>> +			    param, MAKE_PS2_CMD(0, 2, TP_READ_ID));
>> +	if (error)
>> +		return false;
>> +
>> +	if (param[0] == TP_VARIANT_ALPS && param[1] & 0x20) {
> Hi!
>
> Do we need to check firmware id? Is not check for "any alps trackpoint"
> enough? If in future there would be more alps trackpoint-only devices it
> probably have different firmware id.
>
> Also you need to put param[1] & 0x20 into parenthesis due to priority of
> & and && operators.
Got it, will drop param[1] & 0x20 and make it work for more alps devices.
>
> Also, what about making trackpoint_start_protocol() function non-static
> and use it in alps_is_c19_trackpoint implementation? It is doing exactly
> same thing.

Suppose users enabled the CONFIG_MOUSE_PS2_ALPS while disabled the 
CONFIG_MOUSE_PS2_TRACKPOINT, there will be some building error.

We may change Kconfig to let ALPS depend on TRACKPOINT, but it is not 
worth doing that only for one function.
>> +		psmouse_dbg(psmouse, "It is an ALPS trackpoint-only device (CS19)\n");
>> +		return true;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>>   static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
>>   {
>>   	const struct alps_protocol_info *protocol;
>> @@ -2883,6 +2902,15 @@ static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
>>   	if ((e6[0] & 0xf8) != 0 || e6[1] != 0 || (e6[2] != 10 && e6[2] != 100))
>>   		return -EINVAL;
>>   
>> +	/*
>> +	 * ALPS cs19 is a trackpoint-only device, it is completely independent
>> +	 * of touchpad. So it is a different device from DualPoint ones, if it
>> +	 * is identified as a cs19 trackpoint device, we return -EINVAL here and
>> +	 * let trackpoint.c drive this device.
>> +	 */
>> +	if (alps_is_cs19_trackpoint(psmouse))
>> +		return -EINVAL;
>> +
> This change is not ideal as this function would be called two times, see
> alps_detect(). I would suggest to think more about detection and come up
> with better solution so above trackpoint check would called only once
> during PS/2 device detection.
>
> Calling that trackpoint check two times is useless and just increase
> detection time of PS/2 devices.

OK, I will try to move this checking into the alps_detect(), then it 
will be called once.

Thanks.


>
>>   	/*
>>   	 * Now get the "E7" and "EC" reports.  These will uniquely identify
>>   	 * most ALPS touchpads.

Xiaoxiao Liu June 13, 2019, 4 a.m. UTC | #3

Hi Pali,

Do we need to check firmware id? Is not check for "any alps trackpoint"
enough? If in future there would be more alps trackpoint-only devices it probably have different firmware id.
   -> Yes ,we need the firmware version to check if the device is trackpoint-only.
        This method fit all the current alps trackpoint device.

Calling that trackpoint check two times is useless and just increase detection time of PS/2 devices.
  ->  what the twice means?
  ->  Do you means  ps2_command(&psmouse->ps2dev,    param, MAKE_PS2_CMD(0, 2, TP_READ_ID)) used in the alps.c and trackpoint.c  or   in function  alps_detect twice?(because the alps_identify was called twice.)
  ->  we must use this command in alps.c to filter the trackpoint-only device.
  -> We can move it into alps_detect function to reduce calls. How about this?
 
Best Regards
Shona
-----邮件原件-----
发件人: Pali Rohár <[e-mail]> 
发送时间: Wednesday, June 12, 2019 3:38 PM
收件人: Hui Wang <[e-mail]>
抄送: [e-mail]; [e-mail]; 劉 曉曉 Xiaoxiao Liu <[e-mail]>; [e-mail]; 曹 曉建 Xiaojian Cao <[e-mail]>; 斉藤 直樹 Naoki Saito <[e-mail]>; 川瀬 英夫 Hideo Kawase <[e-mail]>
主题: Re: [PATCH] Input: alps - Don't handle ALPS cs19 trackpoint-only device

On Wednesday 12 June 2019 15:05:17 Hui Wang wrote:
> 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 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 completely different device 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.
> 
> 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 <[e-mail]>
> Signed-off-by: Hui Wang <[e-mail]>
> ---
>  drivers/input/mouse/alps.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c 
> index 0a6f7ca883e7..ff522cd980a0 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,24 @@ 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 };
> +	int error;
> +
> +	error = ps2_command(&psmouse->ps2dev,
> +			    param, MAKE_PS2_CMD(0, 2, TP_READ_ID));
> +	if (error)
> +		return false;
> +
> +	if (param[0] == TP_VARIANT_ALPS && param[1] & 0x20) {

Hi!

Do we need to check firmware id? Is not check for "any alps trackpoint"
enough? If in future there would be more alps trackpoint-only devices it probably have different firmware id.

Also you need to put param[1] & 0x20 into parenthesis due to priority of & and && operators.

Also, what about making trackpoint_start_protocol() function non-static and use it in alps_is_c19_trackpoint implementation? It is doing exactly same thing.

> +		psmouse_dbg(psmouse, "It is an ALPS trackpoint-only device (CS19)\n");
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
>  static int alps_identify(struct psmouse *psmouse, struct alps_data 
> *priv)  {
>  	const struct alps_protocol_info *protocol; @@ -2883,6 +2902,15 @@ 
> static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
>  	if ((e6[0] & 0xf8) != 0 || e6[1] != 0 || (e6[2] != 10 && e6[2] != 100))
>  		return -EINVAL;
>  
> +	/*
> +	 * ALPS cs19 is a trackpoint-only device, it is completely independent
> +	 * of touchpad. So it is a different device from DualPoint ones, if it
> +	 * is identified as a cs19 trackpoint device, we return -EINVAL here and
> +	 * let trackpoint.c drive this device.
> +	 */
> +	if (alps_is_cs19_trackpoint(psmouse))
> +		return -EINVAL;
> +

This change is not ideal as this function would be called two times, see alps_detect(). I would suggest to think more about detection and come up with better solution so above trackpoint check would called only once during PS/2 device detection.

Calling that trackpoint check two times is useless and just increase detection time of PS/2 devices.

>  	/*
>  	 * Now get the "E7" and "EC" reports.  These will uniquely identify
>  	 * most ALPS touchpads.

--
Pali Rohár
[e-mail]

Pali Rohár June 13, 2019, 7:28 a.m. UTC | #4

On Thursday 13 June 2019 10:09:24 Hui Wang wrote:
> 
> On 2019/6/12 下午3:38, Pali Rohár wrote:
> > On Wednesday 12 June 2019 15:05:17 Hui Wang wrote:
> > > 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 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 completely different device 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.
> > > 
> > > 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 <[e-mail]>
> > > Signed-off-by: Hui Wang <[e-mail]>
> > > ---
> > >   drivers/input/mouse/alps.c | 28 ++++++++++++++++++++++++++++
> > >   1 file changed, 28 insertions(+)
> > > 
> > > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> > > index 0a6f7ca883e7..ff522cd980a0 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,24 @@ 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 };
> > > +	int error;
> > > +
> > > +	error = ps2_command(&psmouse->ps2dev,
> > > +			    param, MAKE_PS2_CMD(0, 2, TP_READ_ID));
> > > +	if (error)
> > > +		return false;
> > > +
> > > +	if (param[0] == TP_VARIANT_ALPS && param[1] & 0x20) {
> > Hi!
> > 
> > Do we need to check firmware id? Is not check for "any alps trackpoint"
> > enough? If in future there would be more alps trackpoint-only devices it
> > probably have different firmware id.
> > 
> > Also you need to put param[1] & 0x20 into parenthesis due to priority of
> > & and && operators.
> Got it, will drop param[1] & 0x20 and make it work for more alps devices.
> > 
> > Also, what about making trackpoint_start_protocol() function non-static
> > and use it in alps_is_c19_trackpoint implementation? It is doing exactly
> > same thing.
> 
> Suppose users enabled the CONFIG_MOUSE_PS2_ALPS while disabled the
> CONFIG_MOUSE_PS2_TRACKPOINT, there will be some building error.

So in this case support for these trackpoint-only devices is broken,
right? Compile error is for sure a problem. But if trackpoint is
disabled and in alps.c we detected that trackpoint.c is needed, I think
you should issue warning to dmesg that current compiled kernel cannot
properly handle input device (alps.c decided that trackpoint.c handles
this input device, but user decided that trackpoint.c does not compile
into kernel).

> We may change Kconfig to let ALPS depend on TRACKPOINT, but it is not worth
> doing that only for one function.

This dependency is not a good idea. Rather stay with current code when
small function is copy+paste and modified to drop unneeded return
variables (param[]).

> > > +		psmouse_dbg(psmouse, "It is an ALPS trackpoint-only device (CS19)\n");
> > > +		return true;
> > > +	}
> > > +
> > > +	return false;
> > > +}
> > > +
> > >   static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
> > >   {
> > >   	const struct alps_protocol_info *protocol;
> > > @@ -2883,6 +2902,15 @@ static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
> > >   	if ((e6[0] & 0xf8) != 0 || e6[1] != 0 || (e6[2] != 10 && e6[2] != 100))
> > >   		return -EINVAL;
> > > +	/*
> > > +	 * ALPS cs19 is a trackpoint-only device, it is completely independent
> > > +	 * of touchpad. So it is a different device from DualPoint ones, if it
> > > +	 * is identified as a cs19 trackpoint device, we return -EINVAL here and
> > > +	 * let trackpoint.c drive this device.
> > > +	 */
> > > +	if (alps_is_cs19_trackpoint(psmouse))
> > > +		return -EINVAL;
> > > +
> > This change is not ideal as this function would be called two times, see
> > alps_detect(). I would suggest to think more about detection and come up
> > with better solution so above trackpoint check would called only once
> > during PS/2 device detection.
> > 
> > Calling that trackpoint check two times is useless and just increase
> > detection time of PS/2 devices.
> 
> OK, I will try to move this checking into the alps_detect(), then it will be
> called once.
> 
> Thanks.
> 
> 
> > 
> > >   	/*
> > >   	 * Now get the "E7" and "EC" reports.  These will uniquely identify
> > >   	 * most ALPS touchpads.

Pali Rohár June 13, 2019, 7:36 a.m. UTC | #5

Hi!

On Thursday 13 June 2019 04:00:14 Xiaoxiao Liu wrote:
> Hi Pali,
> 
> Do we need to check firmware id? Is not check for "any alps trackpoint"
> enough? If in future there would be more alps trackpoint-only devices it probably have different firmware id.
>    -> Yes ,we need the firmware version to check if the device is trackpoint-only.
>         This method fit all the current alps trackpoint device.

Ok, if you as authority from ALPS says it is really needed, then please
use that firmware version check for ALPS trackpoint-only detection.

> Calling that trackpoint check two times is useless and just increase detection time of PS/2 devices.
>   ->  what the twice means?
>   ->  Do you means  ps2_command(&psmouse->ps2dev,    param, MAKE_PS2_CMD(0, 2, TP_READ_ID)) used in the alps.c and trackpoint.c  or   in function  alps_detect twice?(because the alps_identify was called twice.)
>   ->  we must use this command in alps.c to filter the trackpoint-only device.
>   -> We can move it into alps_detect function to reduce calls. How about this?

alps_detect() calls functions in this order:
 1. alps_identify(with priv = NULL)
 2. psmouse_reset()
 3. alps_identify(with priv = allocated memory)

It is because of proper detection on Dell Latitude machines, see
comments in alps_detect() function or commit messages via git blame for
more details.

So if you introduce check for ALPS trackpoint-only device into
alsp_identify() function, you slow down detection of all ALPS devices as
this check function would be called 2 times from alps_detect().

And then this function is called third time from trackpoint.c, but I do
not have an idea how to avoid this.

So instead of calling that detection function 3 times (2 times in alps.c
and one time in trackpoint.c), alps.c should be modified so detection
function is called only two times (one time in alps.c and one time in
trackpoint.c).

Or if you have an idea how to decrease just to one call, feel free to
propose solution and patch.

Or... Dmitry, what do you think about it? Maybe you could have something
in your mind how to avoid these inter-detection problems between two
drivers/protocols.

> Best Regards
> Shona
> -----邮件原件-----
> 发件人: Pali Rohár <[e-mail]> 
> 发送时间: Wednesday, June 12, 2019 3:38 PM
> 收件人: Hui Wang <[e-mail]>
> 抄送: [e-mail]; [e-mail]; 劉 曉曉 Xiaoxiao Liu <[e-mail]>; [e-mail]; 曹 曉建 Xiaojian Cao <[e-mail]>; 斉藤 直樹 Naoki Saito <[e-mail]>; 川瀬 英夫 Hideo Kawase <[e-mail]>
> 主题: Re: [PATCH] Input: alps - Don't handle ALPS cs19 trackpoint-only device
> 
> On Wednesday 12 June 2019 15:05:17 Hui Wang wrote:
> > 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 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 completely different device 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.
> > 
> > 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 <[e-mail]>
> > Signed-off-by: Hui Wang <[e-mail]>
> > ---
> >  drivers/input/mouse/alps.c | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c 
> > index 0a6f7ca883e7..ff522cd980a0 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,24 @@ 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 };
> > +	int error;
> > +
> > +	error = ps2_command(&psmouse->ps2dev,
> > +			    param, MAKE_PS2_CMD(0, 2, TP_READ_ID));
> > +	if (error)
> > +		return false;
> > +
> > +	if (param[0] == TP_VARIANT_ALPS && param[1] & 0x20) {
> 
> Hi!
> 
> Do we need to check firmware id? Is not check for "any alps trackpoint"
> enough? If in future there would be more alps trackpoint-only devices it probably have different firmware id.
> 
> Also you need to put param[1] & 0x20 into parenthesis due to priority of & and && operators.
> 
> Also, what about making trackpoint_start_protocol() function non-static and use it in alps_is_c19_trackpoint implementation? It is doing exactly same thing.
> 
> > +		psmouse_dbg(psmouse, "It is an ALPS trackpoint-only device (CS19)\n");
> > +		return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >  static int alps_identify(struct psmouse *psmouse, struct alps_data 
> > *priv)  {
> >  	const struct alps_protocol_info *protocol; @@ -2883,6 +2902,15 @@ 
> > static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
> >  	if ((e6[0] & 0xf8) != 0 || e6[1] != 0 || (e6[2] != 10 && e6[2] != 100))
> >  		return -EINVAL;
> >  
> > +	/*
> > +	 * ALPS cs19 is a trackpoint-only device, it is completely independent
> > +	 * of touchpad. So it is a different device from DualPoint ones, if it
> > +	 * is identified as a cs19 trackpoint device, we return -EINVAL here and
> > +	 * let trackpoint.c drive this device.
> > +	 */
> > +	if (alps_is_cs19_trackpoint(psmouse))
> > +		return -EINVAL;
> > +
> 
> This change is not ideal as this function would be called two times, see alps_detect(). I would suggest to think more about detection and come up with better solution so above trackpoint check would called only once during PS/2 device detection.
> 
> Calling that trackpoint check two times is useless and just increase detection time of PS/2 devices.
> 
> >  	/*
> >  	 * Now get the "E7" and "EC" reports.  These will uniquely identify
> >  	 * most ALPS touchpads.
> 
> --
> Pali Rohár
> [e-mail]

Hui Wang June 13, 2019, 8:43 a.m. UTC | #6

On 2019/6/13 下午3:28, Pali Rohár wrote:
> On Thursday 13 June 2019 10:09:24 Hui Wang wrote:
>> On 2019/6/12 下午3:38, Pali Rohár wrote:
>>> On Wednesday 12 June 2019 15:05:17 Hui Wang wrote:
>>>> 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 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 completely different device 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.
>>>>
>>>> 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 <[e-mail]>
>>>> Signed-off-by: Hui Wang <[e-mail]>
>>>> ---
>>>>    drivers/input/mouse/alps.c | 28 ++++++++++++++++++++++++++++
>>>>    1 file changed, 28 insertions(+)
>>>>
>>>> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
>>>> index 0a6f7ca883e7..ff522cd980a0 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,24 @@ 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 };
>>>> +	int error;
>>>> +
>>>> +	error = ps2_command(&psmouse->ps2dev,
>>>> +			    param, MAKE_PS2_CMD(0, 2, TP_READ_ID));
>>>> +	if (error)
>>>> +		return false;
>>>> +
>>>> +	if (param[0] == TP_VARIANT_ALPS && param[1] & 0x20) {
>>> Hi!
>>>
>>> Do we need to check firmware id? Is not check for "any alps trackpoint"
>>> enough? If in future there would be more alps trackpoint-only devices it
>>> probably have different firmware id.
>>>
>>> Also you need to put param[1] & 0x20 into parenthesis due to priority of
>>> & and && operators.
>> Got it, will drop param[1] & 0x20 and make it work for more alps devices.
>>> Also, what about making trackpoint_start_protocol() function non-static
>>> and use it in alps_is_c19_trackpoint implementation? It is doing exactly
>>> same thing.
>> Suppose users enabled the CONFIG_MOUSE_PS2_ALPS while disabled the
>> CONFIG_MOUSE_PS2_TRACKPOINT, there will be some building error.
> So in this case support for these trackpoint-only devices is broken,
> right? Compile error is for sure a problem. But if trackpoint is
> disabled and in alps.c we detected that trackpoint.c is needed, I think
> you should issue warning to dmesg that current compiled kernel cannot
> properly handle input device (alps.c decided that trackpoint.c handles
> this input device, but user decided that trackpoint.c does not compile
> into kernel).
Good idea, will add a proper warning for it.
>> We may change Kconfig to let ALPS depend on TRACKPOINT, but it is not worth
>> doing that only for one function.
> This dependency is not a good idea. Rather stay with current code when
> small function is copy+paste and modified to drop unneeded return
> variables (param[]).
>
>>>> +		psmouse_dbg(psmouse, "It is an ALPS trackpoint-only device (CS19)\n");
>>>> +		return true;
>>>> +	}
>>>> +
>>>> +	return false;
>>>> +}
>>>> +
>>>>    static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
>>>>    {
>>>>    	const struct alps_protocol_info *protocol;
>>>> @@ -2883,6 +2902,15 @@ static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
>>>>    	if ((e6[0] & 0xf8) != 0 || e6[1] != 0 || (e6[2] != 10 && e6[2] != 100))
>>>>    		return -EINVAL;
>>>> +	/*
>>>> +	 * ALPS cs19 is a trackpoint-only device, it is completely independent
>>>> +	 * of touchpad. So it is a different device from DualPoint ones, if it
>>>> +	 * is identified as a cs19 trackpoint device, we return -EINVAL here and
>>>> +	 * let trackpoint.c drive this device.
>>>> +	 */
>>>> +	if (alps_is_cs19_trackpoint(psmouse))
>>>> +		return -EINVAL;
>>>> +
>>> This change is not ideal as this function would be called two times, see
>>> alps_detect(). I would suggest to think more about detection and come up
>>> with better solution so above trackpoint check would called only once
>>> during PS/2 device detection.
>>>
>>> Calling that trackpoint check two times is useless and just increase
>>> detection time of PS/2 devices.
>> OK, I will try to move this checking into the alps_detect(), then it will be
>> called once.
>>
>> Thanks.
>>
>>
>>>>    	/*
>>>>    	 * Now get the "E7" and "EC" reports.  These will uniquely identify
>>>>    	 * most ALPS touchpads.

Patch

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 0a6f7ca883e7..ff522cd980a0 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,24 @@  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 };
+	int error;
+
+	error = ps2_command(&psmouse->ps2dev,
+			    param, MAKE_PS2_CMD(0, 2, TP_READ_ID));
+	if (error)
+		return false;
+
+	if (param[0] == TP_VARIANT_ALPS && param[1] & 0x20) {
+		psmouse_dbg(psmouse, "It is an ALPS trackpoint-only device (CS19)\n");
+		return true;
+	}
+
+	return false;
+}
+
 static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
 {
 	const struct alps_protocol_info *protocol;
@@ -2883,6 +2902,15 @@  static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
 	if ((e6[0] & 0xf8) != 0 || e6[1] != 0 || (e6[2] != 10 && e6[2] != 100))
 		return -EINVAL;
 
+	/*
+	 * ALPS cs19 is a trackpoint-only device, it is completely independent
+	 * of touchpad. So it is a different device from DualPoint ones, if it
+	 * is identified as a cs19 trackpoint device, we return -EINVAL here and
+	 * let trackpoint.c drive this device.
+	 */
+	if (alps_is_cs19_trackpoint(psmouse))
+		return -EINVAL;
+
 	/*
 	 * Now get the "E7" and "EC" reports.  These will uniquely identify
 	 * most ALPS touchpads.