[PATCH 1/2] Input: tsc2007 - Disable irq when the irq thread is handling data

[PATCH 1/2] Input: tsc2007 - Disable irq when the irq thread is handling data

[Feng Tang]

The TSC2007 data sheet say in some case the HW may fire some false
interrrupt, which I also met during integrating one TSC2007 device.
So add the disable_irq/enable_irq protection around data handling.

Signed-off-by: Feng Tang <feng.tang@intel.com>
---
 drivers/input/touchscreen/tsc2007.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
index 1f674cb..789f216 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -171,6 +171,13 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
 	struct ts_event tc;
 	u32 rt;
 
+	/*
+	 * Disable the irq, as it may fire several other IRQs during
+	 * this thread is handling data, as suggested by the TSC2007
+	 * datasheet, p19, "Touch Detect" chapter
+	 */
+	disable_irq_nosync(irq);
+
 	while (!ts->stopped && tsc2007_is_pen_down(ts)) {
 
 		/* pen is down, continue with the measurement */
@@ -221,6 +228,7 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
 	if (ts->clear_penirq)
 		ts->clear_penirq();
 
+	enable_irq(irq);
 	return IRQ_HANDLED;
 }
 
-- 
1.7.1

[PATCH 2/2] Input: tsc2007 - Add a z1_low_threshhold platform data parameter

[Feng Tang]

This originates from a patch in Meego IVI kernel with the name
	linux-2.6.37-connext-0027-tsc2007.patch
There is no author info excepte a line "From  MeeGo <kernel@meego.com>"

When integrating tsc2007 on Intel IVI platform, there are a lot of noise
data whose z1 value is around 10 which is not a sane "z1". So add
a "z1_low_threshhold" to filter those nosie data, to make the device work
properly.

