[PATCH] ALPS: fix enabling hardware tapping

[PATCH] ALPS: fix enabling hardware tapping

[Dmitry Torokhov]

Hi Linus, Vojtech,

It looks like logic for enabling hardware tapping in ALPS driver was
inverted and we enable it only if it was already enabled by BIOS or
firmware.

I have a confirmation from one user that the patch below fixes the
problem for him and it might be beneficial if we could get it into
2.6.12.

Thanks!

-- 
Dmitry

===================================================================

Input: ALPS - try enabling tap mode if it was disabled, not if
       it is already enabled.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/mouse/alps.c |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)

Index: work/drivers/input/mouse/alps.c
===================================================================
--- work.orig/drivers/input/mouse/alps.c
+++ work/drivers/input/mouse/alps.c
@@ -364,7 +364,7 @@ static int alps_reconnect(struct psmouse
 	if (alps_get_status(psmouse, param))
 		return -1;
 
-	if (param[0] & 0x04)
+	if (!(param[0] & 0x04))
 		alps_tap_mode(psmouse, 1);
 
 	if (alps_absolute_mode(psmouse)) {

Re: [PATCH] ALPS: fix enabling hardware tapping

[Vojtech Pavlik]

On Wed, Jun 15, 2005 at 01:38:49AM -0500, Dmitry Torokhov wrote:
> Hi Linus, Vojtech,
> 
> It looks like logic for enabling hardware tapping in ALPS driver was
> inverted and we enable it only if it was already enabled by BIOS or
> firmware.
> 
> I have a confirmation from one user that the patch below fixes the
> problem for him and it might be beneficial if we could get it into
> 2.6.12.

Linus, Andrew, please include this patch. I don't have a git tree setup
yet for pulls, and it won't be ready before 2.6.12, however this patch
should definitely go in.

Thanks,
	Vojtech

> Thanks!
> 
> -- 
> Dmitry
> 
> ===================================================================
> 
> Input: ALPS - try enabling tap mode if it was disabled, not if
>        it is already enabled.
> 
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---
> 
>  drivers/input/mouse/alps.c |    2 +-
>  1 files changed, 1 insertion(+), 1 deletion(-)
> 
> Index: work/drivers/input/mouse/alps.c
> ===================================================================
> --- work.orig/drivers/input/mouse/alps.c
> +++ work/drivers/input/mouse/alps.c
> @@ -364,7 +364,7 @@ static int alps_reconnect(struct psmouse
>  	if (alps_get_status(psmouse, param))
>  		return -1;
>  
> -	if (param[0] & 0x04)
> +	if (!(param[0] & 0x04))
>  		alps_tap_mode(psmouse, 1);
>  
>  	if (alps_absolute_mode(psmouse)) {
> 

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: [PATCH] ALPS: fix enabling hardware tapping

[Peter Osterlund]

Dmitry Torokhov <dtor_core@ameritech.net> writes:

> It looks like logic for enabling hardware tapping in ALPS driver was
> inverted and we enable it only if it was already enabled by BIOS or
> firmware.

It looks like alps_init() has the same bug.  This patch fixes that
function too by moving the check if the tapping mode needs to change
into the alps_tap_mode() function, so that the test doesn't have to be
duplicated.

Signed-off-by: Peter Osterlund <petero2@telia.com>
---

 linux-petero/drivers/input/mouse/alps.c |   29 +++++++++++------------------
 1 files changed, 11 insertions(+), 18 deletions(-)

diff -puN drivers/input/mouse/alps.c~alps-hwtaps-fix drivers/input/mouse/alps.c
--- linux/drivers/input/mouse/alps.c~alps-hwtaps-fix	2005-07-03 13:42:47.000000000 +0200
+++ linux-petero/drivers/input/mouse/alps.c	2005-07-03 13:42:47.000000000 +0200
@@ -2,7 +2,7 @@
  * ALPS touchpad PS/2 mouse driver
  *
  * Copyright (c) 2003 Neil Brown <neilb@cse.unsw.edu.au>
- * Copyright (c) 2003 Peter Osterlund <petero2@telia.com>
+ * Copyright (c) 2003-2005 Peter Osterlund <petero2@telia.com>
  * Copyright (c) 2004 Dmitry Torokhov <dtor@mail.ru>
  * Copyright (c) 2005 Vojtech Pavlik <vojtech@suse.cz>
  *
@@ -334,6 +334,13 @@ static int alps_tap_mode(struct psmouse 
 	int cmd = enable ? PSMOUSE_CMD_SETRATE : PSMOUSE_CMD_SETRES;
 	unsigned char tap_arg = enable ? 0x0A : 0x00;
 	unsigned char param[4];
+	int enabled;
+
+	if (alps_get_status(psmouse, param))
+		return -1;
+	enabled = (param[0] & 0x04) != 0;
+	if (enabled == enable)
+		return 0;
 
 	if (ps2_command(ps2dev, param, PSMOUSE_CMD_GETINFO) ||
 	    ps2_command(ps2dev, NULL, PSMOUSE_CMD_DISABLE) ||
@@ -350,7 +357,6 @@ static int alps_tap_mode(struct psmouse 
 static int alps_reconnect(struct psmouse *psmouse)
 {
 	struct alps_data *priv = psmouse->private;
-	unsigned char param[4];
 	int version;
 
 	psmouse_reset(psmouse);
@@ -361,11 +367,7 @@ static int alps_reconnect(struct psmouse
 	if (priv->i->flags & ALPS_PASS && alps_passthrough_mode(psmouse, 1))
 		return -1;
 
-	if (alps_get_status(psmouse, param))
-		return -1;
-
-	if (!(param[0] & 0x04))
-		alps_tap_mode(psmouse, 1);
+	alps_tap_mode(psmouse, 1);
 
 	if (alps_absolute_mode(psmouse)) {
 		printk(KERN_ERR "alps.c: Failed to enable absolute mode\n");
@@ -389,7 +391,6 @@ static void alps_disconnect(struct psmou
 int alps_init(struct psmouse *psmouse)
 {
 	struct alps_data *priv;
-	unsigned char param[4];
 	int version;
 
 	psmouse->private = priv = kmalloc(sizeof(struct alps_data), GFP_KERNEL);
@@ -403,16 +404,8 @@ int alps_init(struct psmouse *psmouse)
 	if ((priv->i->flags & ALPS_PASS) && alps_passthrough_mode(psmouse, 1))
 		goto init_fail;
 
-	if (alps_get_status(psmouse, param)) {
-		printk(KERN_ERR "alps.c: touchpad status report request failed\n");
-		goto init_fail;
-	}
-
-	if (param[0] & 0x04) {
-		printk(KERN_INFO "alps.c: Enabling hardware tapping\n");
-		if (alps_tap_mode(psmouse, 1))
-			printk(KERN_WARNING "alps.c: Failed to enable hardware tapping\n");
-	}
+	if (alps_tap_mode(psmouse, 1))
+		printk(KERN_WARNING "alps.c: Failed to enable hardware tapping\n");
 
 	if (alps_absolute_mode(psmouse)) {
 		printk(KERN_ERR "alps.c: Failed to enable absolute mode\n");
_

-- 
Peter Osterlund - petero2@telia.com
http://web.telia.com/~u89404340

Re: [PATCH] ALPS: fix enabling hardware tapping

[Vojtech Pavlik]

On Sun, Jul 03, 2005 at 01:49:13PM +0200, Peter Osterlund wrote:
> Dmitry Torokhov <dtor_core@ameritech.net> writes:
> 
> > It looks like logic for enabling hardware tapping in ALPS driver was
> > inverted and we enable it only if it was already enabled by BIOS or
> > firmware.
> 
> It looks like alps_init() has the same bug.  This patch fixes that
> function too by moving the check if the tapping mode needs to change
> into the alps_tap_mode() function, so that the test doesn't have to be
> duplicated.

This looks good. However - what's the point in checking whether tapping
is enabled before enabling it?

> Signed-off-by: Peter Osterlund <petero2@telia.com>
> ---
> 
>  linux-petero/drivers/input/mouse/alps.c |   29 +++++++++++------------------
>  1 files changed, 11 insertions(+), 18 deletions(-)
> 
> diff -puN drivers/input/mouse/alps.c~alps-hwtaps-fix drivers/input/mouse/alps.c
> --- linux/drivers/input/mouse/alps.c~alps-hwtaps-fix	2005-07-03 13:42:47.000000000 +0200
> +++ linux-petero/drivers/input/mouse/alps.c	2005-07-03 13:42:47.000000000 +0200
> @@ -2,7 +2,7 @@
>   * ALPS touchpad PS/2 mouse driver
>   *
>   * Copyright (c) 2003 Neil Brown <neilb@cse.unsw.edu.au>
> - * Copyright (c) 2003 Peter Osterlund <petero2@telia.com>
> + * Copyright (c) 2003-2005 Peter Osterlund <petero2@telia.com>
>   * Copyright (c) 2004 Dmitry Torokhov <dtor@mail.ru>
>   * Copyright (c) 2005 Vojtech Pavlik <vojtech@suse.cz>
>   *
> @@ -334,6 +334,13 @@ static int alps_tap_mode(struct psmouse 
>  	int cmd = enable ? PSMOUSE_CMD_SETRATE : PSMOUSE_CMD_SETRES;
>  	unsigned char tap_arg = enable ? 0x0A : 0x00;
>  	unsigned char param[4];
> +	int enabled;
> +
> +	if (alps_get_status(psmouse, param))
> +		return -1;
> +	enabled = (param[0] & 0x04) != 0;
> +	if (enabled == enable)
> +		return 0;
>  
>  	if (ps2_command(ps2dev, param, PSMOUSE_CMD_GETINFO) ||
>  	    ps2_command(ps2dev, NULL, PSMOUSE_CMD_DISABLE) ||
> @@ -350,7 +357,6 @@ static int alps_tap_mode(struct psmouse 
>  static int alps_reconnect(struct psmouse *psmouse)
>  {
>  	struct alps_data *priv = psmouse->private;
> -	unsigned char param[4];
>  	int version;
>  
>  	psmouse_reset(psmouse);
> @@ -361,11 +367,7 @@ static int alps_reconnect(struct psmouse
>  	if (priv->i->flags & ALPS_PASS && alps_passthrough_mode(psmouse, 1))
>  		return -1;
>  
> -	if (alps_get_status(psmouse, param))
> -		return -1;
> -
> -	if (!(param[0] & 0x04))
> -		alps_tap_mode(psmouse, 1);
> +	alps_tap_mode(psmouse, 1);
>  
>  	if (alps_absolute_mode(psmouse)) {
>  		printk(KERN_ERR "alps.c: Failed to enable absolute mode\n");
> @@ -389,7 +391,6 @@ static void alps_disconnect(struct psmou
>  int alps_init(struct psmouse *psmouse)
>  {
>  	struct alps_data *priv;
> -	unsigned char param[4];
>  	int version;
>  
>  	psmouse->private = priv = kmalloc(sizeof(struct alps_data), GFP_KERNEL);
> @@ -403,16 +404,8 @@ int alps_init(struct psmouse *psmouse)
>  	if ((priv->i->flags & ALPS_PASS) && alps_passthrough_mode(psmouse, 1))
>  		goto init_fail;
>  
> -	if (alps_get_status(psmouse, param)) {
> -		printk(KERN_ERR "alps.c: touchpad status report request failed\n");
> -		goto init_fail;
> -	}
> -
> -	if (param[0] & 0x04) {
> -		printk(KERN_INFO "alps.c: Enabling hardware tapping\n");
> -		if (alps_tap_mode(psmouse, 1))
> -			printk(KERN_WARNING "alps.c: Failed to enable hardware tapping\n");
> -	}
> +	if (alps_tap_mode(psmouse, 1))
> +		printk(KERN_WARNING "alps.c: Failed to enable hardware tapping\n");
>  
>  	if (alps_absolute_mode(psmouse)) {
>  		printk(KERN_ERR "alps.c: Failed to enable absolute mode\n");
 

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR

Re: [PATCH] ALPS: fix enabling hardware tapping

[Peter Osterlund]

Vojtech Pavlik <vojtech@suse.cz> writes:

> On Sun, Jul 03, 2005 at 01:49:13PM +0200, Peter Osterlund wrote:
> > Dmitry Torokhov <dtor_core@ameritech.net> writes:
> > 
> > > It looks like logic for enabling hardware tapping in ALPS driver was
> > > inverted and we enable it only if it was already enabled by BIOS or
> > > firmware.
> > 
> > It looks like alps_init() has the same bug.  This patch fixes that
> > function too by moving the check if the tapping mode needs to change
> > into the alps_tap_mode() function, so that the test doesn't have to be
> > duplicated.
> 
> This looks good. However - what's the point in checking whether tapping
> is enabled before enabling it?

I don't think there is a point. IFAIK this code was added by Dmitry as
part of the hardware auto-detection changes. In that version the check
prevented a printk line when the touchpad was already in the correct
state. That printk is deleted anyway by this patch, so the check can
be removed. (Modulo weird hardware behavior, which can't be completely
ruled out because the driver is based largely on reverse engineering,
since no public docs are available.)

Signed-off-by: Peter Osterlund <petero2@telia.com>
---

 linux-petero/drivers/input/mouse/alps.c |   22 ++++------------------
 1 files changed, 4 insertions(+), 18 deletions(-)

diff -puN drivers/input/mouse/alps.c~alps-hwtaps-fix drivers/input/mouse/alps.c
--- linux/drivers/input/mouse/alps.c~alps-hwtaps-fix	2005-07-03 13:42:47.000000000 +0200
+++ linux-petero/drivers/input/mouse/alps.c	2005-07-03 23:50:06.000000000 +0200
@@ -2,7 +2,7 @@
  * ALPS touchpad PS/2 mouse driver
  *
  * Copyright (c) 2003 Neil Brown <neilb@cse.unsw.edu.au>
- * Copyright (c) 2003 Peter Osterlund <petero2@telia.com>
+ * Copyright (c) 2003-2005 Peter Osterlund <petero2@telia.com>
  * Copyright (c) 2004 Dmitry Torokhov <dtor@mail.ru>
  * Copyright (c) 2005 Vojtech Pavlik <vojtech@suse.cz>
  *
@@ -350,7 +350,6 @@ static int alps_tap_mode(struct psmouse 
 static int alps_reconnect(struct psmouse *psmouse)
 {
 	struct alps_data *priv = psmouse->private;
-	unsigned char param[4];
 	int version;
 
 	psmouse_reset(psmouse);
@@ -361,11 +360,7 @@ static int alps_reconnect(struct psmouse
 	if (priv->i->flags & ALPS_PASS && alps_passthrough_mode(psmouse, 1))
 		return -1;
 
-	if (alps_get_status(psmouse, param))
-		return -1;
-
-	if (!(param[0] & 0x04))
-		alps_tap_mode(psmouse, 1);
+	alps_tap_mode(psmouse, 1);
 
 	if (alps_absolute_mode(psmouse)) {
 		printk(KERN_ERR "alps.c: Failed to enable absolute mode\n");
@@ -389,7 +384,6 @@ static void alps_disconnect(struct psmou
 int alps_init(struct psmouse *psmouse)
 {
 	struct alps_data *priv;
-	unsigned char param[4];
 	int version;
 
 	psmouse->private = priv = kmalloc(sizeof(struct alps_data), GFP_KERNEL);
@@ -403,16 +397,8 @@ int alps_init(struct psmouse *psmouse)
 	if ((priv->i->flags & ALPS_PASS) && alps_passthrough_mode(psmouse, 1))
 		goto init_fail;
 
-	if (alps_get_status(psmouse, param)) {
-		printk(KERN_ERR "alps.c: touchpad status report request failed\n");
-		goto init_fail;
-	}
-
-	if (param[0] & 0x04) {
-		printk(KERN_INFO "alps.c: Enabling hardware tapping\n");
-		if (alps_tap_mode(psmouse, 1))
-			printk(KERN_WARNING "alps.c: Failed to enable hardware tapping\n");
-	}
+	if (alps_tap_mode(psmouse, 1))
+		printk(KERN_WARNING "alps.c: Failed to enable hardware tapping\n");
 
 	if (alps_absolute_mode(psmouse)) {
 		printk(KERN_ERR "alps.c: Failed to enable absolute mode\n");
_

-- 
Peter Osterlund - petero2@telia.com
http://web.telia.com/~u89404340

Re: [PATCH] ALPS: fix enabling hardware tapping

[Dmitry Torokhov]

On Sunday 03 July 2005 18:28, Peter Osterlund wrote:
> Vojtech Pavlik <vojtech@suse.cz> writes:
> 
> > On Sun, Jul 03, 2005 at 01:49:13PM +0200, Peter Osterlund wrote:
> > > Dmitry Torokhov <dtor_core@ameritech.net> writes:
> > > 
> > > > It looks like logic for enabling hardware tapping in ALPS driver was
> > > > inverted and we enable it only if it was already enabled by BIOS or
> > > > firmware.
> > > 
> > > It looks like alps_init() has the same bug.  This patch fixes that
> > > function too by moving the check if the tapping mode needs to change
> > > into the alps_tap_mode() function, so that the test doesn't have to be
> > > duplicated.
> > 
> > This looks good. However - what's the point in checking whether tapping
> > is enabled before enabling it?
> 
> I don't think there is a point. IFAIK this code was added by Dmitry as
> part of the hardware auto-detection changes. In that version the check
> prevented a printk line when the touchpad was already in the correct
> state. That printk is deleted anyway by this patch, so the check can
> be removed. (Modulo weird hardware behavior, which can't be completely
> ruled out because the driver is based largely on reverse engineering,
> since no public docs are available.)
>
the only reason for not doing it unconditionally.

-- 
Dmitry

Re: [PATCH] ALPS: fix enabling hardware tapping

[Vojtech Pavlik]

On Mon, Jul 04, 2005 at 01:28:06AM +0200, Peter Osterlund wrote:

> Vojtech Pavlik <vojtech@suse.cz> writes:
> 
> > On Sun, Jul 03, 2005 at 01:49:13PM +0200, Peter Osterlund wrote:
> > > Dmitry Torokhov <dtor_core@ameritech.net> writes:
> > > 
> > > > It looks like logic for enabling hardware tapping in ALPS driver was
> > > > inverted and we enable it only if it was already enabled by BIOS or
> > > > firmware.
> > > 
> > > It looks like alps_init() has the same bug.  This patch fixes that
> > > function too by moving the check if the tapping mode needs to change
> > > into the alps_tap_mode() function, so that the test doesn't have to be
> > > duplicated.
> > 
> > This looks good. However - what's the point in checking whether tapping
> > is enabled before enabling it?
> 
> I don't think there is a point. IFAIK this code was added by Dmitry as
> part of the hardware auto-detection changes. In that version the check
> prevented a printk line when the touchpad was already in the correct
> state. That printk is deleted anyway by this patch, so the check can
> be removed. (Modulo weird hardware behavior, which can't be completely
> ruled out because the driver is based largely on reverse engineering,
> since no public docs are available.)
> 
> Signed-off-by: Peter Osterlund <petero2@telia.com>

Thanks; patch applied.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR