[PATCH 0/3] Fix a regression in the FocalTech touchpad driver

[PATCH 0/3] Fix a regression in the FocalTech touchpad driver

[Mathias Gottschlag]

Hi,

(TLDR: these patches introduce a hack in the psmouse code required for
focaltech touchpads, I am not sure whether there is any better fix)

Patch 1 cleans the code and removes two unused constants.

Patches 2 and 3 fix some problems with the FocalTech driver. Because I
totally managed to mess up Hans' fix for RedHat bug 1110011 [1], the driver did
not work for any of the affected devices.

If the touchpad was sent certain commands, it would stop responding. I did not
read the code properly and assumed that only the probing code would trigger it,
but psmouse_initialize already was enough.

The current architecture always calls psmouse_initialize after the device
specific initialization code. I have added a flag which can be set by drivers
which disables the code within psmouse_initialize, because I did not see any
cleaner way to make sure that no additional commands are sent to the touchpad.

In the same way I also implemented two stub functions focaltech_set_rate and
focaltech_set_resolution which are called instead of the generic psmouse
functions. I did not actually check whether PSMOUSE_CMD_SETRES or
PSMOUSE_CMD_SETRATE alone are enough to break the touchpad because I didn't
have enough time (or access to the hardware), so I thought I'd just play safe
and disable these functions.

The previous patch probably introduced regressions compared to 3.19, so either
these patches (or something equivalent) should go into the next release, or the
previous patch should be temporarily reverted.

Regards,
Mathias

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1110011

Mathias Gottschlag (3):
  psmouse: Remove hardcoded touchpad size from the focaltech driver.
  psmouse: Skip psmouse_initialize for FocalTech touchpads.
  psmouse: Disable resolution/rate changes for FocalTech touchpads.

 drivers/input/mouse/focaltech.c    | 24 +++++++++++++++++-------
 drivers/input/mouse/psmouse-base.c |  4 +++-
 drivers/input/mouse/psmouse.h      |  5 +++++
 3 files changed, 25 insertions(+), 8 deletions(-)

-- 
2.1.0

[PATCH 1/3] psmouse: Remove hardcoded touchpad size from the focaltech driver.

[Mathias Gottschlag]

The size has in most cases already been fetched from the touchpad, the
hardcoded values should have been removed.

Signed-off-by: Mathias Gottschlag <mgottschlag@gmail.com>
---
 drivers/input/mouse/focaltech.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/input/mouse/focaltech.c b/drivers/input/mouse/focaltech.c
index fca38ba..5d8cf98 100644
--- a/drivers/input/mouse/focaltech.c
+++ b/drivers/input/mouse/focaltech.c
@@ -67,9 +67,6 @@ static void focaltech_reset(struct psmouse *psmouse)
 
 #define FOC_MAX_FINGERS 5
 
-#define FOC_MAX_X 2431
-#define FOC_MAX_Y 1663
-
 /*
  * Current state of a single finger on the touchpad.
  */
@@ -131,7 +128,7 @@ static void focaltech_report_state(struct psmouse *psmouse)
 		if (active) {
 			input_report_abs(dev, ABS_MT_POSITION_X, finger->x);
 			input_report_abs(dev, ABS_MT_POSITION_Y,
-					 FOC_MAX_Y - finger->y);
+					 priv->y_max - finger->y);
 		}
 	}
 	input_mt_report_pointer_emulation(dev, true);
-- 
2.1.0

Re: [PATCH 1/3] psmouse: Remove hardcoded touchpad size from the focaltech driver.

[Dmitry Torokhov]