Signed-off-by: Feng Tang <feng.tang@intel.com>
---
 drivers/input/touchscreen/tsc2007.c |    4 +++-
 include/linux/i2c/tsc2007.h         |    1 +
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
index 789f216..1bf3503 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -72,6 +72,7 @@ struct tsc2007 {
 	u16			model;
 	u16			x_plate_ohms;
 	u16			max_rt;
+	u16			z1_low_threshhold;
 	unsigned long		poll_delay;
 	unsigned long		poll_period;
 
@@ -130,7 +131,7 @@ static u32 tsc2007_calculate_pressure(struct tsc2007 *tsc, struct ts_event *tc)
 	if (tc->x == MAX_12BIT)
 		tc->x = 0;
 
-	if (likely(tc->x && tc->z1)) {
+	if (likely(tc->x && tc->z1 > tsc->z1_low_threshhold)) {
 		/* compute touch pressure resistance using equation #1 */
 		rt = tc->z2 - tc->z1;
 		rt *= tc->x;
@@ -313,6 +314,7 @@ static int __devinit tsc2007_probe(struct i2c_client *client,
 	ts->model             = pdata->model;
 	ts->x_plate_ohms      = pdata->x_plate_ohms;
 	ts->max_rt            = pdata->max_rt ? : MAX_12BIT;
+	ts->z1_low_threshhold = pdata->z1_low_threshhold ? : 1;
 	ts->poll_delay        = pdata->poll_delay ? : 1;
 	ts->poll_period       = pdata->poll_period ? : 1;
 	ts->get_pendown_state = pdata->get_pendown_state;
diff --git a/include/linux/i2c/tsc2007.h b/include/linux/i2c/tsc2007.h
index 506a9f7..638c6c7 100644
--- a/include/linux/i2c/tsc2007.h
+++ b/include/linux/i2c/tsc2007.h
@@ -7,6 +7,7 @@ struct tsc2007_platform_data {
 	u16	model;				/* 2007. */
 	u16	x_plate_ohms;	/* must be non-zero value */
 	u16	max_rt; /* max. resistance above which samples are ignored */
+	u16	z1_low_threshhold; /* threshhold to prevent some noise data */
 	unsigned long poll_delay; /* delay (in ms) after pen-down event
 				     before polling starts */
 	unsigned long poll_period; /* time (in ms) between samples */
-- 
1.7.1

Re: [PATCH 1/2] Input: tsc2007 - Disable irq when the irq thread is handling data

[Dmitry Torokhov]

Hi Feng,

On Tue, Nov 29, 2011 at 04:12:57PM +0800, Feng Tang wrote:
> The TSC2007 data sheet say in some case the HW may fire some false
> interrrupt, which I also met during integrating one TSC2007 device.
> So add the disable_irq/enable_irq protection around data handling.

IRQF_ONESHOT should prevent IRQ from firing again while thread is
servicing it. Did you actually observe it not working?

Thanks.

> 
> Signed-off-by: Feng Tang <feng.tang@intel.com>
> ---
>  drivers/input/touchscreen/tsc2007.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
> index 1f674cb..789f216 100644
> --- a/drivers/input/touchscreen/tsc2007.c
> +++ b/drivers/input/touchscreen/tsc2007.c
> @@ -171,6 +171,13 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
>  	struct ts_event tc;
>  	u32 rt;
>  
> +	/*
> +	 * Disable the irq, as it may fire several other IRQs during
> +	 * this thread is handling data, as suggested by the TSC2007
> +	 * datasheet, p19, "Touch Detect" chapter
> +	 */
> +	disable_irq_nosync(irq);
> +
>  	while (!ts->stopped && tsc2007_is_pen_down(ts)) {
>  
>  		/* pen is down, continue with the measurement */
> @@ -221,6 +228,7 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
>  	if (ts->clear_penirq)
>  		ts->clear_penirq();
>  
> +	enable_irq(irq);
>  	return IRQ_HANDLED;
>  }
>  
> -- 
> 1.7.1
> 
> 

-- 
Dmitry

Re: [PATCH 2/2] Input: tsc2007 - Add a z1_low_threshhold platform data parameter

[Dmitry Torokhov]

On Tue, Nov 29, 2011 at 04:12:58PM +0800, Feng Tang wrote:
> This originates from a patch in Meego IVI kernel with the name
> 	linux-2.6.37-connext-0027-tsc2007.patch
> There is no author info excepte a line "From  MeeGo <kernel@meego.com>"
> 
> When integrating tsc2007 on Intel IVI platform, there are a lot of noise
> data whose z1 value is around 10 which is not a sane "z1". So add
> a "z1_low_threshhold" to filter those nosie data, to make the device work
> properly.

Sounds like a task for userspace to ignore pressure that is too low. Bonus
points for making it configurable so user can adjust sensitivity.

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] Input: tsc2007 - Disable irq when the irq thread is handling data

[Feng Tang]

Hi Dmitry,

Thanks for the review.

On Tue, 29 Nov 2011 17:22:08 +0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> Hi Feng,
> 
> On Tue, Nov 29, 2011 at 04:12:57PM +0800, Feng Tang wrote:
> > The TSC2007 data sheet say in some case the HW may fire some false
> > interrrupt, which I also met during integrating one TSC2007 device.
> > So add the disable_irq/enable_irq protection around data handling.
> 
> IRQF_ONESHOT should prevent IRQ from firing again while thread is
> servicing it. Did you actually observe it not working?

You are right, the tsc's threaded IRQ function is not re-entered, and
the driver is working actually. My bad not stating the problem clearly.
The real problem I want solve is, many platforms including ours use a
GPIO line as the tsc2007's IRQ line, and when these extra tsc2007 IRQ 
is triggered on the gpio line, that GPIO controller will fire up extra
noise IRQ accordingly, causing its ISR to be called. And my patch is
 trying to let the GPIO controller driver disable that specific IRQ pin
from tsc2007. As disable_irq will call GPIO irq_chip's irq_disable() or
mask() hook.

By grep the tsc2007_platform_data in kernel, I see most of the most of
the tsc2007 platforms are using GPIO line as tsc2007 IRQ ine. So this
should be a general problem.

Following is patch with updated log and comments.

Thanks,
Feng

-----------------

>From 16585020d27a066d2908959e370ea4f8905a0d34 Mon Sep 17 00:00:00 2001
From: Feng Tang <feng.tang@intel.com>
Date: Tue, 29 Nov 2011 15:48:42 +0800
Subject: [PATCH 1/2] Input: tsc2007 - Disable irq when the irq thread is handling data

The TSC2007 data sheet say in some case the HW may fire some false
interrrupt, which I also met during integrating one TSC2007 device.
Most of the tsc2007 platforms including ours are using a gpio line as
tsc2007's irq line, so calling disable_irq_nosync() to actually
prevent the gpio controller from firing IRQ for tsc2007 in such case.

Signed-off-by: Feng Tang <feng.tang@intel.com>
---
 drivers/input/touchscreen/tsc2007.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
index 1f674cb..03c1961 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -171,6 +171,18 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
 	struct ts_event tc;
 	u32 rt;
 
+	/*
+	 * Disable the irq, as it may fire several other IRQs during
+	 * this thread is handling data, as suggested by the TSC2007
+	 * datasheet, p19, "Touch Detect" chapter.
+	 *
+	 * Most of the tsc2007 platforms are using a gpio line as
+	 * tsc2007's irq line, so calling disable_irq_nosync() will
+	 * actually prevent the gpio controller from firing IRQ for
+	 * this tsc2007 line in such case.
+	 */
+	disable_irq_nosync(irq);
+
 	while (!ts->stopped && tsc2007_is_pen_down(ts)) {
 
 		/* pen is down, continue with the measurement */
@@ -221,6 +233,7 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
 	if (ts->clear_penirq)
 		ts->clear_penirq();
 
+	enable_irq(irq);
 	return IRQ_HANDLED;
 }
 
-- 
1.7.1
> 
> Thanks.
> 
> > 
> > Signed-off-by: Feng Tang <feng.tang@intel.com>
> > ---
> >  drivers/input/touchscreen/tsc2007.c |    8 ++++++++
> >  1 files changed, 8 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/input/touchscreen/tsc2007.c
> > b/drivers/input/touchscreen/tsc2007.c index 1f674cb..789f216 100644
> > --- a/drivers/input/touchscreen/tsc2007.c
> > +++ b/drivers/input/touchscreen/tsc2007.c
> > @@ -171,6 +171,13 @@ static irqreturn_t tsc2007_soft_irq(int irq,
> > void *handle) struct ts_event tc;
> >  	u32 rt;
> >  
> > +	/*
> > +	 * Disable the irq, as it may fire several other IRQs
> > during
> > +	 * this thread is handling data, as suggested by the
> > TSC2007
> > +	 * datasheet, p19, "Touch Detect" chapter
> > +	 */
> > +	disable_irq_nosync(irq);
> > +
> >  	while (!ts->stopped && tsc2007_is_pen_down(ts)) {
> >  
> >  		/* pen is down, continue with the measurement */
> > @@ -221,6 +228,7 @@ static irqreturn_t tsc2007_soft_irq(int irq,
> > void *handle) if (ts->clear_penirq)
> >  		ts->clear_penirq();
> >  
> > +	enable_irq(irq);
> >  	return IRQ_HANDLED;
> >  }
> >  
> > -- 
> > 1.7.1
> > 
> > 
> 

Re: [PATCH 2/2] Input: tsc2007 - Add a z1_low_threshhold platform data parameter

[Feng Tang]

On Tue, 29 Nov 2011 17:23:10 +0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Tue, Nov 29, 2011 at 04:12:58PM +0800, Feng Tang wrote:
> > This originates from a patch in Meego IVI kernel with the name
> > 	linux-2.6.37-connext-0027-tsc2007.patch
> > There is no author info excepte a line "From  MeeGo
> > <kernel@meego.com>"
> > 
> > When integrating tsc2007 on Intel IVI platform, there are a lot of
> > noise data whose z1 value is around 10 which is not a sane "z1". So
> > add a "z1_low_threshhold" to filter those nosie data, to make the
> > device work properly.
> 
> Sounds like a task for userspace to ignore pressure that is too low.
> Bonus points for making it configurable so user can adjust
> sensitivity.

Actually, there is one more point :), without this patch, the driver won't
work on our platforms.

The tsc2007_soft_irq will keep reading the input data unless there is no
valid pressure data. In our case, those noise data will be seen as valid
data during tsc2007_calculate_pressure(), and the tsc2007_soft_irq will
run endlessly.

Thanks,
Feng 

> 
> Thanks.
> 

Re: [PATCH 2/2] Input: tsc2007 - Add a z1_low_threshhold platform data parameter

[Dmitry Torokhov]

On Wed, Nov 30, 2011 at 10:34:18AM +0800, Feng Tang wrote:
> On Tue, 29 Nov 2011 17:23:10 +0800
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > On Tue, Nov 29, 2011 at 04:12:58PM +0800, Feng Tang wrote:
> > > This originates from a patch in Meego IVI kernel with the name
> > > 	linux-2.6.37-connext-0027-tsc2007.patch
> > > There is no author info excepte a line "From  MeeGo
> > > <kernel@meego.com>"
> > > 
> > > When integrating tsc2007 on Intel IVI platform, there are a lot of
> > > noise data whose z1 value is around 10 which is not a sane "z1". So
> > > add a "z1_low_threshhold" to filter those nosie data, to make the
> > > device work properly.
> > 
> > Sounds like a task for userspace to ignore pressure that is too low.
> > Bonus points for making it configurable so user can adjust
> > sensitivity.
> 
> Actually, there is one more point :), without this patch, the driver won't
> work on our platforms.
> 
> The tsc2007_soft_irq will keep reading the input data unless there is no
> valid pressure data. In our case, those noise data will be seen as valid
> data during tsc2007_calculate_pressure(), and the tsc2007_soft_irq will
> run endlessly.

Even if we add the pressure threshold would not that noise cause endless
stream of interrupts?

Also, what kind of z2 is reported with low z1? And do you implement
get_pendown_state()?

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] Input: tsc2007 - Disable irq when the irq thread is handling data

[Dmitry Torokhov]

On Wed, Nov 30, 2011 at 10:08:24AM +0800, Feng Tang wrote:
> Hi Dmitry,
> 
> Thanks for the review.
> 
> On Tue, 29 Nov 2011 17:22:08 +0800
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > Hi Feng,
> > 
> > On Tue, Nov 29, 2011 at 04:12:57PM +0800, Feng Tang wrote:
> > > The TSC2007 data sheet say in some case the HW may fire some false
> > > interrrupt, which I also met during integrating one TSC2007 device.
> > > So add the disable_irq/enable_irq protection around data handling.
> > 
> > IRQF_ONESHOT should prevent IRQ from firing again while thread is
> > servicing it. Did you actually observe it not working?
> 
> You are right, the tsc's threaded IRQ function is not re-entered, and
> the driver is working actually. My bad not stating the problem clearly.
> The real problem I want solve is, many platforms including ours use a
> GPIO line as the tsc2007's IRQ line, and when these extra tsc2007 IRQ 
> is triggered on the gpio line, that GPIO controller will fire up extra
> noise IRQ accordingly, causing its ISR to be called. And my patch is
>  trying to let the GPIO controller driver disable that specific IRQ pin
> from tsc2007. As disable_irq will call GPIO irq_chip's irq_disable() or
> mask() hook.

But ONESHOT interrupt handler will not unmask interrupt until thead
finishes servicing it so we should not be seeing these extra IRQs.
I'm adding Thomas in case I misunderstand how it threaded IRQ supposed to
work.

Also, there is clear_penirq() platform method that is called to clean
penirq state, if needed.


> 
> By grep the tsc2007_platform_data in kernel, I see most of the most of
> the tsc2007 platforms are using GPIO line as tsc2007 IRQ ine. So this
> should be a general problem.
> 
> Following is patch with updated log and comments.
> 
> Thanks,
> Feng
> 
> -----------------
> 
> From 16585020d27a066d2908959e370ea4f8905a0d34 Mon Sep 17 00:00:00 2001
> From: Feng Tang <feng.tang@intel.com>
> Date: Tue, 29 Nov 2011 15:48:42 +0800
> Subject: [PATCH 1/2] Input: tsc2007 - Disable irq when the irq thread is handling data
> 
> The TSC2007 data sheet say in some case the HW may fire some false
> interrrupt, which I also met during integrating one TSC2007 device.
> Most of the tsc2007 platforms including ours are using a gpio line as
> tsc2007's irq line, so calling disable_irq_nosync() to actually
> prevent the gpio controller from firing IRQ for tsc2007 in such case.
> 
> Signed-off-by: Feng Tang <feng.tang@intel.com>
> ---
>  drivers/input/touchscreen/tsc2007.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
> index 1f674cb..03c1961 100644
> --- a/drivers/input/touchscreen/tsc2007.c
> +++ b/drivers/input/touchscreen/tsc2007.c
> @@ -171,6 +171,18 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
>  	struct ts_event tc;
>  	u32 rt;
>  
> +	/*
> +	 * Disable the irq, as it may fire several other IRQs during
> +	 * this thread is handling data, as suggested by the TSC2007
> +	 * datasheet, p19, "Touch Detect" chapter.
> +	 *
> +	 * Most of the tsc2007 platforms are using a gpio line as
> +	 * tsc2007's irq line, so calling disable_irq_nosync() will
> +	 * actually prevent the gpio controller from firing IRQ for
> +	 * this tsc2007 line in such case.
> +	 */
> +	disable_irq_nosync(irq);
> +
>  	while (!ts->stopped && tsc2007_is_pen_down(ts)) {
>  
>  		/* pen is down, continue with the measurement */
> @@ -221,6 +233,7 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
>  	if (ts->clear_penirq)
>  		ts->clear_penirq();
>  
> +	enable_irq(irq);
>  	return IRQ_HANDLED;
>  }
>  
> -- 
> 1.7.1
> > 
> > Thanks.
> > 
> > > 
> > > Signed-off-by: Feng Tang <feng.tang@intel.com>
> > > ---
> > >  drivers/input/touchscreen/tsc2007.c |    8 ++++++++
> > >  1 files changed, 8 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/input/touchscreen/tsc2007.c
> > > b/drivers/input/touchscreen/tsc2007.c index 1f674cb..789f216 100644
> > > --- a/drivers/input/touchscreen/tsc2007.c
> > > +++ b/drivers/input/touchscreen/tsc2007.c
> > > @@ -171,6 +171,13 @@ static irqreturn_t tsc2007_soft_irq(int irq,
> > > void *handle) struct ts_event tc;
> > >  	u32 rt;
> > >  
> > > +	/*
> > > +	 * Disable the irq, as it may fire several other IRQs
> > > during
> > > +	 * this thread is handling data, as suggested by the
> > > TSC2007
> > > +	 * datasheet, p19, "Touch Detect" chapter
> > > +	 */
> > > +	disable_irq_nosync(irq);
> > > +
> > >  	while (!ts->stopped && tsc2007_is_pen_down(ts)) {
> > >  
> > >  		/* pen is down, continue with the measurement */
> > > @@ -221,6 +228,7 @@ static irqreturn_t tsc2007_soft_irq(int irq,
> > > void *handle) if (ts->clear_penirq)
> > >  		ts->clear_penirq();
> > >  
> > > +	enable_irq(irq);
> > >  	return IRQ_HANDLED;
> > >  }
> > >  
> > > -- 
> > > 1.7.1
> > > 
> > > 
> > 

-- 
Dmitry

Re: [PATCH 2/2] Input: tsc2007 - Add a z1_low_threshhold platform data parameter

[Feng Tang]

On Thu, 1 Dec 2011 13:47:26 +0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Wed, Nov 30, 2011 at 10:34:18AM +0800, Feng Tang wrote:
> > On Tue, 29 Nov 2011 17:23:10 +0800
> > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > 
> > > On Tue, Nov 29, 2011 at 04:12:58PM +0800, Feng Tang wrote:
> > > > This originates from a patch in Meego IVI kernel with the name
> > > > 	linux-2.6.37-connext-0027-tsc2007.patch
> > > > There is no author info excepte a line "From  MeeGo
> > > > <kernel@meego.com>"
> > > > 
> > > > When integrating tsc2007 on Intel IVI platform, there are a lot
> > > > of noise data whose z1 value is around 10 which is not a sane
> > > > "z1". So add a "z1_low_threshhold" to filter those nosie data,
> > > > to make the device work properly.
> > > 
> > > Sounds like a task for userspace to ignore pressure that is too
> > > low. Bonus points for making it configurable so user can adjust
> > > sensitivity.
> > 
> > Actually, there is one more point :), without this patch, the
> > driver won't work on our platforms.
> > 
> > The tsc2007_soft_irq will keep reading the input data unless there
> > is no valid pressure data. In our case, those noise data will be
> > seen as valid data during tsc2007_calculate_pressure(), and the
> > tsc2007_soft_irq will run endlessly.
> 
> Even if we add the pressure threshold would not that noise cause
> endless stream of interrupts?

