[PATCH] MXC: set GPIO IRQ handler

[PATCH] MXC: set GPIO IRQ handler

[John Ogness]

The irq chip function gpio_set_irq_type() correctly sets the i.MX
registers but does not set the irq handler. This means that all
gpio-based irq's are handled with handle_edge_irq().

This patch corrects this by also setting the appropriate handler.

This patch is against 2.6.32-rc8.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 arch/arm/plat-mxc/gpio.c |    5 +++++
 1 file changed, 5 insertions(+)
--- a/arch/arm/plat-mxc/gpio.c
+++ b/arch/arm/plat-mxc/gpio.c
@@ -126,6 +126,11 @@ static int gpio_set_irq_type(u32 irq, u3
 	__raw_writel(val | (edge << (bit << 1)), reg);
 	_clear_gpio_irqstatus(port, gpio & 0x1f);
 
+	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
+		__set_irq_handler_unlocked(irq, handle_level_irq);
+	else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
+		__set_irq_handler_unlocked(irq, handle_edge_irq);
+
 	return 0;
 }
 

[PATCH] MXC: set GPIO IRQ handler

[Uwe Kleine-König]

Hello John,

On Wed, Nov 25, 2009 at 07:19:58PM +0100, John Ogness wrote:
> The irq chip function gpio_set_irq_type() correctly sets the i.MX
> registers but does not set the irq handler.
I assume you see some breakage without your patch?  In mainline?

>                                             This means that all
> gpio-based irq's are handled with handle_edge_irq().
This is not true in mainline, until 060d20d (imx/gpio: Use
handle_level_irq) (currently in imx/mxc-master) hits Linus' tree ...

> This patch corrects this by also setting the appropriate handler.
> 
> This patch is against 2.6.32-rc8.
... so there is no hurry.

Can you please test with 060d20d if your breakage still occurs and
if it is still valid report some details (for me and the commit log)?

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-K?nig            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

[PATCH] MXC: set GPIO IRQ handler

[Thomas Gleixner]

On Thu, 26 Nov 2009, Uwe Kleine-K?nig wrote:
> On Wed, Nov 25, 2009 at 07:19:58PM +0100, John Ogness wrote:
> > The irq chip function gpio_set_irq_type() correctly sets the i.MX
> > registers but does not set the irq handler.
>
> I assume you see some breakage without your patch?  In mainline?

Is there some other sensible reason why someone would send such a
patch with a completely clear change log ?

> 
>                                             This means that all
> > gpio-based irq's are handled with handle_edge_irq().

> This is not true in mainline, ...

We probably look at a different mainline, right ?

