[PATCH 0/4] Input: wm97xx: Fix handling of wrong readings

[PATCH 0/4] Input: wm97xx: Fix handling of wrong readings

[Markus Pargmann]

Hi,

the series adds/fixes some handling for wrong readings of penup/down
and out of screen readings. Those wrong readings had a big impact on
the usability in my tests.

Regards,

Markus

[PATCH 1/4] Input: wm97xx: Drop out of range inputs

[Markus Pargmann]

With fast movements, there occured some out of screen jumps with my
touchscreen. The abs_x and abs_y module parameters should fix this by
default, but the driver doesn't actively checks the x/y coordinates.

Instead it seems that the input layer was supposed to drop out of range
inputs, as described in the comments:
"These parameters are used to help the input layer discard out of
range readings and reduce jitter etc"

The input layer documentation describes that values that are not in the
absolute range are also accepted.

So this patch adds a check within the driver.

Cc: stable@vger.kernel.org
Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/input/touchscreen/wm97xx-core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/input/touchscreen/wm97xx-core.c b/drivers/input/touchscreen/wm97xx-core.c
index 5dbe73a..e4c286b 100644
--- a/drivers/input/touchscreen/wm97xx-core.c
+++ b/drivers/input/touchscreen/wm97xx-core.c
@@ -442,6 +442,17 @@ static int wm97xx_read_samples(struct wm97xx *wm)
 			"pen down: x=%x:%d, y=%x:%d, pressure=%x:%d\n",
 			data.x >> 12, data.x & 0xfff, data.y >> 12,
 			data.y & 0xfff, data.p >> 12, data.p & 0xfff);
+
+		if (
+				abs_x[0] > (data.x & 0xfff)
+				|| abs_x[1] < (data.x & 0xfff)
+				|| abs_y[0] > (data.y & 0xfff)
+				|| abs_y[1] < (data.y & 0xfff)) {
+			dev_dbg(wm->dev, "Measurement out of range, dropping it\n");
+			rc = RC_AGAIN;
+			goto out;
+		}
+
 		input_report_abs(wm->input_dev, ABS_X, data.x & 0xfff);
 		input_report_abs(wm->input_dev, ABS_Y, data.y & 0xfff);
 		input_report_abs(wm->input_dev, ABS_PRESSURE, data.p & 0xfff);
@@ -455,6 +466,7 @@ static int wm97xx_read_samples(struct wm97xx *wm)
 		wm->ts_reader_interval = wm->ts_reader_min_interval;
 	}
 
+out:
 	mutex_unlock(&wm->codec_mutex);
 	return rc;
 }
-- 
1.8.1.4

Re: [PATCH 1/4] Input: wm97xx: Drop out of range inputs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 463 bytes --]

On Fri, Mar 08, 2013 at 05:15:06PM +0100, Markus Pargmann wrote:

> +		if (
> +				abs_x[0] > (data.x & 0xfff)
> +				|| abs_x[1] < (data.x & 0xfff)
> +				|| abs_y[0] > (data.y & 0xfff)
> +				|| abs_y[1] < (data.y & 0xfff)) {
> +			dev_dbg(wm->dev, "Measurement out of range, dropping it\n");
> +			rc = RC_AGAIN;
> +			goto out;

The change is good but not a fan of the coding style here.  Otherwise

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/4] Input: wm97xx: Drop out of range inputs

[Markus Pargmann]

On Sat, Mar 09, 2013 at 01:15:49AM +0800, Mark Brown wrote:
> On Fri, Mar 08, 2013 at 05:15:06PM +0100, Markus Pargmann wrote:
> 
> > +		if (
> > +				abs_x[0] > (data.x & 0xfff)
> > +				|| abs_x[1] < (data.x & 0xfff)
> > +				|| abs_y[0] > (data.y & 0xfff)
> > +				|| abs_y[1] < (data.y & 0xfff)) {
> > +			dev_dbg(wm->dev, "Measurement out of range, dropping it\n");
> > +			rc = RC_AGAIN;
> > +			goto out;
> 
> The change is good but not a fan of the coding style here.  Otherwise
> 
> Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Thanks. I would change the style to this:

		if (abs_x[0] > (data.x & 0xfff) ||
				abs_x[1] < (data.x & 0xfff) ||
				abs_y[0] > (data.y & 0xfff) ||
				abs_y[1] < (data.y & 0xfff)) {

Regards

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 2/4] Input: wm9712: Fix returncode for wrong sample

[Markus Pargmann]

Instead of interpreting a wrong measurement as pen up, we should try to read
again.

Based on wm9712: pen up by Teresa Gámez and Christian Hemp.

Cc: Teresa Gámez <t.gamez@phytec.de>
Cc: Christian Hemp <c.hemp@phytec.de>
Cc: stable@vger.kernel.org
Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/input/touchscreen/wm9712.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/wm9712.c b/drivers/input/touchscreen/wm9712.c
index 6e743e3..a983da1 100644
--- a/drivers/input/touchscreen/wm9712.c
+++ b/drivers/input/touchscreen/wm9712.c
@@ -298,7 +298,7 @@ static int wm9712_poll_sample(struct wm97xx *wm, int adcsel, int *sample)
 		dev_dbg(wm->dev, "adc wrong sample, wanted %x got %x",
 			adcsel & WM97XX_ADCSEL_MASK,
 			*sample & WM97XX_ADCSEL_MASK);
-		return RC_PENUP;
+		return RC_AGAIN;
 	}
 
 	if (wants_pen && !(*sample & WM97XX_PEN_DOWN)) {
-- 
1.8.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] Input: wm9712: Fix returncode for wrong sample

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 213 bytes --]

On Fri, Mar 08, 2013 at 05:15:07PM +0100, Markus Pargmann wrote:
> Instead of interpreting a wrong measurement as pen up, we should try to read
> again.

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 3/4] Input: wm9712: Fix wrong pen up readings

[Markus Pargmann]

Often a reading can be wrong. This patch assures that this is really a
pen up event and not a false reading.

Based on wm9712: pen up by Teresa Gámez and Christian Hemp.

Cc: Teresa Gámez <t.gamez@phytec.de>
Cc: Christian Hemp <c.hemp@phytec.de>
Cc: stable@vger.kernel.org
Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/input/touchscreen/wm9712.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/wm9712.c b/drivers/input/touchscreen/wm9712.c
index a983da1..3b4eed4 100644
--- a/drivers/input/touchscreen/wm9712.c
+++ b/drivers/input/touchscreen/wm9712.c
@@ -302,8 +302,12 @@ static int wm9712_poll_sample(struct wm97xx *wm, int adcsel, int *sample)
 	}
 
 	if (wants_pen && !(*sample & WM97XX_PEN_DOWN)) {
-		wm->pen_probably_down = 0;
-		return RC_PENUP;
+		/* Sometimes it reads a wrong value the first time. */
+		*sample = wm97xx_reg_read(wm, AC97_WM97XX_DIGITISER_RD);
+		if (!(*sample & WM97XX_PEN_DOWN)) {
+			wm->pen_probably_down = 0;
+			return RC_PENUP;
+		}
 	}
 
 	return RC_VALID;
-- 
1.8.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] Input: wm9712: Fix wrong pen up readings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 453 bytes --]

On Fri, Mar 08, 2013 at 05:15:08PM +0100, Markus Pargmann wrote:
> Often a reading can be wrong. This patch assures that this is really a
> pen up event and not a false reading.

Hrm, it feels like we should wait for the device to tell us another
reading is ready but on the other hand I can see the sort of issue that
might mean this is needed and if it improves performance in your
testing:

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 4/4] Input: wm9712: Fix dev_dbg newlines

[Markus Pargmann]

