ALPS: fix enabling hardware tapping

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!

Signature · 15 lines

Dmitry

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

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

Signed-off-by: Dmitry Torokhov <[email protected]>
---

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

Index: work/drivers/input/mouse/alps.c
===================================================================

drivers/input/mouse/alps.c +1 -1

@@ -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)) {

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.

Quoted 3 lines · Dmitry Torokhov

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 <[email protected]>

---

 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

drivers/input/mouse/alps.c renamed from drivers/input/mouse/alps.c~alps-hwtaps-fix +11 -18

@@ -2,7 +2,7 @@
  * ALPS touchpad PS/2 mouse driver
  *
  * Copyright (c) 2003 Neil Brown <[email protected]>
- * Copyright (c) 2003 Peter Osterlund <[email protected]>
+ * Copyright (c) 2003-2005 Peter Osterlund <[email protected]>
  * Copyright (c) 2004 Dmitry Torokhov <[email protected]>
  * Copyright (c) 2005 Vojtech Pavlik <[email protected]>
  *

@@ -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");

Quoted 6 lines · Peter Osterlund

Quoted 3 lines · Dmitry Torokhov

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.

Quoted 4 lines

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?

Quoted 7 lines · Vojtech Pavlik

Quoted 6 lines · Peter Osterlund

Quoted 3 lines · Dmitry Torokhov

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.

Quoted 7 lines

Quoted 4 lines

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.)

Quoted 7 lines · Vojtech Pavlik

Quoted 6 lines · Peter Osterlund

Quoted 3 lines · Dmitry Torokhov

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.

Quoted 7 lines

Quoted 4 lines

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 <[email protected]>