Re: [PATCH 1/7] input: alps: Set correct name of psmouse device in alps_init()

Pali Rohár
2015-01-08T17:58:38Z

[-- Attachment #1: Type: Text/Plain, Size: 5117 bytes --]

On Wednesday 24 December 2014 08:48:42 Pali Rohár wrote:
> On Saturday 20 December 2014 09:53:38 Pali Rohár wrote:
> > On Tuesday 16 December 2014 12:58:20 Pali Rohár wrote:
> > > On Tuesday 16 December 2014 06:02:34 Dmitry Torokhov wrote:
> > > > Hi Pali,
> > > >
> > > > On Fri, Nov 14, 2014 at 08:38:20PM +0100, Pali Rohár
wrote:
> > > > > On some laptops after starting them from off state
> > > > > (not after reboot), function
> > > > > alps_probe_trackstick_v3() (called from function
> > > > > alps_identify()) does not detect trackstick. To fix
> > > > > this problem we need to reset device. But function
> > > > > alps_identify() is called also from alps_detect() and
> > > > > we do not want to reset device in detect function
> > > > > because it will slow down
> > > > > initialization of all other non alps devices.
> > > > >
> > > > > This patch moves code for setting correct device name
> > > > > & protocol from function alps_detect() to alps_init()
> > > > > which already doing full device reset.
> > > > >
> > > > > So this patch removes need to do trackstick detection
> > > > > in alps_detect() function.
> > > > >
> > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > > > ---
> > > > >
> > > > > drivers/input/mouse/alps.c | 17 ++++++++++++++---
> > > > > 1 file changed, 14 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/input/mouse/alps.c
> > > > > b/drivers/input/mouse/alps.c index 8d85c79..9ffa98d
> > > > > 100644 --- a/drivers/input/mouse/alps.c
> > > > > +++ b/drivers/input/mouse/alps.c
> > > > > @@ -2392,6 +2392,10 @@ int alps_init(struct psmouse
> > > > > *psmouse)
> > > > >
> > > > > if (input_register_device(priv->dev2))
> > > > >
> > > > > goto init_fail;
> > > > >
> > > > > + if (!(priv->flags & ALPS_DUALPOINT))
> > > > > + psmouse->name = "GlidePoint TouchPad";
> > > > > + psmouse->model = priv->proto_version;
> > > > > +
> > > > >
> > > > > psmouse->protocol_handler = alps_process_byte;
> > > > > psmouse->poll = alps_poll;
> > > > > psmouse->disconnect = alps_disconnect;
> > > > >
> > > > > @@ -2422,11 +2426,18 @@ int alps_detect(struct psmouse
> > > > > *psmouse, bool set_properties)
> > > > >
> > > > > return -1;
> > > > >
> > > > > if (set_properties) {
> > > > >
> > > > > + /*
> > > > > + * NOTE: To detect model and trackstick
presence we
> > >
> > > need
> > >
> > > > > to do + * full device reset. To speed up
> > >
> > > detection
> > >
> > > > > and prevent + * calling duplicate
> >
> > initialization
> >
> > > > > sequence (both in + * alps_detect() and
> > > > > alps_init()) we set model/protocol + *
version
> >
> > and
> >
> > > > > correct name in alps_init() (which will + *
>
> do
>
> > > full
> > >
> > > > > device reset). For now set name to DualPoint. + */
> > > > >
> > > > > psmouse->vendor = "ALPS";
> > > > >
> > > > > - psmouse->name = dummy.flags & ALPS_DUALPOINT ?
> > > > > - "DualPoint TouchPad" : "GlidePoint";
> > > > > - psmouse->model = dummy.proto_version << 8;
> > > > > + psmouse->name = "DualPoint TouchPad";
> > > > >
> > > > > }
> > > > >
> > > > > +
> > > >
> > > > I do not quite like the way we change the device
> > > > description back and forth. Do you think we could
> > > > allocate the "real" priv structure in alps_detect() and
> > > > have alps_init() expect to find it (and free it if
> > > > set_properties is false). This way we'd go through
> > > > initialization once in detect, it will be authoritative,
> > > > and we would set the name of the device properly from
> > > > the beginning.
> > > >
> > > > Thanks.
> > >
> > > No without introducing another psmouse_reset call. I want
> > > to reduce time of loading driver, so I think this is
> > > better.
> >
> > Also psmouse-base.c call psmouse_reset between alps_detect()
> > and alps_init(). We must do trackstick detection after
> > psmouse_reset, but intorducing another psmouse_reset in
> > alps_detect() will slow down initialization for ALPS
> > devices. Also do not remember that psmouse_reset will lock
> > both keyboard & mouse for one second (sometimes for two) on
> > my laptop. I played with this and I think my patch is good
> > approach how to do trackstick detection without slowdown or
> > other side effects.
> >
> > Also we do not need to know if ALPS touchpad has trackstick
> > device in alps_detect() phase. In alps_detect() we just need
> > to know if PS/2 device is ALPS or not. From psmouse/serio
> > point of view there is only one ALPS device on PS/2/i8042
> > bus. Just alps.ko driver parse PS/2 packets and detect which
> > packet has touchpad header and which trackpoint. So we
> > really do not need to know if trackstick is there or not in
> > alps_detect() function.
>
> Dmitry ping, can you finally review this patch series? I it
> there for more then month...

Dmitry ping, see ^^^^^

--
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]