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

[Feng Tang]

Hi Dmitry,

On Thu, 1 Dec 2011 14:10:15 +0800
Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
> > 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@xxxxxxxxx>
> > ---
> >  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/