int __init mxc_gpio_init(struct mxc_gpio_port *port, int cnt)
{
....
         set_irq_chip(j, &gpio_irq_chip);
         set_irq_handler(j, handle_edge_irq);

> .... until 060d20d (imx/gpio: Use
> handle_level_irq) (currently in imx/mxc-master) hits Linus' tree ...

The patch fixes an existing problem in mainline. It does not matter
whether there is a different fix pending in some git tree which is
supposed to hit mainline at some undefined point in the future.

> > This patch corrects this by also setting the appropriate handler.
> > 
> > This patch is against 2.6.32-rc8.
> ... so there is no hurry.

Interesting conclusion: every level triggered GPIO interrupt in
2.6.32-rc8 is affected by this problem. Definitely nothing to worry
about ....

> Can you please test with 060d20d if your breakage still occurs and
> if it is still valid report some details (for me and the commit log)?

Could you please provide useful details, i.e. a short explanation why
that commit is the superior fix, instead of forcing people to clone a
git tree to figure out what you are (not) talking about ?

That's the change log of 060d20d:

    imx/gpio: Use handle_level_irq
    
    According to Russell King handle_edge_irq is only useful for "edge-based
    inputs where the controller does not remember transitions with the input
    masked."
    
    So using handle_edge_irq unconditionally for both edge and level irqs is
    wrong.  Testing showed that the controller does remember transitions
    while the interrupt is masked.  So use handle_level_irq unconditionally.

And the changelog is utter crap.

Using handle_edge_irq for level triggered interrupts has nothing to do
with Russell's observation simply because using handle_edge_irq for
level triggered interrupts is patently wrong.

The fact that the irq controller of the imx happens to be designed by
people who seem to have understood the pitfalls of edge triggered irqs
allows to use handle_level_irq for both edge and level triggered. 

That does not make John's patch incorrect. Using handle_level_irq for
both is merily an optimization which would be even more understandable
if there would be an useful comment in the code.

Thanks,

	tglx

[PATCH] MXC: set GPIO IRQ handler

[Uwe Kleine-König]

Hi Thomas,

On Sat, Nov 28, 2009 at 12:46:18AM +0100, Thomas Gleixner wrote:
> On Thu, 26 Nov 2009, Uwe Kleine-K?nig wrote:
> > On Wed, Nov 25, 2009 at 07:19:58PM +0100, John Ogness wrote:
> > > The irq chip function gpio_set_irq_type() correctly sets the i.MX
> > > registers but does not set the irq handler.
> >
> > I assume you see some breakage without your patch?  In mainline?
> 
> Is there some other sensible reason why someone would send such a
> patch with a completely clear change log ?
IMHO John's commit could point out where he saw breakage.  This applies
to mine, too, of course.

> > 
> >                                             This means that all
> > > gpio-based irq's are handled with handle_edge_irq().
> 
> > This is not true in mainline, ...
> 
> We probably look at a different mainline, right ?
> 
> int __init mxc_gpio_init(struct mxc_gpio_port *port, int cnt)
> {
> ....
>          set_irq_chip(j, &gpio_irq_chip);
>          set_irq_handler(j, handle_edge_irq);

Oh, I was sure that John wrote "... all gpio-based irq's are handled
with handle_level_irq()."  Sorry for the confusion.
 
> > .... until 060d20d (imx/gpio: Use
> > handle_level_irq) (currently in imx/mxc-master) hits Linus' tree ...

> 
> The patch fixes an existing problem in mainline. It does not matter
> whether there is a different fix pending in some git tree which is
> supposed to hit mainline at some undefined point in the future.
That undefined point in future is the next merge window and the commit
is already contained in rmk's devel branch.

> > > This patch corrects this by also setting the appropriate handler.
> > > 
> > > This patch is against 2.6.32-rc8.
> > ... so there is no hurry.
> 
> Interesting conclusion: every level triggered GPIO interrupt in
> 2.6.32-rc8 is affected by this problem. Definitely nothing to worry
> about ....
When I sent the patch that became 060d20d to Sascha Hauer I considered
this a fix that should go in before 2.6.32, too.  Sascha decided it to
be too risky to take it outside of the merge window as he didn't knew of
any breakage in mainline code.  That's why I asked where John saw
breakage and if my patch helps.  And yes, if (and only if) the breakage
happens only for a platform or a driver that is not in mainline I don't
see a reason to take the patch before 2.6.32.  Agreed?

> > Can you please test with 060d20d if your breakage still occurs and
> > if it is still valid report some details (for me and the commit log)?
> 
> Could you please provide useful details, i.e. a short explanation why
> that commit is the superior fix, instead of forcing people to clone a
> git tree to figure out what you are (not) talking about ?
As I misread John's commit log I thought that he already has looked at
the imx tree.  (Which seems natural when working with that platform.)
I just noticed that the tree isn't listed in MAINTAINERS, I will send a
patch for that.

Having the commit log of 060d20d I don't consider it too hard to work
out why my commit should fix John's problem, too.

> That's the change log of 060d20d:
> 
>     imx/gpio: Use handle_level_irq
>     
>     According to Russell King handle_edge_irq is only useful for "edge-based
>     inputs where the controller does not remember transitions with the input
>     masked."
>     
>     So using handle_edge_irq unconditionally for both edge and level irqs is
>     wrong.  Testing showed that the controller does remember transitions
>     while the interrupt is masked.  So use handle_level_irq unconditionally.
> 
> And the changelog is utter crap.
> 
> Using handle_edge_irq for level triggered interrupts has nothing to do
> with Russell's observation simply because using handle_edge_irq for
> level triggered interrupts is patently wrong.
> 
> The fact that the irq controller of the imx happens to be designed by
> people who seem to have understood the pitfalls of edge triggered irqs
> allows to use handle_level_irq for both edge and level triggered. 
> 
> That does not make John's patch incorrect. Using handle_level_irq for
> both is merily an optimization which would be even more understandable
> if there would be an useful comment in the code.
I didn't say that John's patch is wrong.  And instead of documenting
that handle_level_irq (which is named a bit misleading) is capable of
handling edge irqs on imx, too, what about creating a handler with a
better name (say handle_generic_irq?---open for better suggestions) and
making handle_level_irq a deprecated wrapper for it?

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-K?nig            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

[PATCH] MXC: set GPIO IRQ handler

[Thomas Gleixner]

Uwe,

On Sun, 29 Nov 2009, Uwe Kleine-K?nig wrote:
> > > Can you please test with 060d20d if your breakage still occurs and
> > > if it is still valid report some details (for me and the commit log)?
> > 
> > Could you please provide useful details, i.e. a short explanation why
> > that commit is the superior fix, instead of forcing people to clone a
> > git tree to figure out what you are (not) talking about ?
> As I misread John's commit log I thought that he already has looked at
> the imx tree.  (Which seems natural when working with that platform.)
> I just noticed that the tree isn't listed in MAINTAINERS, I will send a
> patch for that.
> 
> Having the commit log of 060d20d I don't consider it too hard to work
> out why my commit should fix John's problem, too.

Do you actually read, what people write ?

1) I asked you to add that information into your reply and not request
   people to clone a git tree to figure out what you are talking about

2) I said your changelog is crap. And again:

