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

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

quoted

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.

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.

quoted

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