Re: çå: çå: [PATCH] input: alps-fix the issue the special alps trackpoint do not work.

[Pali RohÃr]

Hi!

On Wednesday 22 May 2019 02:53:18 Xiaoxiao Liu wrote:
> Hi Pali,
> 
> Why it does not report input data?
> --> The alps devices which detected to use the ALPS_PROTO_V8  contains ALPS touchpad and ALPS track point. 
>      But the ALPS_PROTO_V8 do not support the track point device process. 
>      When the track point was detected to use ALPS_PROTO_V8 ,the v8 process_packet method  alps_process_packet_ss4_v2 will reject to report the data because the v8 device is      not set the ALPS_DUALPOINT flag. 
>      You can refer below code.

Ok, and cannot you set ALPS_DUALPOINT flag based on that
alps_check_is_trackpoint() result and then update
alps_process_packet_ss4_v3() code to supports also
V8 trackpoint packets?

> 	/* Report trackstick */
> 	if (alps_get_pkt_id_ss4_v2(packet) == SS4_PACKET_ID_STICK) {
> 		if (!(priv->flags & ALPS_DUALPOINT)) {
> 			psmouse_warn(psmouse,
> 				     "Rejected trackstick packet from non DualPoint device");
> 			return;
> 		}
> 		...
> 		return;
> 	}
> 
> why is for non-ALPS trackpoints used ALPS driver?
> --> the non-Alps track point is the ALPS touchpad, it is right to use the ALPS driver for ALPS touchpad.

Ok, now I got it.

> The v8 protocol condition may modified  as below, it will be more easier to understand.
> 
> 	 if (e7[0] == 0x73 && e7[1] == 0x03 && (e7[2] == 0x14 || e7[2] == 0x28) &&  (alps_check_is_trackpoint(psmouse) != 0)  ) {
>  		protocol = &alps_v8_protocol_data;
> 	}

Yes, this would be more easier to understand.

Anyway, more information should be in commit message.

> Best Regards
> XiaoXiao Liu
> sliuuxiaonxiao@xxxxxxxxx
> xiaoxiao.liu-1@xxxxxxxxxxx
> 
> -----éäåä-----
> åää: Pali RohÃr <pali.rohar@xxxxxxxxx> 
> åéæé: Tuesday, May 21, 2019 5:46 PM
> æää: Hui Wang <hui.wang@xxxxxxxxxxxxx>
> æé: å ææ Xiaoxiao Liu <xiaoxiao.liu-1@xxxxxxxxxxx>; XiaoXiao Liu <sliuuxiaonxiao@xxxxxxxxx>; dmitry.torokhov@xxxxxxxxx; linux-input@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; æ æå Xiaojian Cao <xiaojian.cao@xxxxxxxxxxx>; zhangfp1@xxxxxxxxxx
> äé: Re: çå: [PATCH] input: alps-fix the issue the special alps trackpoint do not work.
> 
> Hello!
> 
> On Tuesday 21 May 2019 10:26:47 Hui Wang wrote:
> > Tested-by: Hui Wang <hui.wang@xxxxxxxxxxxxx>
> > 
> > On 2019/5/21 äå9:07, Xiaoxiao Liu wrote:
> > > Add Pali RohÃr.
> > > 
> > > -----éäåä-----
> > > åää: XiaoXiao Liu <sliuuxiaonxiao@xxxxxxxxx>
> > > åéæé: Monday, May 20, 2019 7:02 PM
> > > æää: dmitry.torokhov@xxxxxxxxx
> > > æé: linux-input@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; 
> > > hui.wang@xxxxxxxxxxxxx; æ æå Xiaojian Cao 
> > > <xiaojian.cao@xxxxxxxxxxx>; zhangfp1@xxxxxxxxxx; å ææ Xiaoxiao Liu 
> > > <xiaoxiao.liu-1@xxxxxxxxxxx>; XiaoXiao Liu 
> > > <sliuuxiaonxiao@xxxxxxxxx>
> > > äé: [PATCH] input: alps-fix the issue the special alps trackpoint do not work.
> > > 
> > > when the alps trackpoint is detected and using the 
> > > alps_v8_protocol_data procotol, the alps driver will not report the input data.
> 
> Why it does not report input data?
> 
> > > solution: use standard mouse driver instead of alps driver when the specail trackpoint was detected.
> 
> This looks like an (undocumented) hack or workaround. Not a solution.
> 
> > > Signed-off-by: XiaoXiao Liu <sliuuxiaonxiao@xxxxxxxxx>
> > > ---
> > >   drivers/input/mouse/alps.c | 23 ++++++++++++++++++++++-
> > >   1 file changed, 22 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c 
> > > index 0a6f7ca883e7..516ae1d0eb17 100644
> > > --- a/drivers/input/mouse/alps.c
> > > +++ b/drivers/input/mouse/alps.c
> > > @@ -24,7 +24,7 @@
> > >   #include "psmouse.h"
> > >   #include "alps.h"
> > > -
> > > +#include "trackpoint.h"
> > >   /*
> > >    * Definitions for ALPS version 3 and 4 command mode protocol
> > >    */
> > > @@ -2864,6 +2864,22 @@ static const struct alps_protocol_info *alps_match_table(unsigned char *e7,
> > >   	return NULL;
> > >   }
> > > +int alps_check_is_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 error;
> > > +
> > > +	if (param[0] == TP_VARIANT_ALPS)
> > > +		return 0;
> > > +	psmouse_warn(psmouse, "It is not alps trackpoint.\n");
> > > +	return -ENODEV;
> > > +}
> 
> So, this function returns 0 when detected ALPS trackpoint and -ENODEV when not.
> 
> > > +
> > >   static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)  {
> > >   	const struct alps_protocol_info *protocol; @@ -2912,6 +2928,11 @@ static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
> > >   			protocol = &alps_v3_protocol_data;
> > >   		} else if (e7[0] == 0x73 && e7[1] == 0x03 &&
> > >   			   (e7[2] == 0x14 || e7[2] == 0x28)) {
> > > +				if (alps_check_is_trackpoint(psmouse) == 0) {
> > > +					psmouse_warn(psmouse,
> > > +					"It is alps trackpoint, use the standard mouse driver.\n");
> > > +					return -EINVAL;
> 
> And here I'm lost. If we have *not* detected ALPS trackpoint then if block is not called which means that ALPS driver is used.
> 
> So why is for non-ALPS trackpoints used ALPS driver? This does not seem like a correct...
> 
> And when we have detected ALPS trackpoint (return value 0) then standard mouse driver is used and returned -EINVAL. This seems strange too.
> 
> So either this code is wrong or there are missing more details for explaining this strange logic. But still this looks like a hack not a proper fix/solution.
> 
> > > +				}
> > >   			protocol = &alps_v8_protocol_data;
> > >   		} else if (e7[0] == 0x73 && e7[1] == 0x03 && e7[2] == 0xc8) {
> > >   			protocol = &alps_v9_protocol_data;
> > > --
> > > 2.20.1
> > > 
> 
> --
> Pali RohÃr
> pali.rohar@xxxxxxxxx

-- 
Pali RohÃr
pali.rohar@xxxxxxxxx

Attachment: signature.asc
Description: PGP signature