No, there is no endless interrupts for tsc2007. Without the z1
threshold, the while circle in tsc2007_soft_irq will run endlessly
as the noise data will be seen as a valid data:

		rt = tsc2007_calculate_pressure(ts, &tc);
		if (rt == 0 && !ts->get_pendown_state) {
			/*
			 * If pressure reported is 0 and we don't have
			 * callback to check pendown state, we have to
			 * assume that pen was lifted up.
			 */
			break;
		}

With the z1 threshold check, the rt will be 0 for noise data, and the
code flow broke out.

> 
> Also, what kind of z2 is reported with low z1? And do you implement
> get_pendown_state()?

z2 seems normal as some data between 3000-4000. We don't have a
get_pendown_state().

Thanks,
Feng
> 
> Thanks.
> 

Re: [PATCH 1/2] Input: tsc2007 - Disable irq when the irq thread is handling data

[Feng Tang]

Hi Dmitry,

On Thu, 1 Dec 2011 14:10:15 +0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Wed, Nov 30, 2011 at 10:08:24AM +0800, Feng Tang wrote:
> > Hi Dmitry,
> > 
> > Thanks for the review.
> > 
> > On Tue, 29 Nov 2011 17:22:08 +0800
> > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > 
> > > Hi Feng,
> > > 
> > > On Tue, Nov 29, 2011 at 04:12:57PM +0800, Feng Tang wrote:
> > > > The TSC2007 data sheet say in some case the HW may fire some
> > > > false interrrupt, which I also met during integrating one
> > > > TSC2007 device. So add the disable_irq/enable_irq protection
> > > > around data handling.
> > > 
> > > IRQF_ONESHOT should prevent IRQ from firing again while thread is
> > > servicing it. Did you actually observe it not working?
> > 
> > You are right, the tsc's threaded IRQ function is not re-entered,
> > and the driver is working actually. My bad not stating the problem
> > clearly. The real problem I want solve is, many platforms including
> > ours use a GPIO line as the tsc2007's IRQ line, and when these
> > extra tsc2007 IRQ is triggered on the gpio line, that GPIO
> > controller will fire up extra noise IRQ accordingly, causing its
> > ISR to be called. And my patch is trying to let the GPIO controller
> > driver disable that specific IRQ pin from tsc2007. As disable_irq
> > will call GPIO irq_chip's irq_disable() or mask() hook.
> 
> But ONESHOT interrupt handler will not unmask interrupt until thead
> finishes servicing it so we should not be seeing these extra IRQs.
> I'm adding Thomas in case I misunderstand how it threaded IRQ
> supposed to work.

