Re: [PATCH v3 1/6] arm: davinci: Fix low level gpio irq handlers' argument

Ido Yariv
2011-07-12T22:19:23Z

Hi Sekhar,

On Tue, Jul 12, 2011 at 02:52:17PM +0530, Nori, Sekhar wrote:
> > - g = (__force struct davinci_gpio_regs __iomem *) irq_desc_get_handler_data(desc);
> > + ctl = (struct davinci_gpio_controller *)irq_desc_get_handler_data(desc);
> > + g = (struct davinci_gpio_regs __iomem *)ctl->regs;
> >
> > /* we only care about one bank */
> > if (irq & 1)
> > @@ -278,7 +280,7 @@ gpio_irq_handler(unsigned irq, struct irq_desc *desc)
> > status >>= 16;
> >
> > /* now demux them to the right lowlevel handler */
> > - n = (int)irq_get_handler_data(irq);
> > + n = ctl->irq_base;
>
> I realized that this breaks for odd banks as the status is
> right shifted by 16. The GPIO you are using must have been
> in even bank?

You're absolutely right, I missed that. And yes, I have been using an
even bank GPIO.

> > while (status) {
> > res = ffs(status);
> > n += res;
> > @@ -424,7 +426,13 @@ static int __init davinci_gpio_irq_setup(void)
> >
> > /* set up all irqs in this bank */
> > irq_set_chained_handler(bank_irq, gpio_irq_handler);
> > - irq_set_handler_data(bank_irq, (__force void *)g);
> > +
> > + /*
> > + * Each chip handles 32 gpios, and each irq bank consists of 16
> > + * gpio irqs. Pass the irq bank's corresponding controller to
> > + * the chained irq handler.
> > + */
> > + irq_set_handler_data(bank_irq, &chips[bank * 16 / 32]);
>
> This can simply be:
>
> irq_set_handler_data(bank_irq, &chips[gpio / 32]);
>
> In the interest of time, I did these fixes and pushed the
> patch to "fixes" branch of git://gitorious.org/linux-davinci/linux-davinci.git
>
> Can you please test it out and let me know if it works.

This patch seems to work just fine. I'm afraid I can't test an odd bank
GPIO here to verify that this indeed fixed the issue you raised, but it
looks correct.

Thanks,
Ido.