On Sat, Feb 14, 2015 at 08:01:40PM +0100, Mathias Gottschlag wrote:
> The size has in most cases already been fetched from the touchpad, the
> hardcoded values should have been removed.
> 
> Signed-off-by: Mathias Gottschlag <mgottschlag@gmail.com>
> ---
>  drivers/input/mouse/focaltech.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/input/mouse/focaltech.c b/drivers/input/mouse/focaltech.c
> index fca38ba..5d8cf98 100644
> --- a/drivers/input/mouse/focaltech.c
> +++ b/drivers/input/mouse/focaltech.c
> @@ -67,9 +67,6 @@ static void focaltech_reset(struct psmouse *psmouse)
>  
>  #define FOC_MAX_FINGERS 5
>  
> -#define FOC_MAX_X 2431
> -#define FOC_MAX_Y 1663
> -
>  /*
>   * Current state of a single finger on the touchpad.
>   */
> @@ -131,7 +128,7 @@ static void focaltech_report_state(struct psmouse *psmouse)
>  		if (active) {
>  			input_report_abs(dev, ABS_MT_POSITION_X, finger->x);
>  			input_report_abs(dev, ABS_MT_POSITION_Y,
> -					 FOC_MAX_Y - finger->y);
> +					 priv->y_max - finger->y);

I wonder if we also want to camp teh vaue to 0, y_max... I know userspace is OK
with kernel reporting values greater than max value for the axis, but I worry
about negative vaues.

Thanks.

-- 
Dmitry

[PATCH 2/3] psmouse: Skip psmouse_initialize for FocalTech touchpads.

[Mathias Gottschlag]

The driver used to skip all initialization code for FocalTech touchpads
because they stop responding when certain commands are sent. The new
driver failed reproduce this behaviour and psmouse_reconnect would still
cause standard initialization code to be executed.

This patch introduces a field "skip_standard_init" to struct psmouse. If
this field is set to true, psmouse_initialize will not do anything. This
solution is somewhat ugly, but I believe it is the least ugly way to
introduce this apparently required special case.

Also, the code is modified to not send PSMOUSE_CMD_ENABLE twice (once in
focaltech.c, once in the generic code in psmouse-base.c).

Signed-off-by: Mathias Gottschlag <mgottschlag@gmail.com>
---
 drivers/input/mouse/focaltech.c    | 5 ++---
 drivers/input/mouse/psmouse-base.c | 4 +++-
 drivers/input/mouse/psmouse.h      | 5 +++++
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/input/mouse/focaltech.c b/drivers/input/mouse/focaltech.c
index 5d8cf98..05919cc 100644
--- a/drivers/input/mouse/focaltech.c
+++ b/drivers/input/mouse/focaltech.c
@@ -284,9 +284,6 @@ static int focaltech_switch_protocol(struct psmouse *psmouse)
 	if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETSCALE11))
 		return -EIO;
 
-	if (ps2_command(ps2dev, param, PSMOUSE_CMD_ENABLE))
-		return -EIO;
-
 	return 0;
 }
 
@@ -413,6 +410,8 @@ int focaltech_init(struct psmouse *psmouse)
 	/* resync is not supported yet */
 	psmouse->resync_time = 0;
 
+	psmouse->skip_standard_init = true;
+
 	return 0;
 
 fail:
diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
index 68469fe..b55a116 100644
--- a/drivers/input/mouse/psmouse-base.c
+++ b/drivers/input/mouse/psmouse-base.c
@@ -697,6 +697,8 @@ static void psmouse_apply_defaults(struct psmouse *psmouse)
 	psmouse->cleanup = NULL;
 	psmouse->pt_activate = NULL;
 	psmouse->pt_deactivate = NULL;
+
+	psmouse->skip_standard_init = false;
 }
 
 /*
@@ -1157,7 +1159,7 @@ static void psmouse_initialize(struct psmouse *psmouse)
  * We set the mouse report rate, resolution and scaling.
  */
 
-	if (psmouse_max_proto != PSMOUSE_PS2) {
+	if (psmouse_max_proto != PSMOUSE_PS2 && !psmouse->skip_standard_init) {
 		psmouse->set_rate(psmouse, psmouse->rate);
 		psmouse->set_resolution(psmouse, psmouse->resolution);
 		ps2_command(&psmouse->ps2dev, NULL, PSMOUSE_CMD_SETSCALE11);
diff --git a/drivers/input/mouse/psmouse.h b/drivers/input/mouse/psmouse.h
index c2ff137..d1412ac 100644
--- a/drivers/input/mouse/psmouse.h
+++ b/drivers/input/mouse/psmouse.h
@@ -64,6 +64,11 @@ struct psmouse {
 	unsigned int resync_time;
 	bool smartscroll;	/* Logitech only */
 
+	/* FocalTech touchpads sometimes stop responding when standard commands
+	 * are sent after the custom protocol has been selected, so this flag
+	 * makes the code skip psmouse_initialize */
+	bool skip_standard_init;
+
 	psmouse_ret_t (*protocol_handler)(struct psmouse *psmouse);
 	void (*set_rate)(struct psmouse *psmouse, unsigned int rate);
 	void (*set_resolution)(struct psmouse *psmouse, unsigned int resolution);
-- 
2.1.0

Re: [PATCH 2/3] psmouse: Skip psmouse_initialize for FocalTech touchpads.

[Dmitry Torokhov]

Hi Mathias,

On Sat, Feb 14, 2015 at 08:01:41PM +0100, Mathias Gottschlag wrote:
>  /*
> @@ -1157,7 +1159,7 @@ static void psmouse_initialize(struct psmouse *psmouse)
>   * We set the mouse report rate, resolution and scaling.
>   */
>  
> -	if (psmouse_max_proto != PSMOUSE_PS2) {
> +	if (psmouse_max_proto != PSMOUSE_PS2 && !psmouse->skip_standard_init) {
>  		psmouse->set_rate(psmouse, psmouse->rate);
>  		psmouse->set_resolution(psmouse, psmouse->resolution);
>  		ps2_command(&psmouse->ps2dev, NULL, PSMOUSE_CMD_SETSCALE11);

Instead of introducing the new skip_standard_init flag can you instead define a
pointer for set_scale implementation and provide a dummy stub for it in
focatech, the same as you do for set_rate() and set_resoluton() in patch 3?

Thanks.

-- 
Dmitry

[PATCH 3/3] psmouse: Disable resolution/rate changes for FocalTech touchpads.

[Mathias Gottschlag]

These PS/2 commands might already be enough to make FocalTech touchpads
stop responding.

Signed-off-by: Mathias Gottschlag <mgottschlag@gmail.com>
---
 drivers/input/mouse/focaltech.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/input/mouse/focaltech.c b/drivers/input/mouse/focaltech.c
index 05919cc..3d879c5 100644
--- a/drivers/input/mouse/focaltech.c
+++ b/drivers/input/mouse/focaltech.c
@@ -375,6 +375,17 @@ static int focaltech_read_size(struct psmouse *psmouse)
 
 	return 0;
 }
+
+void focaltech_set_resolution(struct psmouse *psmouse, unsigned int resolution)
+{
+	/* not supported yet */
+}
+
+static void focaltech_set_rate(struct psmouse *psmouse, unsigned int rate)
+{
+	/* not supported yet */
+}
+
 int focaltech_init(struct psmouse *psmouse)
 {
 	struct focaltech_data *priv;
@@ -409,6 +420,9 @@ int focaltech_init(struct psmouse *psmouse)
 	psmouse->cleanup = focaltech_reset;
 	/* resync is not supported yet */
 	psmouse->resync_time = 0;
+	/* rate/resolution changes are not supported yet */
+	psmouse->set_resolution = focaltech_set_resolution;
+	psmouse->set_rate = focaltech_set_rate;
 
 	psmouse->skip_standard_init = true;
 
-- 
2.1.0

[PATCHv2 0/4] FocalTech touchpad fixes

[Mathias Gottschlag]

Hi,

Changes since the last version:
- I added a commit which clamps the reported coordinates to valid values, even
  if the values reported by the touchpad are wrong/misinterpreted.
- PSMOUSE_CMD_SETSCALE11 is now encapsulated in a member function of struct
  psmouse, like the other configuration commands, and focaltech.c provides
  a dummy function.
- I added a commit which disables palm detection. The driver used to check the
  reported contact size from the touchpad. However, the threshold of the value
  for "very large contact area" seems to be rather low on some devices, leading
  to frequent pointer freezes. As I don't know how to indentify these devices,
  I thought it would be better to disable the check for now. If you don't agree
  with this patch, we can surely leave it out, but I don't know how to fix the
  problem and would rather not have devices where the driver is known to be
  broken.

Especially, the last commit now removes some code which so far is the only
available documentation for the touchpad behaviour.
As soon as I find some time (haha), I'll probably write some documentation.
Would such documentation (as incomplete and probably wrong as it will be) be
suitable for Documentation/, or should I make it available through a different
channel?

Regards,
Mathias

Mathias Gottschlag (4):
  psmouse: Remove hardcoded touchpad size from the focaltech driver.
  psmouse: Ensure that the focaltech driver reports consistent
    coordinates.
  psmouse: Disable resolution/rate/scale changes for FocalTech
    touchpads.
  psmouse: Disable "palm detection" in the focaltech driver.

 drivers/input/mouse/focaltech.c    | 49 ++++++++++++++++++++++++++------------
 drivers/input/mouse/psmouse-base.c | 18 +++++++++++++-
 drivers/input/mouse/psmouse.h      |  7 ++++++
 3 files changed, 58 insertions(+), 16 deletions(-)

-- 
2.1.0

[PATCHv2 1/4] psmouse: Remove hardcoded touchpad size from the focaltech driver.

[Mathias Gottschlag]

The size has in most cases already been fetched from the touchpad, the
hardcoded values should have been removed.

Signed-off-by: Mathias Gottschlag <mgottschlag@gmail.com>
---
 drivers/input/mouse/focaltech.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/input/mouse/focaltech.c b/drivers/input/mouse/focaltech.c
index fca38ba..5d8cf98 100644
--- a/drivers/input/mouse/focaltech.c
+++ b/drivers/input/mouse/focaltech.c
@@ -67,9 +67,6 @@ static void focaltech_reset(struct psmouse *psmouse)
 
 #define FOC_MAX_FINGERS 5
 
-#define FOC_MAX_X 2431
-#define FOC_MAX_Y 1663
-
 /*
  * Current state of a single finger on the touchpad.
  */
@@ -131,7 +128,7 @@ static void focaltech_report_state(struct psmouse *psmouse)
 		if (active) {
 			input_report_abs(dev, ABS_MT_POSITION_X, finger->x);
 			input_report_abs(dev, ABS_MT_POSITION_Y,
-					 FOC_MAX_Y - finger->y);
+					 priv->y_max - finger->y);
 		}
 	}
 	input_mt_report_pointer_emulation(dev, true);
-- 
2.1.0

[PATCHv2 2/4] psmouse: Ensure that the focaltech driver reports consistent coordinates.

[Mathias Gottschlag]

We don't know whether x_max or y_max really hold the maximum possible
coordinates, and we don't know for sure whether we correctly interpret the
coordinates sent by the touchpad, so we clamp the reported values to
prevent confusion in userspace code.

Signed-off-by: Mathias Gottschlag <mgottschlag@gmail.com>
---
 drivers/input/mouse/focaltech.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/input/mouse/focaltech.c b/drivers/input/mouse/focaltech.c
index 5d8cf98..0cfc646 100644
--- a/drivers/input/mouse/focaltech.c
+++ b/drivers/input/mouse/focaltech.c
@@ -126,9 +126,17 @@ static void focaltech_report_state(struct psmouse *psmouse)
 		input_mt_slot(dev, i);
 		input_mt_report_slot_state(dev, MT_TOOL_FINGER, active);
 		if (active) {
-			input_report_abs(dev, ABS_MT_POSITION_X, finger->x);
+			int clamped_x, clamped_y;
+			/*
+			 * The touchpad might report invalid data, so we clamp
+			 * the resulting values so that we do not confuse
+			 * userspace.
+			 */
+			clamped_x = clamp((int)finger->x, 0, (int)priv->x_max);
+			clamped_y = clamp((int)finger->y, 0, (int)priv->y_max);
+			input_report_abs(dev, ABS_MT_POSITION_X, clamped_x);
 			input_report_abs(dev, ABS_MT_POSITION_Y,
-					 priv->y_max - finger->y);
+					 priv->y_max - clamped_y);
 		}
 	}
 	input_mt_report_pointer_emulation(dev, true);
-- 
2.1.0

[PATCHv2 3/4] psmouse: Disable resolution/rate/scale changes for FocalTech touchpads.

[Mathias Gottschlag]

These PS/2 commands make some touchpads stop responding, so this commit
adds some dummy functions to replace the generic implementation. Because
scale changes were not encapsulated in a method of struct psmouse yet, this
commit adds a method set_scale to psmouse.

Signed-off-by: Mathias Gottschlag <mgottschlag@gmail.com>
---
 drivers/input/mouse/focaltech.c    | 24 ++++++++++++++++++++++++
 drivers/input/mouse/psmouse-base.c | 18 +++++++++++++++++-
 drivers/input/mouse/psmouse.h      |  7 +++++++
 3 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/input/mouse/focaltech.c b/drivers/input/mouse/focaltech.c
index 0cfc646..d96a847 100644
--- a/drivers/input/mouse/focaltech.c
+++ b/drivers/input/mouse/focaltech.c
@@ -386,6 +386,23 @@ static int focaltech_read_size(struct psmouse *psmouse)
 
 	return 0;
 }