> >     imx/gpio: Use handle_level_irq
> >     
> >     According to Russell King handle_edge_irq is only useful for "edge-based
> >     inputs where the controller does not remember transitions with the input
> >     masked."
> >
> >     So using handle_edge_irq unconditionally for both edge and level irqs is
> >     wrong. 

It's not wrong because Russell explained when to use handle_edge_irq.

It's fundamentally wrong to use handle_edge_irq for level type
interrupts. And that's the bug in the current code.

The natural adhoc fix for this is what John posted with an
understandable explanation.

Now the better solution is to use handle_level_irq for both types
because the interrupt controller is well designed.

> > That does not make John's patch incorrect. Using handle_level_irq for
> > both is merily an optimization which would be even more understandable
> > if there would be an useful comment in the code.
> I didn't say that John's patch is wrong.  And instead of documenting
> that handle_level_irq (which is named a bit misleading) is capable of
> handling edge irqs on imx, too, what about creating a handler with a
> better name (say handle_generic_irq?---open for better suggestions) and
> making handle_level_irq a deprecated wrapper for it?

Come on, such a name change is pretty useless as it does not explain
why you can use it for both edge and level on imx. The reason why you
can do so is that the irq controller is not discarding edge
transitions when the interrupt is masked.

Thanks,

	tglx

[PATCH] MAINTAINERS: add tree and file pattern for ARM IMX

[Uwe Kleine-König]

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
---
 MAINTAINERS |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index c824b4d..f908bd8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -637,6 +637,9 @@ ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
 M:	Sascha Hauer <kernel@pengutronix.de>
 L:	linux-arm-kernel at lists.infradead.org (moderated for non-subscribers)
 S:	Maintained
+T:	git://git.pengutronix.de/git/imx/linux-2.6.git
+F:	arch/arm/mach-mx*/
+F:	arch/arm/plat-mxc
 
 ARM/GLOMATION GESBC9312SX MACHINE SUPPORT
 M:	Lennert Buytenhek <kernel@wantstofly.org>