I did see these extra IRQs. As the tsc2007 datasheet says, the PENIRQ may
be falsely triggered, and that signal is passed to the GPIO controller, if
the tsc2007 specific pin is not disabled in GPIOC level, then the GPIOC HW
will send out a IRQ anyway. 

While calling the disable_irq(), it will call the irq_chip's (implemented by
GPIO controller) irq_disable() or irq_mask() hook to disable that specific
line for tsc2007.

I did check the original tsc2007 driver, which used the disable_irq/enable_irq
too, which means this problem is a general one and has been seen before.

Or should we have a another flag in tsc2007 platform data, to let each platform
chose whether or not to use the disable/enable_irq according to their platform
need.

> 
> Also, there is clear_penirq() platform method that is called to clean
> penirq state, if needed.

Sadly we don't have a way to clear the irq from TSC2007 on our platform :(

Thanks,
Feng

> 
> 
> > 
> > By grep the tsc2007_platform_data in kernel, I see most of the most
> > of the tsc2007 platforms are using GPIO line as tsc2007 IRQ ine. So
> > this should be a general problem.
> > 
> > Following is patch with updated log and comments.
> > 
> > Thanks,
> > Feng
> > 
> > -----------------
> > 
> > From 16585020d27a066d2908959e370ea4f8905a0d34 Mon Sep 17 00:00:00
> > 2001 From: Feng Tang <feng.tang@intel.com>
> > Date: Tue, 29 Nov 2011 15:48:42 +0800
> > Subject: [PATCH 1/2] Input: tsc2007 - Disable irq when the irq
> > thread is handling data
> > 
> > The TSC2007 data sheet say in some case the HW may fire some false
> > interrrupt, which I also met during integrating one TSC2007 device.
> > Most of the tsc2007 platforms including ours are using a gpio line
> > as tsc2007's irq line, so calling disable_irq_nosync() to actually
> > prevent the gpio controller from firing IRQ for tsc2007 in such
> > case.
> > 
> > Signed-off-by: Feng Tang <feng.tang@intel.com>
> > ---
> >  drivers/input/touchscreen/tsc2007.c |   13 +++++++++++++
> >  1 files changed, 13 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/input/touchscreen/tsc2007.c
> > b/drivers/input/touchscreen/tsc2007.c index 1f674cb..03c1961 100644
> > --- a/drivers/input/touchscreen/tsc2007.c
> > +++ b/drivers/input/touchscreen/tsc2007.c
> > @@ -171,6 +171,18 @@ static irqreturn_t tsc2007_soft_irq(int irq,
> > void *handle) struct ts_event tc;
> >  	u32 rt;
> >  
> > +	/*
> > +	 * Disable the irq, as it may fire several other IRQs
> > during
> > +	 * this thread is handling data, as suggested by the
> > TSC2007
> > +	 * datasheet, p19, "Touch Detect" chapter.
> > +	 *
> > +	 * Most of the tsc2007 platforms are using a gpio line as
> > +	 * tsc2007's irq line, so calling disable_irq_nosync() will
> > +	 * actually prevent the gpio controller from firing IRQ for
> > +	 * this tsc2007 line in such case.
> > +	 */
> > +	disable_irq_nosync(irq);
> > +
> >  	while (!ts->stopped && tsc2007_is_pen_down(ts)) {
> >  
> >  		/* pen is down, continue with the measurement */
> > @@ -221,6 +233,7 @@ static irqreturn_t tsc2007_soft_irq(int irq,
> > void *handle) if (ts->clear_penirq)
> >  		ts->clear_penirq();
> >  
> > +	enable_irq(irq);
> >  	return IRQ_HANDLED;
> >  }
> >  
> > -- 
> > 1.7.1

