Patchwork powerpc/mpc8xxx_gpio.c: extend the driver to support mpc512x gpios

login
register
mail settings
Submitter Anatolij Gustschin
Date Aug. 7, 2010, 1:28 p.m.
Message ID <1281187711-10215-1-git-send-email-agust@denx.de>
Download mbox | patch
Permalink /patch/61179/
State Superseded
Headers show

Comments

Anatolij Gustschin - Aug. 7, 2010, 1:28 p.m.
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(-)
Grant Likely - Aug. 7, 2010, 3:12 p.m.
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
>
>
Anatolij Gustschin - Aug. 7, 2010, 4:39 p.m.
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
Grant Likely - Aug. 7, 2010, 4:58 p.m.
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.

Patch

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);