+
+void focaltech_set_resolution(struct psmouse *psmouse, unsigned int resolution)
+{
+	/* not supported yet */
+}
+
+static void focaltech_set_rate(struct psmouse *psmouse, unsigned int rate)
+{
+	/* not supported yet */
+}
+
+static void focaltech_set_scale(struct psmouse *psmouse,
+				enum psmouse_scale scale)
+{
+	/* not supported yet */
+}
+
 int focaltech_init(struct psmouse *psmouse)
 {
 	struct focaltech_data *priv;
@@ -420,6 +437,13 @@ int focaltech_init(struct psmouse *psmouse)
 	psmouse->cleanup = focaltech_reset;
 	/* resync is not supported yet */
 	psmouse->resync_time = 0;
+	/*
+	 * rate/resolution/scale changes are not supported yet, and the generic
+	 * implementations of these functions seem to confuse some touchpads
+	 */
+	psmouse->set_resolution = focaltech_set_resolution;
+	psmouse->set_rate = focaltech_set_rate;
+	psmouse->set_scale = focaltech_set_scale;
 
 	return 0;
 
diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
index 68469fe..58537eb 100644
--- a/drivers/input/mouse/psmouse-base.c
+++ b/drivers/input/mouse/psmouse-base.c
@@ -454,6 +454,20 @@ static void psmouse_set_rate(struct psmouse *psmouse, unsigned int rate)
 }
 
 /*
+ * Here we set the mouse scaling.
+ */
+
+static void psmouse_set_scale(struct psmouse *psmouse, enum psmouse_scale scale)
+{
+	if (scale == PSMOUSE_SCALE21) {
+		ps2_command(&psmouse->ps2dev, NULL, PSMOUSE_CMD_SETSCALE21);
+	} else {
+		ps2_command(&psmouse->ps2dev, NULL, PSMOUSE_CMD_SETSCALE11);
+	}
+	psmouse->scale = scale;
+}
+
+/*
  * psmouse_poll() - default poll handler. Everyone except for ALPS uses it.
  */
 