Re: [PATCH 2/2] Input: tsc2007 - Add a z1_low_threshhold platform data parameter

[Dmitry Torokhov]

On Thu, Dec 01, 2011 at 02:19:48PM +0800, Feng Tang wrote:
> On Thu, 1 Dec 2011 13:47:26 +0800
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > On Wed, Nov 30, 2011 at 10:34:18AM +0800, Feng Tang wrote:
> > > On Tue, 29 Nov 2011 17:23:10 +0800
> > > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > 
> > > > On Tue, Nov 29, 2011 at 04:12:58PM +0800, Feng Tang wrote:
> > > > > This originates from a patch in Meego IVI kernel with the name
> > > > > 	linux-2.6.37-connext-0027-tsc2007.patch
> > > > > There is no author info excepte a line "From  MeeGo
> > > > > <kernel@meego.com>"
> > > > > 
> > > > > When integrating tsc2007 on Intel IVI platform, there are a lot
> > > > > of noise data whose z1 value is around 10 which is not a sane
> > > > > "z1". So add a "z1_low_threshhold" to filter those nosie data,
> > > > > to make the device work properly.
> > > > 
> > > > Sounds like a task for userspace to ignore pressure that is too
> > > > low. Bonus points for making it configurable so user can adjust
> > > > sensitivity.
> > > 
> > > Actually, there is one more point :), without this patch, the
> > > driver won't work on our platforms.
> > > 
> > > The tsc2007_soft_irq will keep reading the input data unless there
> > > is no valid pressure data. In our case, those noise data will be
> > > seen as valid data during tsc2007_calculate_pressure(), and the
> > > tsc2007_soft_irq will run endlessly.
> > 
> > Even if we add the pressure threshold would not that noise cause
> > endless stream of interrupts?
> 
> No, there is no endless interrupts for tsc2007. Without the z1
> threshold, the while circle in tsc2007_soft_irq will run endlessly
> as the noise data will be seen as a valid data:
> 
> 		rt = tsc2007_calculate_pressure(ts, &tc);
> 		if (rt == 0 && !ts->get_pendown_state) {
> 			/*
> 			 * If pressure reported is 0 and we don't have
> 			 * callback to check pendown state, we have to
> 			 * assume that pen was lifted up.
> 			 */
> 			break;
> 		}
> 
> With the z1 threshold check, the rt will be 0 for noise data, and the
> code flow broke out.

What I meant is with the threshold check we'll break out of the ISR but
why won't IRQ be raised again?

> 
> > 
> > Also, what kind of z2 is reported with low z1? And do you implement
> > get_pendown_state()?
> 
> z2 seems normal as some data between 3000-4000. We don't have a
> get_pendown_state().

OK, there is max_rt platform parameter. I think we should employ it
instead and break out if we get several incorrect samples in a row. Too
bad you do not have a dedicate method.

-- 
Dmitry

Re: [PATCH 1/2] Input: tsc2007 - Disable irq when the irq thread is handling data

[Dmitry Torokhov]

On Thu, Dec 01, 2011 at 02:30:12PM +0800, Feng Tang wrote:
> Hi Dmitry,
> 
> On Thu, 1 Dec 2011 14:10:15 +0800
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > On Wed, Nov 30, 2011 at 10:08:24AM +0800, Feng Tang wrote:
> > > Hi Dmitry,
> > > 
> > > Thanks for the review.
> > > 
> > > On Tue, 29 Nov 2011 17:22:08 +0800
> > > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > 
> > > > Hi Feng,
> > > > 
> > > > On Tue, Nov 29, 2011 at 04:12:57PM +0800, Feng Tang wrote:
> > > > > The TSC2007 data sheet say in some case the HW may fire some
> > > > > false interrrupt, which I also met during integrating one
> > > > > TSC2007 device. So add the disable_irq/enable_irq protection
> > > > > around data handling.
> > > > 
> > > > IRQF_ONESHOT should prevent IRQ from firing again while thread is
> > > > servicing it. Did you actually observe it not working?
> > > 
> > > You are right, the tsc's threaded IRQ function is not re-entered,
> > > and the driver is working actually. My bad not stating the problem
> > > clearly. The real problem I want solve is, many platforms including
> > > ours use a GPIO line as the tsc2007's IRQ line, and when these
> > > extra tsc2007 IRQ is triggered on the gpio line, that GPIO
> > > controller will fire up extra noise IRQ accordingly, causing its
> > > ISR to be called. And my patch is trying to let the GPIO controller
> > > driver disable that specific IRQ pin from tsc2007. As disable_irq
> > > will call GPIO irq_chip's irq_disable() or mask() hook.
> > 
> > But ONESHOT interrupt handler will not unmask interrupt until thead
> > finishes servicing it so we should not be seeing these extra IRQs.
> > I'm adding Thomas in case I misunderstand how it threaded IRQ
> > supposed to work.
> 
> I did see these extra IRQs. As the tsc2007 datasheet says, the PENIRQ may
> be falsely triggered, and that signal is passed to the GPIO controller, if
> the tsc2007 specific pin is not disabled in GPIOC level, then the GPIOC HW
> will send out a IRQ anyway. 
> 
> While calling the disable_irq(), it will call the irq_chip's (implemented by
> GPIO controller) irq_disable() or irq_mask() hook to disable that specific
> line for tsc2007.
> 
> I did check the original tsc2007 driver, which used the disable_irq/enable_irq
> too, which means this problem is a general one and has been seen before.
> 

No, the original had disable/enable IRQ because it was the only way to
stop interrupt storm with combination of hard IRQ + workqueue or thread.

BTW, do you have it configured as level or edge interrupt?

> Or should we have a another flag in tsc2007 platform data, to let each platform
> chose whether or not to use the disable/enable_irq according to their platform
> need.
> 
> > 
> > Also, there is clear_penirq() platform method that is called to clean
> > penirq state, if needed.
> 
> Sadly we don't have a way to clear the irq from TSC2007 on our platform :(
>

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] Input: tsc2007 - Disable irq when the irq thread is handling data

[Feng Tang]

Hi Dmitry

On Thu, 1 Dec 2011 16:48:18 +0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Thu, Dec 01, 2011 at 02:30:12PM +0800, Feng Tang wrote:
> > Hi Dmitry,
> > 
> > On Thu, 1 Dec 2011 14:10:15 +0800
> > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > 
> > > On Wed, Nov 30, 2011 at 10:08:24AM +0800, Feng Tang wrote:
> > > > Hi Dmitry,
> > > > 
> > > > Thanks for the review.
> > > > 
> > > > On Tue, 29 Nov 2011 17:22:08 +0800
> > > > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > > 
> > > > > Hi Feng,
> > > > > 
> > > > > On Tue, Nov 29, 2011 at 04:12:57PM +0800, Feng Tang wrote:
> > > > > > The TSC2007 data sheet say in some case the HW may fire some
> > > > > > false interrrupt, which I also met during integrating one
> > > > > > TSC2007 device. So add the disable_irq/enable_irq protection
> > > > > > around data handling.
> > > > > 
> > > > > IRQF_ONESHOT should prevent IRQ from firing again while
> > > > > thread is servicing it. Did you actually observe it not
> > > > > working?
> > > > 
> > > > You are right, the tsc's threaded IRQ function is not
> > > > re-entered, and the driver is working actually. My bad not
> > > > stating the problem clearly. The real problem I want solve is,
> > > > many platforms including ours use a GPIO line as the tsc2007's
> > > > IRQ line, and when these extra tsc2007 IRQ is triggered on the
> > > > gpio line, that GPIO controller will fire up extra noise IRQ
> > > > accordingly, causing its ISR to be called. And my patch is
> > > > trying to let the GPIO controller driver disable that specific
> > > > IRQ pin from tsc2007. As disable_irq will call GPIO irq_chip's
> > > > irq_disable() or mask() hook.
> > > 
> > > But ONESHOT interrupt handler will not unmask interrupt until
> > > thead finishes servicing it so we should not be seeing these
> > > extra IRQs. I'm adding Thomas in case I misunderstand how it
> > > threaded IRQ supposed to work.
> > 
> > I did see these extra IRQs. As the tsc2007 datasheet says, the
> > PENIRQ may be falsely triggered, and that signal is passed to the
> > GPIO controller, if the tsc2007 specific pin is not disabled in
> > GPIOC level, then the GPIOC HW will send out a IRQ anyway. 
> > 
> > While calling the disable_irq(), it will call the irq_chip's
> > (implemented by GPIO controller) irq_disable() or irq_mask() hook
> > to disable that specific line for tsc2007.
> > 
> > I did check the original tsc2007 driver, which used the
> > disable_irq/enable_irq too, which means this problem is a general
> > one and has been seen before.
> > 
> 
> No, the original had disable/enable IRQ because it was the only way to
> stop interrupt storm with combination of hard IRQ + workqueue or
> thread.

