| Message ID | 1281187711-10215-1-git-send-email-agust@denx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi Anatolij, Looks pretty good, but some comments below... On Sat, Aug 7, 2010 at 7:28 AM, Anatolij Gustschin <agust@denx.de> wrote: > Signed-off-by: Anatolij Gustschin <agust@denx.de> You haven't written a patch description. Give some details about how the 512x gpio controller is different from the 8xxx one. > --- > arch/powerpc/platforms/Kconfig | 7 ++-- > arch/powerpc/sysdev/mpc8xxx_gpio.c | 54 +++++++++++++++++++++++++++++++++++- > 2 files changed, 57 insertions(+), 4 deletions(-) > > diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig > index d1663db..471115a 100644 > --- a/arch/powerpc/platforms/Kconfig > +++ b/arch/powerpc/platforms/Kconfig > @@ -304,13 +304,14 @@ config OF_RTC > source "arch/powerpc/sysdev/bestcomm/Kconfig" > > config MPC8xxx_GPIO > - bool "MPC8xxx GPIO support" > - depends on PPC_MPC831x || PPC_MPC834x || PPC_MPC837x || FSL_SOC_BOOKE || PPC_86xx > + bool "MPC512x/MPC8xxx GPIO support" > + depends on PPC_MPC512x || PPC_MPC831x || PPC_MPC834x || PPC_MPC837x || \ > + FSL_SOC_BOOKE || PPC_86xx > select GENERIC_GPIO > select ARCH_REQUIRE_GPIOLIB > help > Say Y here if you're going to use hardware that connects to the > - MPC831x/834x/837x/8572/8610 GPIOs. > + MPC512x/831x/834x/837x/8572/8610 GPIOs. > > config SIMPLE_GPIO > bool "Support for simple, memory-mapped GPIO controllers" > diff --git a/arch/powerpc/sysdev/mpc8xxx_gpio.c b/arch/powerpc/sysdev/mpc8xxx_gpio.c > index 2b69084..f5b4959 100644 > --- a/arch/powerpc/sysdev/mpc8xxx_gpio.c > +++ b/arch/powerpc/sysdev/mpc8xxx_gpio.c > @@ -1,5 +1,5 @@ > /* > - * GPIOs on MPC8349/8572/8610 and compatible > + * GPIOs on MPC512x/8349/8572/8610 and compatible > * > * Copyright (C) 2008 Peter Korsgaard <jacmet@sunsite.dk> > * > @@ -26,6 +26,7 @@ > #define GPIO_IER 0x0c > #define GPIO_IMR 0x10 > #define GPIO_ICR 0x14 > +#define GPIO_ICR2 0x18 > > struct mpc8xxx_gpio_chip { > struct of_mm_gpio_chip mm_gc; > @@ -215,6 +216,51 @@ static int mpc8xxx_irq_set_type(unsigned int virq, unsigned int flow_type) > return 0; > } > > +static int mpc512x_irq_set_type(unsigned int virq, unsigned int flow_type) > +{ > + struct mpc8xxx_gpio_chip *mpc8xxx_gc = get_irq_chip_data(virq); > + struct of_mm_gpio_chip *mm = &mpc8xxx_gc->mm_gc; > + unsigned long gpio = virq_to_hw(virq); > + void __iomem *reg; > + unsigned int shift; > + unsigned long flags; > + > + if (gpio < 16) { > + reg = mm->regs + GPIO_ICR; > + shift = (15 - gpio) * 2; > + } else { > + reg = mm->regs + GPIO_ICR2; > + shift = (15 - (gpio % 16)) * 2; > + } > + > + switch (flow_type) { > + case IRQ_TYPE_EDGE_FALLING: > + case IRQ_TYPE_LEVEL_LOW: > + spin_lock_irqsave(&mpc8xxx_gc->lock, flags); > + clrsetbits_be32(reg, 3 << shift, 2 << shift); > + spin_unlock_irqrestore(&mpc8xxx_gc->lock, flags); > + break; > + > + case IRQ_TYPE_EDGE_RISING: > + case IRQ_TYPE_LEVEL_HIGH: > + spin_lock_irqsave(&mpc8xxx_gc->lock, flags); > + clrsetbits_be32(reg, 3 << shift, 1 << shift); > + spin_unlock_irqrestore(&mpc8xxx_gc->lock, flags); > + break; > + > + case IRQ_TYPE_EDGE_BOTH: > + spin_lock_irqsave(&mpc8xxx_gc->lock, flags); > + clrbits32(reg, 3 << shift); > + spin_unlock_irqrestore(&mpc8xxx_gc->lock, flags); > + break; > + > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > static struct irq_chip mpc8xxx_irq_chip = { > .name = "mpc8xxx-gpio", > .unmask = mpc8xxx_irq_unmask, > @@ -226,6 +272,9 @@ static struct irq_chip mpc8xxx_irq_chip = { > static int mpc8xxx_gpio_irq_map(struct irq_host *h, unsigned int virq, > irq_hw_number_t hw) > { > + if (of_device_is_compatible(h->of_node, "fsl,mpc5121-gpio")) > + mpc8xxx_irq_chip.set_type = mpc512x_irq_set_type; > + You can put the set type hook into the of_match_table data which you will need for of_find_matching_node() (see below). > set_irq_chip_data(virq, h->host_data); > set_irq_chip_and_handler(virq, &mpc8xxx_irq_chip, handle_level_irq); > set_irq_type(virq, IRQ_TYPE_NONE); > @@ -330,6 +379,9 @@ static int __init mpc8xxx_add_gpiochips(void) > for_each_compatible_node(np, NULL, "fsl,mpc8610-gpio") > mpc8xxx_add_controller(np); > > + for_each_compatible_node(np, NULL, "fsl,mpc5121-gpio") > + mpc8xxx_add_controller(np); > + This list is getting too long. Refactor this function to use for_each_matching_node(). Also doing it this way is dangerous because if say a 5121 gpio node claims compatibility with a 8610 gpio node, then the gpio controller will get registered twice. > return 0; > } > arch_initcall(mpc8xxx_add_gpiochips); > -- > 1.7.0.4 > >
Hi Grant, On Sat, 7 Aug 2010 09:12:50 -0600 Grant Likely <grant.likely@secretlab.ca> wrote: > Hi Anatolij, > > Looks pretty good, but some comments below... > > On Sat, Aug 7, 2010 at 7:28 AM, Anatolij Gustschin <agust@denx.de> wrote: > > Signed-off-by: Anatolij Gustschin <agust@denx.de> > > You haven't written a patch description. Give some details about how > the 512x gpio controller is different from the 8xxx one. Ok, will fix. ... > > @@ -226,6 +272,9 @@ static struct irq_chip mpc8xxx_irq_chip = { > > static int mpc8xxx_gpio_irq_map(struct irq_host *h, unsigned int virq, > > irq_hw_number_t hw) > > { > > + if (of_device_is_compatible(h->of_node, "fsl,mpc5121-gpio")) > > + mpc8xxx_irq_chip.set_type = mpc512x_irq_set_type; > > + > > You can put the set type hook into the of_match_table data which you > will need for of_find_matching_node() (see below). How can I get this match table data reference in mpc8xxx_gpio_irq_map() ? Is it okay to set data field of struct device_node to the set type hook? I could do it in mpc8xxx_add_gpiochips() but I'm not sure whether the data field will be used for other purposes somewhere else. ... > > @@ -330,6 +379,9 @@ static int __init mpc8xxx_add_gpiochips(void) > > for_each_compatible_node(np, NULL, "fsl,mpc8610-gpio") > > mpc8xxx_add_controller(np); > > > > + for_each_compatible_node(np, NULL, "fsl,mpc5121-gpio") > > + mpc8xxx_add_controller(np); > > + > > This list is getting too long. Refactor this function to use > for_each_matching_node(). Also doing it this way is dangerous because > if say a 5121 gpio node claims compatibility with a 8610 gpio node, > then the gpio controller will get registered twice. Fixed. Thanks, Anatolij
On Sat, Aug 7, 2010 at 10:39 AM, Anatolij Gustschin <agust@denx.de> wrote: > Grant Likely <grant.likely@secretlab.ca> wrote: >> > @@ -226,6 +272,9 @@ static struct irq_chip mpc8xxx_irq_chip = { >> > static int mpc8xxx_gpio_irq_map(struct irq_host *h, unsigned int virq, >> > irq_hw_number_t hw) >> > { >> > + if (of_device_is_compatible(h->of_node, "fsl,mpc5121-gpio")) >> > + mpc8xxx_irq_chip.set_type = mpc512x_irq_set_type; >> > + >> >> You can put the set type hook into the of_match_table data which you >> will need for of_find_matching_node() (see below). > > How can I get this match table data reference in mpc8xxx_gpio_irq_map() ? of_match_node() will return the matching entry in the table. > Is it okay to set data field of struct device_node to the set type > hook? I could do it in mpc8xxx_add_gpiochips() but I'm not sure whether > the data field will be used for other purposes somewhere else. You are safe to use the .data field. g.
diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig index d1663db..471115a 100644 --- a/arch/powerpc/platforms/Kconfig +++ b/arch/powerpc/platforms/Kconfig @@ -304,13 +304,14 @@ config OF_RTC source "arch/powerpc/sysdev/bestcomm/Kconfig" config MPC8xxx_GPIO - bool "MPC8xxx GPIO support" - depends on PPC_MPC831x || PPC_MPC834x || PPC_MPC837x || FSL_SOC_BOOKE || PPC_86xx + bool "MPC512x/MPC8xxx GPIO support" + depends on PPC_MPC512x || PPC_MPC831x || PPC_MPC834x || PPC_MPC837x || \ + FSL_SOC_BOOKE || PPC_86xx select GENERIC_GPIO select ARCH_REQUIRE_GPIOLIB help Say Y here if you're going to use hardware that connects to the - MPC831x/834x/837x/8572/8610 GPIOs. + MPC512x/831x/834x/837x/8572/8610 GPIOs. config SIMPLE_GPIO bool "Support for simple, memory-mapped GPIO controllers" diff --git a/arch/powerpc/sysdev/mpc8xxx_gpio.c b/arch/powerpc/sysdev/mpc8xxx_gpio.c index 2b69084..f5b4959 100644 --- a/arch/powerpc/sysdev/mpc8xxx_gpio.c +++ b/arch/powerpc/sysdev/mpc8xxx_gpio.c @@ -1,5 +1,5 @@ /* - * GPIOs on MPC8349/8572/8610 and compatible + * GPIOs on MPC512x/8349/8572/8610 and compatible * * Copyright (C) 2008 Peter Korsgaard <jacmet@sunsite.dk> * @@ -26,6 +26,7 @@ #define GPIO_IER 0x0c #define GPIO_IMR 0x10 #define GPIO_ICR 0x14 +#define GPIO_ICR2 0x18 struct mpc8xxx_gpio_chip { struct of_mm_gpio_chip mm_gc; @@ -215,6 +216,51 @@ static int mpc8xxx_irq_set_type(unsigned int virq, unsigned int flow_type) return 0; } +static int mpc512x_irq_set_type(unsigned int virq, unsigned int flow_type) +{ + struct mpc8xxx_gpio_chip *mpc8xxx_gc = get_irq_chip_data(virq); + struct of_mm_gpio_chip *mm = &mpc8xxx_gc->mm_gc; + unsigned long gpio = virq_to_hw(virq); + void __iomem *reg; + unsigned int shift; + unsigned long flags; + + if (gpio < 16) { + reg = mm->regs + GPIO_ICR; + shift = (15 - gpio) * 2; + } else { + reg = mm->regs + GPIO_ICR2; + shift = (15 - (gpio % 16)) * 2; + } + + switch (flow_type) { + case IRQ_TYPE_EDGE_FALLING: + case IRQ_TYPE_LEVEL_LOW: + spin_lock_irqsave(&mpc8xxx_gc->lock, flags); + clrsetbits_be32(reg, 3 << shift, 2 << shift); + spin_unlock_irqrestore(&mpc8xxx_gc->lock, flags); + break; + + case IRQ_TYPE_EDGE_RISING: + case IRQ_TYPE_LEVEL_HIGH: + spin_lock_irqsave(&mpc8xxx_gc->lock, flags); + clrsetbits_be32(reg, 3 << shift, 1 << shift); + spin_unlock_irqrestore(&mpc8xxx_gc->lock, flags); + break; + + case IRQ_TYPE_EDGE_BOTH: + spin_lock_irqsave(&mpc8xxx_gc->lock, flags); + clrbits32(reg, 3 << shift); + spin_unlock_irqrestore(&mpc8xxx_gc->lock, flags); + break; + + default: + return -EINVAL; + } + + return 0; +} + static struct irq_chip mpc8xxx_irq_chip = { .name = "mpc8xxx-gpio", .unmask = mpc8xxx_irq_unmask, @@ -226,6 +272,9 @@ static struct irq_chip mpc8xxx_irq_chip = { static int mpc8xxx_gpio_irq_map(struct irq_host *h, unsigned int virq, irq_hw_number_t hw) { + if (of_device_is_compatible(h->of_node, "fsl,mpc5121-gpio")) + mpc8xxx_irq_chip.set_type = mpc512x_irq_set_type; + set_irq_chip_data(virq, h->host_data); set_irq_chip_and_handler(virq, &mpc8xxx_irq_chip, handle_level_irq); set_irq_type(virq, IRQ_TYPE_NONE); @@ -330,6 +379,9 @@ static int __init mpc8xxx_add_gpiochips(void) for_each_compatible_node(np, NULL, "fsl,mpc8610-gpio") mpc8xxx_add_controller(np); + for_each_compatible_node(np, NULL, "fsl,mpc5121-gpio") + mpc8xxx_add_controller(np); + return 0; } arch_initcall(mpc8xxx_add_gpiochips);
Signed-off-by: Anatolij Gustschin <agust@denx.de> --- arch/powerpc/platforms/Kconfig | 7 ++-- arch/powerpc/sysdev/mpc8xxx_gpio.c | 54 +++++++++++++++++++++++++++++++++++- 2 files changed, 57 insertions(+), 4 deletions(-)