@@ -689,6 +703,7 @@ static void psmouse_apply_defaults(struct psmouse *psmouse)
 
 	psmouse->set_rate = psmouse_set_rate;
 	psmouse->set_resolution = psmouse_set_resolution;
+	psmouse->set_scale = psmouse_set_scale;
 	psmouse->poll = psmouse_poll;
 	psmouse->protocol_handler = psmouse_process_byte;
 	psmouse->pktsize = 3;
@@ -1160,7 +1175,7 @@ static void psmouse_initialize(struct psmouse *psmouse)
 	if (psmouse_max_proto != PSMOUSE_PS2) {
 		psmouse->set_rate(psmouse, psmouse->rate);
 		psmouse->set_resolution(psmouse, psmouse->resolution);
-		ps2_command(&psmouse->ps2dev, NULL, PSMOUSE_CMD_SETSCALE11);
+		psmouse->set_scale(psmouse, PSMOUSE_SCALE11);
 	}
 }
 
@@ -1492,6 +1507,7 @@ static int psmouse_connect(struct serio *serio, struct serio_driver *drv)
 
 	psmouse->rate = psmouse_rate;
 	psmouse->resolution = psmouse_resolution;
+	psmouse->scale = PSMOUSE_SCALE11;
 	psmouse->resetafter = psmouse_resetafter;
 	psmouse->resync_time = parent ? 0 : psmouse_resync_time;
 	psmouse->smartscroll = psmouse_smartscroll;