My understanding is, if the GPIO line used by tsc2007 is not disabled
in GPIO controller level, the GPIOC will always be triggered by tsc2007
to fire those extra interrupts.

Actually I did try the old version driver (no threaded irq version), which
will see the extra interrupts if the disable_irq/enable_irq is removed.

> 
> BTW, do you have it configured as level or edge interrupt?

I'm now using the falling edge trigger, but I tried the level trigger
which will have more interrupts storm.

Thanks,
Feng

> 
> > Or should we have a another flag in tsc2007 platform data, to let
> > each platform chose whether or not to use the disable/enable_irq
> > according to their platform need.
> > 
> > > 
> > > Also, there is clear_penirq() platform method that is called to
> > > clean penirq state, if needed.
> > 
> > Sadly we don't have a way to clear the irq from TSC2007 on our
> > platform :(
> >
> 
> Thanks.
> 

Re: [PATCH 2/2] Input: tsc2007 - Add a z1_low_threshhold platform data parameter

[Feng Tang]

Hi Dmitry,


On Thu, 1 Dec 2011 16:45:24 +0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:


> > > 
> > > Even if we add the pressure threshold would not that noise cause
> > > endless stream of interrupts?
> > 
> > No, there is no endless interrupts for tsc2007. Without the z1
> > threshold, the while circle in tsc2007_soft_irq will run endlessly
> > as the noise data will be seen as a valid data:
> > 
> > 		rt = tsc2007_calculate_pressure(ts, &tc);
> > 		if (rt == 0 && !ts->get_pendown_state) {
> > 			/*
> > 			 * If pressure reported is 0 and we don't
> > have
> > 			 * callback to check pendown state, we have
> > to
> > 			 * assume that pen was lifted up.
> > 			 */
> > 			break;
> > 		}
> > 
> > With the z1 threshold check, the rt will be 0 for noise data, and
> > the code flow broke out.
> 
> What I meant is with the threshold check we'll break out of the ISR
> but why won't IRQ be raised again?

The IRQ will be fired again after we exist the tsc2007_soft_irq in my
test, as I re-enable the irq before existing the code.

> 
> > 
> > > 
> > > Also, what kind of z2 is reported with low z1? And do you
> > > implement get_pendown_state()?
> > 
> > z2 seems normal as some data between 3000-4000. We don't have a
> > get_pendown_state().
> 
> OK, there is max_rt platform parameter. I think we should employ it
> instead and break out if we get several incorrect samples in a row.
> Too bad you do not have a dedicate method.
> 
No, the max_rt won't help here, if the rt > max_rt, it won't break the
while loop, but just issue a warning message

		if (rt <= ts->max_rt) {
			.......

		} else {
			/*
			 * Sample found inconsistent by debouncing or pressure is
			 * beyond the maximum. Don't report it to user space,
			 * repeat at least once more the measurement.
			 */
			dev_dbg(&ts->client->dev, "ignored pressure %d\n", rt);
		}

Thanks,
Feng

Re: [PATCH 2/2] Input: tsc2007 - Add a z1_low_threshhold platform data parameter

[Dmitry Torokhov]

On Thu, Dec 01, 2011 at 05:04:55PM +0800, Feng Tang wrote:
> Hi Dmitry,
> 
> 
> On Thu, 1 Dec 2011 16:45:24 +0800
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> 
> > > > 
> > > > Even if we add the pressure threshold would not that noise cause
> > > > endless stream of interrupts?
> > > 
> > > No, there is no endless interrupts for tsc2007. Without the z1
> > > threshold, the while circle in tsc2007_soft_irq will run endlessly
> > > as the noise data will be seen as a valid data:
> > > 
> > > 		rt = tsc2007_calculate_pressure(ts, &tc);
> > > 		if (rt == 0 && !ts->get_pendown_state) {
> > > 			/*
> > > 			 * If pressure reported is 0 and we don't
> > > have
> > > 			 * callback to check pendown state, we have
> > > to
> > > 			 * assume that pen was lifted up.
> > > 			 */
> > > 			break;
> > > 		}
> > > 
> > > With the z1 threshold check, the rt will be 0 for noise data, and
> > > the code flow broke out.
> > 
> > What I meant is with the threshold check we'll break out of the ISR
> > but why won't IRQ be raised again?
> 
> The IRQ will be fired again after we exist the tsc2007_soft_irq in my
> test, as I re-enable the irq before existing the code.
> 
> > 
> > > 
> > > > 
> > > > Also, what kind of z2 is reported with low z1? And do you
> > > > implement get_pendown_state()?
> > > 
> > > z2 seems normal as some data between 3000-4000. We don't have a
> > > get_pendown_state().
> > 
> > OK, there is max_rt platform parameter. I think we should employ it
> > instead and break out if we get several incorrect samples in a row.
> > Too bad you do not have a dedicate method.
> > 
> No, the max_rt won't help here, if the rt > max_rt, it won't break the
> while loop, but just issue a warning message
> 
> 		if (rt <= ts->max_rt) {
> 			.......
> 
> 		} else {
> 			/*
> 			 * Sample found inconsistent by debouncing or pressure is
> 			 * beyond the maximum. Don't report it to user space,
> 			 * repeat at least once more the measurement.
> 			 */
> 			dev_dbg(&ts->client->dev, "ignored pressure %d\n", rt);
> 		}

What I meant that we need to ajust the logic to _exit_ the loop if we
receive several samples with rt > max_rt instead of adding a new
parameter.

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] Input: tsc2007 - Add a z1_low_threshhold platform data parameter