dev_dbg should end with a new line.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/input/touchscreen/wm9712.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/input/touchscreen/wm9712.c b/drivers/input/touchscreen/wm9712.c
index 3b4eed4..16b5211 100644
--- a/drivers/input/touchscreen/wm9712.c
+++ b/drivers/input/touchscreen/wm9712.c
@@ -162,14 +162,14 @@ static void wm9712_phy_init(struct wm97xx *wm)
 	if (rpu) {
 		dig2 &= 0xffc0;
 		dig2 |= WM9712_RPU(rpu);
-		dev_dbg(wm->dev, "setting pen detect pull-up to %d Ohms",
+		dev_dbg(wm->dev, "setting pen detect pull-up to %d Ohms\n",
 			64000 / rpu);
 	}
 
 	/* WM9712 five wire */
 	if (five_wire) {
 		dig2 |= WM9712_45W;
-		dev_dbg(wm->dev, "setting 5-wire touchscreen mode.");
+		dev_dbg(wm->dev, "setting 5-wire touchscreen mode.\n");
 
 		if (pil) {
 			dev_warn(wm->dev, "pressure measurement is not "
@@ -182,21 +182,21 @@ static void wm9712_phy_init(struct wm97xx *wm)
 	if (pil == 2) {
 		dig2 |= WM9712_PIL;
 		dev_dbg(wm->dev,
-			"setting pressure measurement current to 400uA.");
+			"setting pressure measurement current to 400uA.\n");
 	} else if (pil)
 		dev_dbg(wm->dev,
-			"setting pressure measurement current to 200uA.");
+			"setting pressure measurement current to 200uA.\n");
 	if (!pil)
 		pressure = 0;
 
 	/* polling mode sample settling delay */
 	if (delay < 0 || delay > 15) {
-		dev_dbg(wm->dev, "supplied delay out of range.");
+		dev_dbg(wm->dev, "supplied delay out of range.\n");
 		delay = 4;
 	}
 	dig1 &= 0xff0f;
 	dig1 |= WM97XX_DELAY(delay);
-	dev_dbg(wm->dev, "setting adc sample delay to %d u Secs.",
+	dev_dbg(wm->dev, "setting adc sample delay to %d u Secs.\n",
 		delay_table[delay]);
 
 	/* mask */
@@ -285,7 +285,7 @@ static int wm9712_poll_sample(struct wm97xx *wm, int adcsel, int *sample)
 		if (is_pden(wm))
 			wm->pen_probably_down = 0;
 		else
-			dev_dbg(wm->dev, "adc sample timeout");
+			dev_dbg(wm->dev, "adc sample timeout\n");
 		return RC_PENUP;
 	}
 
@@ -295,7 +295,7 @@ static int wm9712_poll_sample(struct wm97xx *wm, int adcsel, int *sample)
 
 	/* check we have correct sample */
 	if ((*sample ^ adcsel) & WM97XX_ADCSEL_MASK) {
-		dev_dbg(wm->dev, "adc wrong sample, wanted %x got %x",
+		dev_dbg(wm->dev, "adc wrong sample, wanted %x got %x\n",
 			adcsel & WM97XX_ADCSEL_MASK,
 			*sample & WM97XX_ADCSEL_MASK);
 		return RC_AGAIN;
@@ -349,7 +349,7 @@ static int wm9712_poll_coord(struct wm97xx *wm, struct wm97xx_data *data)
 		if (is_pden(wm))
 			wm->pen_probably_down = 0;
 		else
-			dev_dbg(wm->dev, "adc sample timeout");
+			dev_dbg(wm->dev, "adc sample timeout\n");
 		return RC_PENUP;
 	}
 
-- 
1.8.1.4

Re: [PATCH 4/4] Input: wm9712: Fix dev_dbg newlines

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 163 bytes --]

On Fri, Mar 08, 2013 at 05:15:09PM +0100, Markus Pargmann wrote:
> dev_dbg should end with a new line.

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]