[PATCH 1/5] tsc2007: Debounce pressure measurement.

[PATCH 1/5] tsc2007: Debounce pressure measurement.

[Thierry Reding]

When the controller signals a pen-down event via the platform-specific
GPIO, while the sample values indicate an invalid measurement, the
measurement needs to be repeated.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Kwangwoo Lee <kwlee@mtekvision.com>
Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 drivers/input/touchscreen/tsc2007.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
index 80467f2..df4ae35 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -156,6 +156,7 @@ static void tsc2007_work(struct work_struct *work)
 {
 	struct tsc2007 *ts =
 		container_of(to_delayed_work(work), struct tsc2007, work);
+	bool debounced = false;
 	struct ts_event tc;
 	u32 rt;
 
@@ -191,6 +192,7 @@ static void tsc2007_work(struct work_struct *work)
 		 * repeat at least once more the measurement.
 		 */
 		dev_dbg(&ts->client->dev, "ignored pressure %d\n", rt);
+		debounced = true;
 		goto out;
 
 	}
@@ -225,7 +227,7 @@ static void tsc2007_work(struct work_struct *work)
 	}
 
  out:
-	if (ts->pendown)
+	if (ts->pendown || debounced)
 		schedule_delayed_work(&ts->work,
 				      msecs_to_jiffies(TS_POLL_PERIOD));
 	else
-- 
1.7.5.1

[PATCH 2/5] tsc2007: Add max_rt module parameter.

[Thierry Reding]

Finger touch events or very quick stylus events on low-quality panels
can cause the tsc2007 to read bogus values. Looking at oscilloscope
snapshots, this seems to be caused by the touch event disappearing
during the measurements. These bogus values result in misclicks, where
the X and Y values deviate from the real position.

Most of these misclicks can be filtered out by setting a low enough
threshold for the maximum resistance (which is loosely the inverse of
the pressure) allowed to consider a set of values valid. Since this
behaviour is largely dependent on the type and quality of the panel,
this commit introduces the max_rt parameter. The default value is kept
at MAX_12BIT.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Kwangwoo Lee <kwlee@mtekvision.com>
Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 drivers/input/touchscreen/tsc2007.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
index df4ae35..95d619c 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -83,6 +83,10 @@ struct tsc2007 {
 	void			(*clear_penirq)(void);
 };
 
+static int max_rt = MAX_12BIT;
+module_param(max_rt, int, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(max_rt, "Maximum resistance above which samples are ignored");
+
 static inline int tsc2007_xfer(struct tsc2007 *tsc, u8 cmd)
 {
 	s32 data;
@@ -185,7 +189,7 @@ static void tsc2007_work(struct work_struct *work)
 	tsc2007_read_values(ts, &tc);
 
 	rt = tsc2007_calculate_pressure(ts, &tc);
-	if (rt > MAX_12BIT) {
+	if (rt > max_rt) {
 		/*
 		 * Sample found inconsistent by debouncing or pressure is
 		 * beyond the maximum. Don't report it to user space,
-- 
1.7.5.1

Re: [PATCH 2/5] tsc2007: Add max_rt module parameter.

[Dmitry Torokhov]

Hi Thierry,

On Mon, May 16, 2011 at 10:33:00AM +0200, Thierry Reding wrote:
> Finger touch events or very quick stylus events on low-quality panels
> can cause the tsc2007 to read bogus values. Looking at oscilloscope
> snapshots, this seems to be caused by the touch event disappearing
> during the measurements. These bogus values result in misclicks, where
> the X and Y values deviate from the real position.
> 
> Most of these misclicks can be filtered out by setting a low enough
> threshold for the maximum resistance (which is loosely the inverse of
> the pressure) allowed to consider a set of values valid. Since this
> behaviour is largely dependent on the type and quality of the panel,
> this commit introduces the max_rt parameter. The default value is kept
> at MAX_12BIT.

I expect that the values, once selected, will not be changed for a given
panel, so why don't we pass max_rt (and poll_delay and poll_interval) vi
platform (board) data instead of being module parameters?

Thanks.

-- 
Dmitry

Re: [PATCH 2/5] tsc2007: Add max_rt module parameter.

[Thierry Reding]

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

* Dmitry Torokhov wrote:
> Hi Thierry,
> 
> On Mon, May 16, 2011 at 10:33:00AM +0200, Thierry Reding wrote:
> > Finger touch events or very quick stylus events on low-quality panels
> > can cause the tsc2007 to read bogus values. Looking at oscilloscope
> > snapshots, this seems to be caused by the touch event disappearing
> > during the measurements. These bogus values result in misclicks, where
> > the X and Y values deviate from the real position.
> > 
> > Most of these misclicks can be filtered out by setting a low enough
> > threshold for the maximum resistance (which is loosely the inverse of
> > the pressure) allowed to consider a set of values valid. Since this
> > behaviour is largely dependent on the type and quality of the panel,
> > this commit introduces the max_rt parameter. The default value is kept
> > at MAX_12BIT.
> 
> I expect that the values, once selected, will not be changed for a given
> panel, so why don't we pass max_rt (and poll_delay and poll_interval) vi
> platform (board) data instead of being module parameters?

I was using module parameters because it allows the parameters to be tuned at
runtime. You are correct however in that they don't vary after being selected
once. I can resend an updated patch series with those parameters added to the
platform data if you prefer.

By the way, Kwangwoo's email address no longer seems to be valid. Searching
the kernel tree, there also seems to be kwangwoo.lee@gmail.com. Do you know
if it is current?

Thierry

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

Re: [PATCH 2/5] tsc2007: Add max_rt module parameter.

[Dmitry Torokhov]

On Tue, May 17, 2011 at 07:46:20AM +0200, Thierry Reding wrote:
> * Dmitry Torokhov wrote:
> > Hi Thierry,
> > 
> > On Mon, May 16, 2011 at 10:33:00AM +0200, Thierry Reding wrote:
> > > Finger touch events or very quick stylus events on low-quality panels
> > > can cause the tsc2007 to read bogus values. Looking at oscilloscope
> > > snapshots, this seems to be caused by the touch event disappearing
> > > during the measurements. These bogus values result in misclicks, where
> > > the X and Y values deviate from the real position.
> > > 
> > > Most of these misclicks can be filtered out by setting a low enough
> > > threshold for the maximum resistance (which is loosely the inverse of
> > > the pressure) allowed to consider a set of values valid. Since this
> > > behaviour is largely dependent on the type and quality of the panel,
> > > this commit introduces the max_rt parameter. The default value is kept
> > > at MAX_12BIT.
> > 
> > I expect that the values, once selected, will not be changed for a given
> > panel, so why don't we pass max_rt (and poll_delay and poll_interval) vi
> > platform (board) data instead of being module parameters?
> 
> I was using module parameters because it allows the parameters to be tuned at
> runtime. You are correct however in that they don't vary after being selected
> once. I can resend an updated patch series with those parameters added to the
> platform data if you prefer.

Yes, please.

> 
> By the way, Kwangwoo's email address no longer seems to be valid. Searching
> the kernel tree, there also seems to be kwangwoo.lee@gmail.com. Do you know
> if it is current?

No, I do not.

-- 
Dmitry

Re: [PATCH 2/5] tsc2007: Add max_rt module parameter.

[Thierry Reding]

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

* Dmitry Torokhov wrote:
> Hi Thierry,
> 
> On Mon, May 16, 2011 at 10:33:00AM +0200, Thierry Reding wrote:
> > Finger touch events or very quick stylus events on low-quality panels
> > can cause the tsc2007 to read bogus values. Looking at oscilloscope
> > snapshots, this seems to be caused by the touch event disappearing
> > during the measurements. These bogus values result in misclicks, where
> > the X and Y values deviate from the real position.
> > 
> > Most of these misclicks can be filtered out by setting a low enough
> > threshold for the maximum resistance (which is loosely the inverse of
> > the pressure) allowed to consider a set of values valid. Since this
> > behaviour is largely dependent on the type and quality of the panel,
> > this commit introduces the max_rt parameter. The default value is kept
> > at MAX_12BIT.
> 
> I expect that the values, once selected, will not be changed for a given
> panel, so why don't we pass max_rt (and poll_delay and poll_interval) vi
> platform (board) data instead of being module parameters?

How do I best handle keeping the default values? Should I check whether the
value has been set at all in the platform data and set the default otherwise?
That would reserve the value 0 as special. Or should I just assume that all
the values must be set explicitly?

Thierry

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

Re: [PATCH 2/5] tsc2007: Add max_rt module parameter.

[Dmitry Torokhov]

On Tue, May 17, 2011 at 08:59:47AM +0200, Thierry Reding wrote:
> * Dmitry Torokhov wrote:
> > Hi Thierry,
> > 
> > On Mon, May 16, 2011 at 10:33:00AM +0200, Thierry Reding wrote:
> > > Finger touch events or very quick stylus events on low-quality panels
> > > can cause the tsc2007 to read bogus values. Looking at oscilloscope
> > > snapshots, this seems to be caused by the touch event disappearing
> > > during the measurements. These bogus values result in misclicks, where
> > > the X and Y values deviate from the real position.
> > > 
> > > Most of these misclicks can be filtered out by setting a low enough
> > > threshold for the maximum resistance (which is loosely the inverse of
> > > the pressure) allowed to consider a set of values valid. Since this
> > > behaviour is largely dependent on the type and quality of the panel,
> > > this commit introduces the max_rt parameter. The default value is kept
> > > at MAX_12BIT.
> > 
> > I expect that the values, once selected, will not be changed for a given
> > panel, so why don't we pass max_rt (and poll_delay and poll_interval) vi
> > platform (board) data instead of being module parameters?
> 
> How do I best handle keeping the default values? Should I check whether the
> value has been set at all in the platform data and set the default otherwise?
> That would reserve the value 0 as special. Or should I just assume that all
> the values must be set explicitly?

We need to try and keep being compatible with existing setups. Treating
0 as "unset" seems reasonable.

Thanks.

-- 
Dmitry

[PATCH 3/5] tsc2007: Introduce poll_delay parameter.

[Thierry Reding]

Depending on the quality of the touch panel, the time for the X-, X+, Y-
and Y+ inputs to settle may vary. The poll_delay parameter can be used
to override the default of 1 millisecond.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Kwangwoo Lee <kwlee@mtekvision.com>
Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 drivers/input/touchscreen/tsc2007.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
index 95d619c..43b5c09 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -27,7 +27,6 @@
 #include <linux/i2c.h>
 #include <linux/i2c/tsc2007.h>
 
-#define TS_POLL_DELAY			1 /* ms delay between samples */
 #define TS_POLL_PERIOD			1 /* ms delay between samples */
 
 #define TSC2007_MEASURE_TEMP0		(0x0 << 4)
@@ -83,6 +82,10 @@ struct tsc2007 {
 	void			(*clear_penirq)(void);
 };
 
+static int poll_delay = 1;
+module_param(poll_delay, int, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(poll_delay, "Delay (in ms) after pen-down event before polling starts");
+
 static int max_rt = MAX_12BIT;
 module_param(max_rt, int, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(max_rt, "Maximum resistance above which samples are ignored");
@@ -245,7 +248,7 @@ static irqreturn_t tsc2007_irq(int irq, void *handle)
 	if (!ts->get_pendown_state || likely(ts->get_pendown_state())) {
 		disable_irq_nosync(ts->irq);
 		schedule_delayed_work(&ts->work,
-				      msecs_to_jiffies(TS_POLL_DELAY));
+				      msecs_to_jiffies(poll_delay));
 	}
 
 	if (ts->clear_penirq)
-- 
1.7.5.1

[PATCH 4/5] tsc2007: Introduce poll_period parameter.

[Thierry Reding]

This new parameter allows the polling frequency to be configured while
keeping the default of once every millisecond.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Kwangwoo Lee <kwlee@mtekvision.com>
Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 drivers/input/touchscreen/tsc2007.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
index 43b5c09..cdaf06f 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -27,8 +27,6 @@
 #include <linux/i2c.h>
 #include <linux/i2c/tsc2007.h>
 
-#define TS_POLL_PERIOD			1 /* ms delay between samples */
-
 #define TSC2007_MEASURE_TEMP0		(0x0 << 4)
 #define TSC2007_MEASURE_AUX		(0x2 << 4)
 #define TSC2007_MEASURE_TEMP1		(0x4 << 4)
@@ -86,6 +84,10 @@ static int poll_delay = 1;
 module_param(poll_delay, int, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(poll_delay, "Delay (in ms) after pen-down event before polling starts");
 
+static int poll_period = 1;
+module_param(poll_period, int, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(poll_period, "Time (in ms) between successive samples");
+
 static int max_rt = MAX_12BIT;
 module_param(max_rt, int, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(max_rt, "Maximum resistance above which samples are ignored");
@@ -236,7 +238,7 @@ static void tsc2007_work(struct work_struct *work)
  out:
 	if (ts->pendown || debounced)
 		schedule_delayed_work(&ts->work,
-				      msecs_to_jiffies(TS_POLL_PERIOD));
+				      msecs_to_jiffies(poll_period));
 	else
 		enable_irq(ts->irq);
 }
-- 
1.7.5.1

[PATCH 5/5] tsc2007: Add X, Y and Z fuzz factors to platform data.

[Thierry Reding]

These new platform-specific values can be used to set the fuzz parameter
passed to the input_set_abs_params() function for the ABS_X, ABS_Y and
ABS_PRESSURE axes.

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Kwangwoo Lee <kwlee@mtekvision.com>
Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 drivers/input/touchscreen/tsc2007.c |    7 ++++---
 include/linux/i2c/tsc2007.h         |    3 +++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
index cdaf06f..8776999 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -316,9 +316,10 @@ static int __devinit tsc2007_probe(struct i2c_client *client,
 	input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
 	input_dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
 
-	input_set_abs_params(input_dev, ABS_X, 0, MAX_12BIT, 0, 0);
-	input_set_abs_params(input_dev, ABS_Y, 0, MAX_12BIT, 0, 0);
-	input_set_abs_params(input_dev, ABS_PRESSURE, 0, MAX_12BIT, 0, 0);
+	input_set_abs_params(input_dev, ABS_X, 0, MAX_12BIT, pdata->fuzzx, 0);
+	input_set_abs_params(input_dev, ABS_Y, 0, MAX_12BIT, pdata->fuzzy, 0);
+	input_set_abs_params(input_dev, ABS_PRESSURE, 0, MAX_12BIT,
+			pdata->fuzzz, 0);
 
 	if (pdata->init_platform_hw)
 		pdata->init_platform_hw();
diff --git a/include/linux/i2c/tsc2007.h b/include/linux/i2c/tsc2007.h
index c6361fb..003c8c8 100644
--- a/include/linux/i2c/tsc2007.h
+++ b/include/linux/i2c/tsc2007.h
@@ -6,6 +6,9 @@
 struct tsc2007_platform_data {
 	u16	model;				/* 2007. */
 	u16	x_plate_ohms;
+	int	fuzzx;				/* fuzz factor for X, Y and */
+	int	fuzzy;				/* pressure axes */
+	int	fuzzz;
 
 	int	(*get_pendown_state)(void);
 	void	(*clear_penirq)(void);		/* If needed, clear 2nd level
-- 
1.7.5.1

Re: [PATCH 1/5] tsc2007: Debounce pressure measurement.

[Dmitry Torokhov]

On Mon, May 16, 2011 at 10:32:59AM +0200, Thierry Reding wrote:
> When the controller signals a pen-down event via the platform-specific
> GPIO, while the sample values indicate an invalid measurement, the
> measurement needs to be repeated.
> 

Would not we be interrupted again and take another sample then?

Thanks.

-- 
Dmitry

Re: [PATCH 1/5] tsc2007: Debounce pressure measurement.

[Thierry Reding]

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

[Cc'ing Kwangwoo via the alternate email address]

* Dmitry Torokhov wrote:
> On Mon, May 16, 2011 at 10:32:59AM +0200, Thierry Reding wrote:
> > When the controller signals a pen-down event via the platform-specific
> > GPIO, while the sample values indicate an invalid measurement, the
> > measurement needs to be repeated.
> > 
> 
> Would not we be interrupted again and take another sample then?

Not necessarily. The problem is that if the pendown GPIO reports pendown, it
doesn't necessarily mean that the pressure measurement will be valid. This is
especially true if max_rt is configurable (as introduced by one of the
follow-up patchs).

What happens is that we are interrupted, check the GPIO to see that the pen
is indeed down and then read the values and compute the pressure to see that
it is invalid and we stop sampling. The TSC2007's nPEN_IRQ line never goes
high again after that because the pen is still down (according to the GPIO).

The comment in the old code of the (rt > max_rt) even says "[...] repeat at
least once more the measurement", which the old code actually doesn't.

Thierry

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

Re: [PATCH 1/5] tsc2007: Debounce pressure measurement.

[Dmitry Torokhov]

On Tue, May 17, 2011 at 08:27:16AM +0200, Thierry Reding wrote:
> [Cc'ing Kwangwoo via the alternate email address]
> 
> * Dmitry Torokhov wrote:
> > On Mon, May 16, 2011 at 10:32:59AM +0200, Thierry Reding wrote:
> > > When the controller signals a pen-down event via the platform-specific
> > > GPIO, while the sample values indicate an invalid measurement, the
> > > measurement needs to be repeated.
> > > 
> > 
> > Would not we be interrupted again and take another sample then?
> 
> Not necessarily. The problem is that if the pendown GPIO reports pendown, it
> doesn't necessarily mean that the pressure measurement will be valid. This is
> especially true if max_rt is configurable (as introduced by one of the
> follow-up patchs).
> 
> What happens is that we are interrupted, check the GPIO to see that the pen
> is indeed down and then read the values and compute the pressure to see that
> it is invalid and we stop sampling. The TSC2007's nPEN_IRQ line never goes
> high again after that because the pen is still down (according to the GPIO).
> 
> The comment in the old code of the (rt > max_rt) even says "[...] repeat at
> least once more the measurement", which the old code actually doesn't.
> 

I see. I am concerned with resubmitting work over and over when we do
not have ts->get_pendown_state method.

-- 
Dmitry

Re: [PATCH 1/5] tsc2007: Debounce pressure measurement.

[Thierry Reding]

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

* Dmitry Torokhov wrote:
> On Tue, May 17, 2011 at 08:27:16AM +0200, Thierry Reding wrote:
> > [Cc'ing Kwangwoo via the alternate email address]
> > 
> > * Dmitry Torokhov wrote:
> > > On Mon, May 16, 2011 at 10:32:59AM +0200, Thierry Reding wrote:
> > > > When the controller signals a pen-down event via the platform-specific
> > > > GPIO, while the sample values indicate an invalid measurement, the
> > > > measurement needs to be repeated.
> > > > 
> > > 
> > > Would not we be interrupted again and take another sample then?
> > 
> > Not necessarily. The problem is that if the pendown GPIO reports pendown, it
> > doesn't necessarily mean that the pressure measurement will be valid. This is
> > especially true if max_rt is configurable (as introduced by one of the
> > follow-up patchs).
> > 
> > What happens is that we are interrupted, check the GPIO to see that the pen
> > is indeed down and then read the values and compute the pressure to see that
> > it is invalid and we stop sampling. The TSC2007's nPEN_IRQ line never goes
> > high again after that because the pen is still down (according to the GPIO).
> > 
> > The comment in the old code of the (rt > max_rt) even says "[...] repeat at
> > least once more the measurement", which the old code actually doesn't.
> > 
> 
> I see. I am concerned with resubmitting work over and over when we do
> not have ts->get_pendown_state method.

I hadn't thought about that case. But it seems as if that case should behave
the same and stop rescheduling work as soon as no pressure is applied and rt
becomes 0.

Perhaps if I change this:

	debounced = true;

into

	if (ts->get_pendown_state)
		debounced = true;

it becomes more obvious that this "fix" only applies to the case where the
GPIO and pressure measurement contradict each other.

Thierry

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