[Feng Tang]

Hi Dmitry,

On Sun, 4 Dec 2011 16:54:05 +0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Thu, Dec 01, 2011 at 05:04:55PM +0800, Feng Tang wrote:
> > Hi Dmitry,
> > 
> > 
> > On Thu, 1 Dec 2011 16:45:24 +0800
> > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > 
> > 
> > > > > 
> > > > > Even if we add the pressure threshold would not that noise
> > > > > cause endless stream of interrupts?
> > > > 
> > > > No, there is no endless interrupts for tsc2007. Without the z1
> > > > threshold, the while circle in tsc2007_soft_irq will run
> > > > endlessly as the noise data will be seen as a valid data:
> > > > 
> > > > 		rt = tsc2007_calculate_pressure(ts, &tc);
> > > > 		if (rt == 0 && !ts->get_pendown_state) {
> > > > 			/*
> > > > 			 * If pressure reported is 0 and we
> > > > don't have
> > > > 			 * callback to check pendown state, we
> > > > have to
> > > > 			 * assume that pen was lifted up.
> > > > 			 */
> > > > 			break;
> > > > 		}
> > > > 
> > > > With the z1 threshold check, the rt will be 0 for noise data,
> > > > and the code flow broke out.
> > > 
> > > What I meant is with the threshold check we'll break out of the
> > > ISR but why won't IRQ be raised again?
> > 
> > The IRQ will be fired again after we exist the tsc2007_soft_irq in
> > my test, as I re-enable the irq before existing the code.
> > 
> > > 
> > > > 
> > > > > 
> > > > > Also, what kind of z2 is reported with low z1? And do you
> > > > > implement get_pendown_state()?
> > > > 
> > > > z2 seems normal as some data between 3000-4000. We don't have a
> > > > get_pendown_state().
> > > 
> > > OK, there is max_rt platform parameter. I think we should employ
> > > it instead and break out if we get several incorrect samples in a
> > > row. Too bad you do not have a dedicate method.
> > > 
> > No, the max_rt won't help here, if the rt > max_rt, it won't break
> > the while loop, but just issue a warning message
> > 
> > 		if (rt <= ts->max_rt) {
> > 			.......
> > 
> > 		} else {
> > 			/*
> > 			 * Sample found inconsistent by debouncing
> > or pressure is
> > 			 * beyond the maximum. Don't report it to
> > user space,
> > 			 * repeat at least once more the
> > measurement. */
> > 			dev_dbg(&ts->client->dev, "ignored pressure
> > %d\n", rt); }
> 
> What I meant that we need to ajust the logic to _exit_ the loop if we
> receive several samples with rt > max_rt instead of adding a new
> parameter.

Yes, I did try a similar way to set a retry limit which also works
basically, code is like this

@@ -169,6 +169,7 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
        struct tsc2007 *ts = handle;
        struct input_dev *input = ts->input;
        struct ts_event tc;
+       u32 max_retry = 2;
        u32 rt;
 
        while (!ts->stopped && tsc2007_is_pen_down(ts)) {
@@ -206,6 +207,8 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
                         * repeat at least once more the measurement.
                         */
                        dev_dbg(&ts->client->dev, "ignored pressure %d\n", rt);
+                       if (!--max_retry)
+                               break;
                }


But I still have some concerns:
1. How many retries should we try? the tsc2007_read_values() will take about 70 ms
on our platform, plus the "poll_period", one retry will take about 100ms.
2. I checked the noise data, its z1 value is always in a range from 9 to 13, while
the real data's z1 is always bigger than 300. So I think there is a very clear gap
to tell the noise data from valid data by z1 value.

How do you think about it?

Thanks,
Feng

> 
> Thanks.
> 

Re: [PATCH 2/2] Input: tsc2007 - Add a z1_low_threshhold platform data parameter

[Feng Tang]

Hi Dmitry,

On Mon, 5 Dec 2011 13:35:41 +0800
Feng Tang <feng.tang@intel.com> wrote:

> > > 			/*
> > > 			 * Sample found inconsistent by debouncing
> > > or pressure is
> > > 			 * beyond the maximum. Don't report it to
> > > user space,
> > > 			 * repeat at least once more the
> > > measurement. */
> > > 			dev_dbg(&ts->client->dev, "ignored pressure
> > > %d\n", rt); }
> > 
> > What I meant that we need to ajust the logic to _exit_ the loop if we
> > receive several samples with rt > max_rt instead of adding a new
> > parameter.
> 
> Yes, I did try a similar way to set a retry limit which also works
> basically, code is like this
> 
> @@ -169,6 +169,7 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
>         struct tsc2007 *ts = handle;
>         struct input_dev *input = ts->input;
>         struct ts_event tc;
> +       u32 max_retry = 2;
>         u32 rt;
>  
>         while (!ts->stopped && tsc2007_is_pen_down(ts)) {
> @@ -206,6 +207,8 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
>                          * repeat at least once more the measurement.
>                          */
>                         dev_dbg(&ts->client->dev, "ignored pressure %d\n",
> rt);
> +                       if (!--max_retry)
> +                               break;
>                 }
> 
> 
> But I still have some concerns:
> 1. How many retries should we try? the tsc2007_read_values() will take about
> 70 ms on our platform, plus the "poll_period", one retry will take about
> 100ms. 2. I checked the noise data, its z1 value is always in a range from 9
> to 13, while the real data's z1 is always bigger than 300. So I think there
> is a very clear gap to tell the noise data from valid data by z1 value.
> 
> How do you think about it?

Any further comments?

Thanks,
Feng