On Mon, Jul 11, 2016 at 07:57:57PM -0700, Dmitry Torokhov wrote:
> On Mon, Jul 11, 2016 at 09:26:20PM +0100, Chris Diamand wrote:
> > Hi Richard,
> >
> > Thanks for working on this!
> >
> > Patch looks mostly good, and seems to have worked judging by the comments on
> > the bug report. Not many comments from me as it's just moving stuff around.
> >
> > I think it might be better to keep detect() and init() separate - it seems to
> > me that a detect() function shouldn't really have any side effects (even if
> > we're certain it's a BYD touchpad), so doing kzalloc and setting up the timer
> > in detect() feels a bit wrong.
>
> Right.
>
> >
> > But as mentioned earlier, we shouldn't do the touchpad initialisation twice.
> > So maybe do the magic sequence stuff in detect(), and the actual driver init
> > in init().
>
> Since we can't transparently detect the device without changing its
> state everything should go into byd_init() and we should remove
> byd_detect() (unless we decide to do some DMI matching).
>
> >
> > > Input: byd - fix issue where generic PS/2 mice are...
> >
> > You may need a shorter commit title.
> >
> > > The secret handshake used was not sufficient to determine whether the connected
> > > device was actually a BYD touchpad. Added some restrictions on what the first
> > > byte returned may be (based off of experiments with BYD touchapd). Moved
> > > subsequent initialization logic from byd_init to tail of byd_detect, and
> > > removed byd_init function.
> > >
> > > Fixes bug 1201781. Tested on laptop with BYD touchpad hardware.
> > >
> > > Applied against commit fcd6eb50eadd83f857eac55f99316f1789707cdb
> >
> > Probably don't need this in the commit message.
> >
> > >
> > > Signed-off-by: Richard Pospesel <pospeselr@xxxxxxxxx>
> > >
> > > ---
> > > diff --git a/drivers/input/mouse/byd.c b/drivers/input/mouse/byd.c
> > > index ec73f75..92f5556 100644
> > > --- a/drivers/input/mouse/byd.c
> > > +++ b/drivers/input/mouse/byd.c
> > > @@ -2,6 +2,10 @@
> > > * BYD TouchPad PS/2 mouse driver
> > > *
> > > * Copyright (C) 2015 Chris Diamand <chris@xxxxxxxxxxx>
> > > + * Copyright (C) 2015 Richard Pospesel
> > > + * Copyright (C) 2015 Tai Chi Minh Ralph Eastwood
> > > + * Copyright (C) 2015 Martin Wimpress
> > > + * Copyright (C) 2015 Jay Kuri
> >
> > I already updated these in 82aaa086019ce0e6fcd3a44c0a2e4329afe988b6, so no
> > need to include it here. (I think there's a disparity between input-next and
> > mainline).
>
> I have 2 branches (for-linus and next), and they may have contain
> different changes (as I re-sync with mainline as needed), not all the
> time.
>
> >
> > > *
> > > * This program is free software; you can redistribute it and/or modify it
> > > * under the terms of the GNU General Public License version 2 as published by
> > > @@ -355,7 +359,7 @@ static int byd_reset_touchpad(struct psmouse *psmouse)
> > > { PSMOUSE_CMD_ENABLE, 0 },
> > > /*
> > > * BYD-specific initialization, which enables absolute mode and
> > > - * (if desired), the touchpad's built-in gesture detection.
> > > + * disables the builtin hardware gesture recogniton.
> > > */
> > > { 0x10E2, 0x00 },
> > > { 0x10E0, 0x02 },
> > > @@ -435,6 +439,10 @@ int byd_detect(struct psmouse *psmouse, bool set_properties)
> > > struct ps2dev *ps2dev = &psmouse->ps2dev;
> > > u8 param[4] = {0x03, 0x00, 0x00, 0x00};
> > >
> > > + if (psmouse_reset(psmouse))
> > > + return -EIO;
>
> I would rather we reset the device after the first magic handshake to
> speed up case when it is not byd device. Hmm... You know, reset is quite
> slow operation and given how unreliable the first handshake is I really
> lean towards asking to add DMI matching into the driver. Because we just
> penalized everyone else with "standard" mice here.
Chris, Richard, do you guys have the same hardware or different laptops?
Could you share DMI data?
>
> > > +
> > > + /* 'Secret' handshake */
> > > if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES))
> > > return -1;
> > > if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES))
> > > @@ -446,62 +454,68 @@ int byd_detect(struct psmouse *psmouse, bool set_properties)
> > > if (ps2_command(ps2dev, param, PSMOUSE_CMD_GETINFO))
> > > return -1;
> > >
> > > - if (param[1] != 0x03 || param[2] != 0x64)
> > > + /*
> > > + * BYD touchpad returns 0x03 for resolution ( 8 count / mm ) and
> > > + * 0x64 ( 100 samples / sec ) for sampling rate
> >
> > Spaces inside brackets?
> >
> > > + * The first byte's value is dependent on the mouse button states:
> > > + * 0 : no button pressed
> > > + * 1 : right button pressed
> > > + * 4 : left button pressed
> > > + * 5 : right and left button pressed
> > > + */
> > > + if ((param[0] & 0x05) != param[0] ||
> >
> > Neat. Took me a while though, would something like this be clearer?
> >
> > param[0] & ~(0x00 | 0x01 | 0x04 | 0x05)
>
> Hmm... Either "param[0] & ~0x05" or "param[0] & ~(BIT(0) | BIT(2))" or
> leave as Richard had it.
>
> Thanks.
>
> --
> Dmitry
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
