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

Hi Dmitry,

On Thu, 1 Dec 2011 14:10:15 +0800
Dmitry Torokhov [off-list ref] wrote:

On Wed, Nov 30, 2011 at 10:08:24AM +0800, Feng Tang wrote:

quoted

Hi Dmitry,

Thanks for the review.

On Tue, 29 Nov 2011 17:22:08 +0800
Dmitry Torokhov [off-list ref] wrote:

quoted

Hi Feng,

On Tue, Nov 29, 2011 at 04:12:57PM +0800, Feng Tang wrote:

quoted

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

quoted

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 [off-list ref]
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 <redacted>
---
 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