Hello,

On Fri, Nov 22, 2013 at 9:45 PM, Russell King - ARM Linux
[off-list ref] wrote:

On Fri, Nov 15, 2013 at 12:47:58PM +0400, Dmitry Eremin-Solenikov wrote:

quoted

mach-sa1100's irq.c contains a mixture of system and GPIO irqs handling.
Split out GPIO irqchip to gpio-sa1100.c. To decouple first 11 GPIO IRQs
handling, make IRQ0-IRQ10 use chained irq handler that just passes the
IRQ to GPIO IRQs.

Also during this refactoring introduce irq domain support for sa1100
gpio irqs.

I'm not sure I quite like this change... How much testing has this had?
I'd much prefer that this code just isn't touched because of all the
problems we've had here in years back with IRQs from CF cards being
dropped and the like.  This was very hard to get right, especially when
interacting with the SA1111.

How can we just check that? And what was the problem with CF IRQs?
Sorry, if I'm asking dumb questions, I really missed that part of the history.

I can go the way Linus Walleij originally did:
1) switch to irq domain / hwirq
2) integrate static variables to containers

And only after that in a separate patch(set)

3) Move GPIO IRQ handling to gpio-sa1100.

What do you think?

quoted

@@ -136,3 +301,58 @@ static int __init sa1100_gpio_init(void)
      return platform_driver_register(&sa1100_gpio_driver);
 }
 postcore_initcall(sa1100_gpio_init);
+
+#ifdef CONFIG_PM
+static unsigned long saved_gplr;
+static unsigned long saved_gpdr;
+static unsigned long saved_grer;
+static unsigned long saved_gfer;
+
+static int sa1100_gpio_suspend(void)
+{
+     struct sa1100_gpio_chip *sgc = chip;
+     saved_gplr = readl_relaxed(sgc->regbase + GPLR_OFFSET);
+     saved_gpdr = readl_relaxed(sgc->regbase + GPDR_OFFSET);
+     saved_grer = readl_relaxed(sgc->regbase + GRER_OFFSET);
+     saved_gfer = readl_relaxed(sgc->regbase + GFER_OFFSET);
+
+     /* Clear GPIO transition detect bits */
+     writel_relaxed(0xffffffff, sgc->regbase + GEDR_OFFSET);
+
+     /* FIXME: Original code also reprogramed GRER/GFER here,
+      * I don't see the purpose though.
+     GRER = PWER & sgc->gpio_rising;
+     GFER = PWER & sgc->gpio_falling;
+      */

So you thought you'd just comment it out because you don't understand...
Not really the way things are done.  If you don't understand something,
you shouldn't be touching the code.

Thus I have the big FIXME.

In this case, it's quite simple.  GRER and GFER need to be programmed
with the interrupts which we want to be active for _each_ mode of the
system.

I see. I will put this back in the next iteration, but see below please.

Therefore, if we want to have certain GPIOs triggering wakeups (iow,
those GPIOs which have had enable_irq_wake() called on them) but not
those which haven't, we need to reprogram GRER and GFER with the
GPIOs which can wake the system up.

I thought so. I was puzzled, because that would mean, that we want to wake
up from a GPIO, which is masked in GPIO_IRQ_mask, but enabled in PWER.
Is that the real scenario/usecase? If we/kernel has disabled IRQ
through _mask_irq,
I thought, we didn't expect events from that pin (including wakeup), did we?


-- 
With best wishes
Dmitry