Re: [PATCH 01/01] drivers:input:byd fix greedy detection of Sentelic FSP by the BYD touchpad driver

[Christophe Tordeux]

Hello,

On Friday 07 October 2016 at 04:37:26PM, Dmitry Torokhov has written:
> Hi Christophe,
> 
> On Fri, Oct 07, 2016 at 12:41:48PM +0800, Christophe Tordeux wrote:
> > From: Christophe TORDEUX <christophe@xxxxxxxxxxx>
> > 
> > With kernel v4.6 and later, the Sentelic touchpad STL3888_C0 and
> > probably other Sentelic FSP touchpads are detected as a BYD touchpad and
> > lose multitouch features.
> > 
> > During the BYD handshake in the byd_detect function, the BYD driver
> > mistakenly interprets a standard PS/2 protocol status request answer
> > from the Sentelic touchpad as a successful handshake with a BYD
> > touchpad. This is clearly a bug of the BYD driver.
> > 
> > Description of the patch: In byd_detect function, remove positive
> > detection result based on standard PS/2 protocol status request answer.
> > Replace it with positive detection based on handshake answers as they
> > can be inferred from the BYD touchpad datasheets found on BYD website.
> > 
> > Signed-off-by: Christophe TORDEUX <christophe@xxxxxxxxxxx>
> 
> What devices was this tested on? Do you have both BYD and Sentelic
> devices?
> 

I only have a Sentelic STL3888_C0 touchpad, I don't have any BYD 
touchpad.

> I am really concerned about docs on BYD side as they seem to contradict
> each other... I wonder how accurate they are.
> 

I agree, there's problems with each of their docs. I tried to infer the 
handshake answers based on the big picture that seems to emerge when 
reading all of them.

If ever that matters, I found out that my STL3888_C0 touchpad:
* answers E8/03/E8/03/E8/03/E8/03/E9 like standard PS/2 protocol
* answers E8/00/E8/00/E8/00/E8/00/E9 with 00/88/64

The latter does not look standard PS/2 so may be a kind of handshake 
from the Sentelic touchpad. And, in the BYD docs, part of the BYD 
devices also call for 00 during the handshake, others call for 03.

Things which can be considered in addition or in parallel to my patch:

1) in psmouse-base.c, move the detection of Sentelic touchpads above the 
detection of BYD touchpads. However based on the code comments could 
have adverse effects I don't know about.

2) Exit the byd_detect function without detection based on 
E8/00/E8/00/E8/00/E8/00/E9 being answered by 00/88/64. However while it 
would certainly fix my issue with my particular touchpad, it still would
leaves the byd_detect function being dirty and greedy otherwise, so I 
don't really like it.

3) take my patch, keep the 00 and 03 handshakes in sequence, add a 
detection based on the first byte of data being 01 regardless of the 
other bytes of data in the answer. It would probably fix the Sentelic 
detection issue... unless the user is pressing the left button during 
the BYD handshake. From the BYD docs "it is expected the get the value 
of 01 in the first byte data (...) and the first byte data is the only 
data needed to be checked". The issue with the docs is that this 
statement is sometimes contradicted by the sketch of the handshake... My 
patch is mostly based on the handshake sketches currently, but I could 
add a detection based on the first byte being 01, with no other test.

Let me know what looks best for you, for me the third choice is probably 
the most reasonable solution, either this or do it like in my initial 
patch.  If you know someone who has the kernel sources and a BYD 
touchpad, ask him to get the answer to E8/00/E8/00/E8/00/E8/00/E9 from 
BYD touchpads.

> Thanks.
> 
> > 
> > ---
> > Resubmitting this patch because I got no feedback on my first 
> > submission.
> > Fixes kernel bug 175421 which is impacting multiple users.
> > 
> > ---
> >  drivers/input/mouse/byd.c | 76 
> >  ++++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 62 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/input/mouse/byd.c b/drivers/input/mouse/byd.c
> > index b27aa63..b5acca0 100644
> > --- a/drivers/input/mouse/byd.c
> > +++ b/drivers/input/mouse/byd.c
> > @@ -35,6 +35,18 @@
> >   * BYD pad constants
> >   */
> >  
> > +/* Handshake answer of BTP6034 */
> > +#define BYD_MODEL_BTP6034	0x00E801
> > +/* Handshake answer of BTP6740 */
> > +#define BYD_MODEL_BTP6740	0x001155
> > +/* Handshake answers of BTP8644, BTP10463 and BTP11484 */
> > +#define BYD_MODEL_BTP8644	0x011155
> > +
> > +/* Handshake SETRES byte of BTP6034 and BTP6740 */
> > +#define BYD_SHAKE_BYTE_A	0x00
> > +/* Handshake SETRES byte of BTP8644, BTP10463 and BTP11484 */
> > +#define BYD_SHAKE_BYTE_B	0x03
> > +
> >  /*
> >   * True device resolution is unknown, however experiments show the
> >   * resolution is about 111 units/mm.
> > @@ -434,23 +446,59 @@ static void byd_disconnect(struct psmouse *psmouse)
> >  	}
> >  }
> >  
> > +u32 byd_try_model(u32 model)
> > +{
> > +	size_t i;
> > +
> > +	u32 byd_model[] = {
> > +		BYD_MODEL_BTP6034,
> > +		BYD_MODEL_BTP6740,
> > +		BYD_MODEL_BTP8644
> > +	};
> > +
> > +	for (i=0; i < ARRAY_SIZE(byd_model); i++) {
> > +		if (model ==  byd_model[i])
> > +			return model;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  int byd_detect(struct psmouse *psmouse, bool set_properties)
> >  {
> >  	struct ps2dev *ps2dev = &psmouse->ps2dev;
> > -	u8 param[4] = {0x03, 0x00, 0x00, 0x00};
> > -
> > -	if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES))
> > -		return -1;
> > -	if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES))
> > -		return -1;
> > -	if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES))
> > -		return -1;
> > -	if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES))
> > -		return -1;
> > -	if (ps2_command(ps2dev, param, PSMOUSE_CMD_GETINFO))
> > -		return -1;
> > -
> > -	if (param[1] != 0x03 || param[2] != 0x64)
> > +	size_t i;
> > +
> > +	u8 byd_shbyte[] = {
> > +		BYD_SHAKE_BYTE_A,
> > +		BYD_SHAKE_BYTE_B
> > +	};
> > +
> > +	bool detect = false;
> > +	for (i=0; i < ARRAY_SIZE(byd_shbyte); i++) {
> > +		u32 model;
> > +		u8 param[4] = {byd_shbyte[i], 0x00, 0x00, 0x00};
> > +
> > +		if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES))
> > +			return -1;
> > +		if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES))
> > +			return -1;
> > +		if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES))
> > +			return -1;
> > +		if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES))
> > +			return -1;
> > +		if (ps2_command(ps2dev, param, PSMOUSE_CMD_GETINFO))
> > +			return -1;
> > +
> > +		model = param[2];
> > +		model += param[1] << 8;
> > +		model += param[0] << 16;
> > +		model = byd_try_model(model);
> > +		if (model)
> > +			detect = true;
> > +	}
> > +
> > +	if (!detect)
> >  		return -ENODEV;
> >  
> >  	psmouse_dbg(psmouse, "BYD touchpad detected\n");
> 
> 
> 
> -- 
> Dmitry
> 

-- 
Christophe TORDEUX
4403, Building 3, Arcadia Court
South side of Liuwei Rd. West
Hedong District
Tianjin, 300350
P.R. China
mob: +86 186 1403 3192
http://christophe.tordeux.net

Attachment: signature.asc
Description: PGP signature