| Message ID | 1343470061-16879-5-git-send-email-zonque@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Jul 28, 2012 at 12:07 PM, Daniel Mack <zonque@gmail.com> wrote: > Simplify the code in gpio-pxa.c and make them based on irq_base. > When not probed from devicetree, initialize irq_base from > PXA_GPIO_TO_IRQ() or MMP_GPIO_TO_IRQ(), respectively, so the non-DT case > still works. > > Only tested on PXA3xx. > > Signed-off-by: Daniel Mack <zonque@gmail.com> > Acked-by: Arnd Bergmann <arnd@arndb.de> OK: Acked-by: Linus Walleij <linus.walleij@linaro.org> But: > +#ifdef CONFIG_ARCH_PXA > + if (gpio_is_pxa_type(gpio_type)) > + irq_base = PXA_GPIO_TO_IRQ(0); > +#endif > +#ifdef CONFIG_ARCH_MMP > + if (gpio_is_mmp_type(gpio_type)) > + irq_base = MMP_GPIO_TO_IRQ(0); > +#endif Grrr. Can we think of a way to get rid of these #ifdef:s? Yours, Linus Walleij
On Sun, Aug 5, 2012 at 8:12 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Sat, Jul 28, 2012 at 12:07 PM, Daniel Mack <zonque@gmail.com> wrote: > >> Simplify the code in gpio-pxa.c and make them based on irq_base. >> When not probed from devicetree, initialize irq_base from >> PXA_GPIO_TO_IRQ() or MMP_GPIO_TO_IRQ(), respectively, so the non-DT case >> still works. >> >> +#ifdef CONFIG_ARCH_PXA >> + if (gpio_is_pxa_type(gpio_type)) >> + irq_base = PXA_GPIO_TO_IRQ(0); >> +#endif >> +#ifdef CONFIG_ARCH_MMP >> + if (gpio_is_mmp_type(gpio_type)) >> + irq_base = MMP_GPIO_TO_IRQ(0); >> +#endif > > Grrr. Can we think of a way to get rid of these #ifdef:s? > It's used for non-DT mode. This driver is used in both arch-pxa and arch-mmp. Since we use static irq allocation in non-DT mode, we have to use this kind of code to get irq_base. What Daniel did is only simplifying the code. Regards Haojian
On Sun, Aug 5, 2012 at 4:56 AM, Haojian Zhuang <haojian.zhuang@gmail.com> wrote: > On Sun, Aug 5, 2012 at 8:12 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >> On Sat, Jul 28, 2012 at 12:07 PM, Daniel Mack <zonque@gmail.com> wrote: >> >>> Simplify the code in gpio-pxa.c and make them based on irq_base. >>> When not probed from devicetree, initialize irq_base from >>> PXA_GPIO_TO_IRQ() or MMP_GPIO_TO_IRQ(), respectively, so the non-DT case >>> still works. >>> >>> +#ifdef CONFIG_ARCH_PXA >>> + if (gpio_is_pxa_type(gpio_type)) >>> + irq_base = PXA_GPIO_TO_IRQ(0); >>> +#endif >>> +#ifdef CONFIG_ARCH_MMP >>> + if (gpio_is_mmp_type(gpio_type)) >>> + irq_base = MMP_GPIO_TO_IRQ(0); >>> +#endif >> >> Grrr. Can we think of a way to get rid of these #ifdef:s? >> > > It's used for non-DT mode. This driver is used in both arch-pxa > and arch-mmp. Since we use static irq allocation in non-DT mode, > we have to use this kind of code to get irq_base. > > What Daniel did is only simplifying the code. Yep I know, and the result is better than without the patch, so I Acked it. But if I have my dreams come true we could do away with this compile-time stuff as well... Yours, Linus Walleij
On Sunday 05 August 2012, Linus Walleij wrote: > >>> > >>> +#ifdef CONFIG_ARCH_PXA > >>> + if (gpio_is_pxa_type(gpio_type)) > >>> + irq_base = PXA_GPIO_TO_IRQ(0); > >>> +#endif > >>> +#ifdef CONFIG_ARCH_MMP > >>> + if (gpio_is_mmp_type(gpio_type)) > >>> + irq_base = MMP_GPIO_TO_IRQ(0); > >>> +#endif > >> > >> Grrr. Can we think of a way to get rid of these #ifdef:s? > >> > > > > It's used for non-DT mode. This driver is used in both arch-pxa > > and arch-mmp. Since we use static irq allocation in non-DT mode, > > we have to use this kind of code to get irq_base. > > > > What Daniel did is only simplifying the code. > > Yep I know, and the result is better than without the patch, so > I Acked it. But if I have my dreams come true we could do away > with this compile-time stuff as well... I think the real solution for these is to merge ARCH_PXA and ARCH_MMP and move all the code into one directory. I think it's ok to have the above hack in there in the meantime as long as we can agree on where we're heading eventually. Arnd
On 06.08.2012 10:09, Arnd Bergmann wrote: > On Sunday 05 August 2012, Linus Walleij wrote: >>>>> >>>>> +#ifdef CONFIG_ARCH_PXA >>>>> + if (gpio_is_pxa_type(gpio_type)) >>>>> + irq_base = PXA_GPIO_TO_IRQ(0); >>>>> +#endif >>>>> +#ifdef CONFIG_ARCH_MMP >>>>> + if (gpio_is_mmp_type(gpio_type)) >>>>> + irq_base = MMP_GPIO_TO_IRQ(0); >>>>> +#endif >>>> >>>> Grrr. Can we think of a way to get rid of these #ifdef:s? >>>> >>> >>> It's used for non-DT mode. This driver is used in both arch-pxa >>> and arch-mmp. Since we use static irq allocation in non-DT mode, >>> we have to use this kind of code to get irq_base. >>> >>> What Daniel did is only simplifying the code. >> >> Yep I know, and the result is better than without the patch, so >> I Acked it. But if I have my dreams come true we could do away >> with this compile-time stuff as well... > > I think the real solution for these is to merge ARCH_PXA and ARCH_MMP > and move all the code into one directory. I think it's ok to have > the above hack in there in the meantime as long as we can agree on where > we're heading eventually. I don't know how much work that would be but I agree that it would definitiely be nicer, yes. Note, however, that there was a lot more #ifdef hackery in that driver before the cleanup. Daniel
diff --git a/drivers/gpio/gpio-pxa.c b/drivers/gpio/gpio-pxa.c index 58a6a63..6d0cb9d 100644 --- a/drivers/gpio/gpio-pxa.c +++ b/drivers/gpio/gpio-pxa.c @@ -59,6 +59,7 @@ #define BANK_OFF(n) (((n) < 3) ? (n) << 2 : 0x100 + (((n) - 3) << 2)) int pxa_last_gpio; +static int irq_base; #ifdef CONFIG_OF static struct irq_domain *domain; @@ -166,63 +167,14 @@ static inline int __gpio_is_occupied(unsigned gpio) return ret; } -#ifdef CONFIG_ARCH_PXA -static inline int __pxa_gpio_to_irq(int gpio) -{ - if (gpio_is_pxa_type(gpio_type)) - return PXA_GPIO_TO_IRQ(gpio); - return -1; -} - -static inline int __pxa_irq_to_gpio(int irq) -{ - if (gpio_is_pxa_type(gpio_type)) - return irq - PXA_GPIO_TO_IRQ(0); - return -1; -} -#else -static inline int __pxa_gpio_to_irq(int gpio) { return -1; } -static inline int __pxa_irq_to_gpio(int irq) { return -1; } -#endif - -#ifdef CONFIG_ARCH_MMP -static inline int __mmp_gpio_to_irq(int gpio) -{ - if (gpio_is_mmp_type(gpio_type)) - return MMP_GPIO_TO_IRQ(gpio); - return -1; -} - -static inline int __mmp_irq_to_gpio(int irq) -{ - if (gpio_is_mmp_type(gpio_type)) - return irq - MMP_GPIO_TO_IRQ(0); - return -1; -} -#else -static inline int __mmp_gpio_to_irq(int gpio) { return -1; } -static inline int __mmp_irq_to_gpio(int irq) { return -1; } -#endif - static int pxa_gpio_to_irq(struct gpio_chip *chip, unsigned offset) { - int gpio, ret; - - gpio = chip->base + offset; - ret = __pxa_gpio_to_irq(gpio); - if (ret >= 0) - return ret; - return __mmp_gpio_to_irq(gpio); + return chip->base + offset + irq_base; } int pxa_irq_to_gpio(int irq) { - int ret; - - ret = __pxa_irq_to_gpio(irq); - if (ret >= 0) - return ret; - return __mmp_irq_to_gpio(irq); + return irq - irq_base; } static int pxa_gpio_direction_input(struct gpio_chip *chip, unsigned offset) @@ -510,7 +462,7 @@ const struct irq_domain_ops pxa_irq_domain_ops = { #ifdef CONFIG_OF static int __devinit pxa_gpio_probe_dt(struct platform_device *pdev) { - int ret, nr_banks, nr_gpios, irq_base; + int ret, nr_banks, nr_gpios; struct device_node *prev, *next, *np = pdev->dev.of_node; const struct of_device_id *of_id = of_match_device(pxa_gpio_dt_ids, &pdev->dev); @@ -564,10 +516,20 @@ static int __devinit pxa_gpio_probe(struct platform_device *pdev) int irq0 = 0, irq1 = 0, irq_mux, gpio_offset = 0; ret = pxa_gpio_probe_dt(pdev); - if (ret < 0) + if (ret < 0) { pxa_last_gpio = pxa_gpio_nums(); - else +#ifdef CONFIG_ARCH_PXA + if (gpio_is_pxa_type(gpio_type)) + irq_base = PXA_GPIO_TO_IRQ(0); +#endif +#ifdef CONFIG_ARCH_MMP + if (gpio_is_mmp_type(gpio_type)) + irq_base = MMP_GPIO_TO_IRQ(0); +#endif + } else { use_of = 1; + } + if (!pxa_last_gpio) return -EINVAL;