| Message ID | 1343230539-7196-1-git-send-email-zonque@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday 25 July 2012, Daniel Mack 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> Both look good to me, Acked-by: Arnd Bergmann <arnd@arndb.de>
On 25.07.2012 18:17, Arnd Bergmann wrote: > On Wednesday 25 July 2012, Daniel Mack 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> > > Both look good to me, > > Acked-by: Arnd Bergmann <arnd@arndb.de> > Thanks. I somehow lost track about the upstream pathes recently. Who would queue them up now? Daniel
On Wednesday 25 July 2012, Daniel Mack wrote: > On 25.07.2012 18:17, Arnd Bergmann wrote: > > On Wednesday 25 July 2012, Daniel Mack 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> > > > > Both look good to me, > > > > Acked-by: Arnd Bergmann <arnd@arndb.de> > > > > Thanks. I somehow lost track about the upstream pathes recently. Who > would queue them up now? Haojian Zhuang is usually the person who picks up PXA patches these days. If he prefers, you can also prepare a series and send them directly to arm@kernel.org (that's Olof and me, for all practical purposes) for integration into the arm-soc tree. I have not received many pxa patches recently, so if he has no other stuff then this might be the more convenient way. Arnd
On Wed, Jul 25, 2012 at 9:10 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday 25 July 2012, Daniel Mack wrote: >> >> Thanks. I somehow lost track about the upstream pathes recently. Who >> would queue them up now? > > Haojian Zhuang is usually the person who picks up PXA patches these days. > If he prefers, you can also prepare a series and send them directly to > arm@kernel.org (that's Olof and me, for all practical purposes) for > integration into the arm-soc tree. Since these two are in drivers/gpio I can take them through the GPIO tree if desired, but still need some PXA maintainer to ACK them, however they haven't really listed themselves as maintainers for the GPIO driver in MAINTAINERS which would be preferable. Yours, Linus Walleij
Hi Daniel, On 07/25/12 18:35, Daniel Mack 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> > --- > drivers/gpio/gpio-pxa.c | 70 +++++++++++------------------------------------ > 1 file changed, 16 insertions(+), 54 deletions(-) > > 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 [...] > @@ -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 The ifdes above do not look essential. Can't we drop them and just have else if ... else if? > + } else { > use_of = 1; > + } > + > if (!pxa_last_gpio) > return -EINVAL; >
On 05.08.2012 10:31, Igor Grinberg wrote: > Hi Daniel, > > On 07/25/12 18:35, Daniel Mack 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> >> --- >> drivers/gpio/gpio-pxa.c | 70 +++++++++++------------------------------------ >> 1 file changed, 16 insertions(+), 54 deletions(-) >> >> 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 > > [...] > >> @@ -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 > > The ifdes above do not look essential. But they are. In case of !CONFIG_ARCH_MMP, MMP_GPIO_TO_IRQ is undefined. Same problem for !CONFIG_ARC_PXA. > Can't we drop them and just have else if ... else if? We can't do that either, as we might have a hybrid kernel that works on both PXA and MMP platforms, and then the condition is a runtime thing. Daniel
On 08/05/12 14:56, Daniel Mack wrote: > On 05.08.2012 10:31, Igor Grinberg wrote: >> Hi Daniel, >> >> On 07/25/12 18:35, Daniel Mack 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> >>> --- >>> drivers/gpio/gpio-pxa.c | 70 +++++++++++------------------------------------ >>> 1 file changed, 16 insertions(+), 54 deletions(-) >>> >>> 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 >> >> [...] >> >>> @@ -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 >> >> The ifdes above do not look essential. > > But they are. In case of !CONFIG_ARCH_MMP, MMP_GPIO_TO_IRQ is undefined. > Same problem for !CONFIG_ARC_PXA. I see. Ok then. I'm not a huge fan of having the #ifdes inside the code (functions) and thinking of moving those outside of the function, I get pretty much the same solution it was before your patch. I really think that PXA|MMP_GPIO_TO_IRQ should be moved to some common location (say arch/arm/plat-pxa/) so both are available regardless of CONFIG_ARCH_MMP|PXA. That will simplify the code even more. But probably it is too much to ask for that simple patch... So, I'm fine with the patch. Thanks > >> Can't we drop them and just have else if ... else if? > > We can't do that either, as we might have a hybrid kernel that works on > both PXA and MMP platforms, and then the condition is a runtime thing. Well, that is exactly what I mean... I mean, the if ... else ... if ... else without the #ifdefs... But again, it requires some more changes.
On Monday 06 August 2012, Igor Grinberg wrote: > I see. Ok then. I'm not a huge fan of having the #ifdes inside > the code (functions) and thinking of moving those outside of the function, > I get pretty much the same solution it was before your patch. > > I really think that PXA|MMP_GPIO_TO_IRQ should be moved to some common > location (say arch/arm/plat-pxa/) so both are available regardless > of CONFIG_ARCH_MMP|PXA. That will simplify the code even more. > But probably it is too much to ask for that simple patch... > So, I'm fine with the patch. Right. I would also not try to spend too much work on making pxa and mmp coexist better when the plan is to merge them eventually. At that point, a couple of these hacks can just be removed. Arnd
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;
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> --- drivers/gpio/gpio-pxa.c | 70 +++++++++++------------------------------------ 1 file changed, 16 insertions(+), 54 deletions(-)