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

Message ID 1415993906-13307-2-git-send-email-pali.rohar@gmail.com
State New, archived
Headers show

Commit Message

Pali Rohár Nov. 14, 2014, 7:38 p.m. UTC
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(-)

Comments

Dmitry Torokhov Dec. 16, 2014, 5:02 a.m. UTC | #1
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.
Pali Rohár Dec. 16, 2014, 11:58 a.m. UTC | #2
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.
Pali Rohár Dec. 20, 2014, 8:53 a.m. UTC | #3
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.
Pali Rohár Dec. 24, 2014, 7:48 a.m. UTC | #4
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...
Pali Rohár Jan. 8, 2015, 5:58 p.m. UTC | #5
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 ^^^^^

Patch
diff mbox

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";
 	}
+
 	return 0;
 }