diff --git a/drivers/input/mouse/psmouse.h b/drivers/input/mouse/psmouse.h
index c2ff137..80fc62c 100644
--- a/drivers/input/mouse/psmouse.h
+++ b/drivers/input/mouse/psmouse.h
@@ -36,6 +36,11 @@ typedef enum {
 	PSMOUSE_FULL_PACKET
 } psmouse_ret_t;
 
+enum psmouse_scale {
+	PSMOUSE_SCALE11,
+	PSMOUSE_SCALE21
+};
+
 struct psmouse {
 	void *private;
 	struct input_dev *dev;
@@ -60,6 +65,7 @@ struct psmouse {
 
 	unsigned int rate;
 	unsigned int resolution;
+	enum psmouse_scale scale;
 	unsigned int resetafter;
 	unsigned int resync_time;
 	bool smartscroll;	/* Logitech only */
@@ -67,6 +73,7 @@ struct psmouse {
 	psmouse_ret_t (*protocol_handler)(struct psmouse *psmouse);
 	void (*set_rate)(struct psmouse *psmouse, unsigned int rate);
 	void (*set_resolution)(struct psmouse *psmouse, unsigned int resolution);
+	void (*set_scale)(struct psmouse *psmouse, enum psmouse_scale scale);
 
 	int (*reconnect)(struct psmouse *psmouse);
 	void (*disconnect)(struct psmouse *psmouse);
-- 
2.1.0

Re: [PATCHv2 3/4] psmouse: Disable resolution/rate/scale changes for FocalTech touchpads.

[Dmitry Torokhov]

Hi Mathias,

On Fri, Feb 27, 2015 at 12:20:07AM +0100, Mathias Gottschlag wrote:
> These PS/2 commands make some touchpads stop responding, so this commit
> adds some dummy functions to replace the generic implementation. Because
> @@ -60,6 +65,7 @@ struct psmouse {
>  
>  	unsigned int rate;
>  	unsigned int resolution;
> +	enum psmouse_scale scale;
>  	unsigned int resetafter;
>  	unsigned int resync_time;
>  	bool smartscroll;	/* Logitech only */

I do not believe we need to store the scaling factor so I dropped these
chunks, otherwise applied all 4.

Thanks.

-- 
Dmitry

[PATCHv2 4/4] psmouse: Disable "palm detection" in the focaltech driver.

[Mathias Gottschlag]

Apparently, the threshold for large contact area seems to be rather low on
some devices, causing the touchpad to frequently freeze during normal
usage. Because we do now know how we are supposed to use the value in
question, this commit just drops the related code completely.

Signed-off-by: Mathias Gottschlag <mgottschlag@gmail.com>
---
 drivers/input/mouse/focaltech.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/input/mouse/focaltech.c b/drivers/input/mouse/focaltech.c
index d96a847..e1d084a 100644
--- a/drivers/input/mouse/focaltech.c
+++ b/drivers/input/mouse/focaltech.c
@@ -185,16 +185,6 @@ static void focaltech_process_abs_packet(struct psmouse *psmouse,
 
 	state->pressed = (packet[0] >> 4) & 1;
 
-	/*
-	 * packet[5] contains some kind of tool size in the most
-	 * significant nibble. 0xff is a special value (latching) that
-	 * signals a large contact area.
-	 */
-	if (packet[5] == 0xff) {
-		state->fingers[finger].valid = false;
-		return;
-	}
-
 	state->fingers[finger].x = ((packet[1] & 0xf) << 8) | packet[2];
 	state->fingers[finger].y = (packet[3] << 8) | packet[4];
 	state->fingers[finger].valid = true;
-- 
2.1.0