-- 
1.6.5.2

[PATCH v2] MAINTAINERS: add tree and file pattern for ARM IMX

[Uwe Kleine-König]

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
---
Hello,

in the first patch I forgot a trailing slash for arch/arm/plat-mxc/.

Best regards
Uwe

 MAINTAINERS |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index c824b4d..d40cf36 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -637,6 +637,9 @@ ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
 M:	Sascha Hauer <kernel@pengutronix.de>
 L:	linux-arm-kernel at lists.infradead.org (moderated for non-subscribers)
 S:	Maintained
+T:	git://git.pengutronix.de/git/imx/linux-2.6.git
+F:	arch/arm/mach-mx*/
+F:	arch/arm/plat-mxc/
 
 ARM/GLOMATION GESBC9312SX MACHINE SUPPORT
 M:	Lennert Buytenhek <kernel@wantstofly.org>
-- 
1.6.5.2

[PATCH] MXC: set GPIO IRQ handler

[Uwe Kleine-König]

Hello Thomas,

On Mon, Nov 30, 2009 at 10:32:50AM +0100, Thomas Gleixner wrote:
> On Sun, 29 Nov 2009, Uwe Kleine-K?nig wrote:
> > > > Can you please test with 060d20d if your breakage still occurs and
> > > > if it is still valid report some details (for me and the commit log)?
> > > 
> > > Could you please provide useful details, i.e. a short explanation why
> > > that commit is the superior fix, instead of forcing people to clone a
> > > git tree to figure out what you are (not) talking about ?
> > As I misread John's commit log I thought that he already has looked at
> > the imx tree.  (Which seems natural when working with that platform.)
> > I just noticed that the tree isn't listed in MAINTAINERS, I will send a
> > patch for that.
> > 
> > Having the commit log of 060d20d I don't consider it too hard to work
> > out why my commit should fix John's problem, too.
> 
> Do you actually read, what people write ?
> 
> 1) I asked you to add that information into your reply and not request
>    people to clone a git tree to figure out what you are talking about
After you having quoted my commit log, I didn't feel that repeating it
adds much to the discussion.
 
> 2) I said your changelog is crap. And again:
> 
> > >     imx/gpio: Use handle_level_irq
> > >     
> > >     According to Russell King handle_edge_irq is only useful for "edge-based
> > >     inputs where the controller does not remember transitions with the input
> > >     masked."
> > >
> > >     So using handle_edge_irq unconditionally for both edge and level irqs is
> > >     wrong. 
> 
> It's not wrong because Russell explained when to use handle_edge_irq.
> 
> It's fundamentally wrong to use handle_edge_irq for level type
> interrupts. And that's the bug in the current code.
For my (admittedly German) feeling for the English language the
grammatical construct in the first and second paragraph (after the
subject) is OK.  Pointing out that using handle_edge_irq for level
triggered interrupts which have bottom-halves simply doesn't work would
have been nice, that's right.

I suggest you compose a better commit log and work out with Sascha if
and when he takes the improved commit.

> The natural adhoc fix for this is what John posted with an
> understandable explanation.
> 
> Now the better solution is to use handle_level_irq for both types
> because the interrupt controller is well designed.
Ack.  This is at least the second time you point that out.  I wonder
what exactly makes you think I don't agree.
 
> > > That does not make John's patch incorrect. Using handle_level_irq for
> > > both is merily an optimization which would be even more understandable
> > > if there would be an useful comment in the code.
> > I didn't say that John's patch is wrong.  And instead of documenting
> > that handle_level_irq (which is named a bit misleading) is capable of
> > handling edge irqs on imx, too, what about creating a handler with a
> > better name (say handle_generic_irq?---open for better suggestions) and
> > making handle_level_irq a deprecated wrapper for it?
> 
> Come on, such a name change is pretty useless as it does not explain
> why you can use it for both edge and level on imx. 
It was only a suggestion.  I was sure you will say it if you don't
consider it a good idea :-)

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-K?nig            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |