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

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

Comments

Anatolij Gustschin - Aug. 7, 2010, 7:03 p.m.
The GPIO controller of MPC512x is slightly different from
8xxx GPIO controllers. The register interface is the same
except the external interrupt control register. The MPC512x
GPIO controller differentiates between four interrupt event
types and therefore provides two interrupt control registers,
GPICR1 and GPICR2. GPIO[0:15] interrupt event types are
configured in GPICR1 register, GPIO[16:31] - in GPICR2 register.

This patch adds MPC512x speciffic set_type() callback and
updates config file and comments. Additionally the gpio chip
registration function is changed to use for_each_matching_node()
preventing multiple registration if a node claimes compatibility
with another gpio controller type.

Signed-off-by: Anatolij Gustschin <agust@denx.de>
---
v2:
 - add patch description
 - use match table data to set irq set_type hook as
   recommended
 - refactor to use for_each_matching_node() in
   mpc8xxx_add_gpiochips() as suggested by Grant

 arch/powerpc/platforms/Kconfig     |    7 ++-
 arch/powerpc/sysdev/mpc8xxx_gpio.c |   72 ++++++++++++++++++++++++++++++++----
 2 files changed, 68 insertions(+), 11 deletions(-)
Grant Likely - Aug. 8, 2010, 12:40 a.m.
"Anatolij Gustschin" <agust@denx.de> wrote:

>The GPIO controller of MPC512x is slightly different from
>8xxx GPIO controllers. The register interface is the same
>except the external interrupt control register. The MPC512x
>GPIO controller differentiates between four interrupt event
>types and therefore provides two interrupt control registers,
>GPICR1 and GPICR2. GPIO[0:15] interrupt event types are
>configured in GPICR1 register, GPIO[16:31] - in GPICR2 register.
>
>This patch adds MPC512x speciffic set_type() callback and
>updates config file and comments. Additionally the gpio chip
>registration function is changed to use for_each_matching_node()
>preventing multiple registration if a node claimes compatibility
>with another gpio controller type.
>
>Signed-off-by: Anatolij Gustschin <agust@denx.de>
>---
>v2:
> - add patch description
> - use match table data to set irq set_type hook as
>   recommended
> - refactor to use for_each_matching_node() in
>   mpc8xxx_add_gpiochips() as suggested by Grant
>
> arch/powerpc/platforms/Kconfig     |    7 ++-
> arch/powerpc/sysdev/mpc8xxx_gpio.c |   72 ++++++++++++++++++++++++++++++++----
> 2 files changed, 68 insertions(+), 11 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..87ad655 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 (h->of_node && h->of_node->data)
>+		mpc8xxx_irq_chip.set_type = h->of_node->data;
>+
> 	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);
>@@ -317,18 +366,25 @@ err:
> 	return;
> }
>
>+static struct of_device_id mpc8xxx_gpio_ids[] __initdata = {
>+	{ .compatible = "fsl,mpc8349-gpio", },
>+	{ .compatible = "fsl,mpc8572-gpio", },
>+	{ .compatible = "fsl,mpc8610-gpio", },
>+	{ .compatible = "fsl,mpc5121-gpio", .data = mpc512x_irq_set_type, },
>+	{}
>+};
>+
> static int __init mpc8xxx_add_gpiochips(void)
> {
>+	const struct of_device_id *id;
> 	struct device_node *np;
>
>-	for_each_compatible_node(np, NULL, "fsl,mpc8349-gpio")
>-		mpc8xxx_add_controller(np);
>-
>-	for_each_compatible_node(np, NULL, "fsl,mpc8572-gpio")
>-		mpc8xxx_add_controller(np);
>-
>-	for_each_compatible_node(np, NULL, "fsl,mpc8610-gpio")
>+	for_each_matching_node(np, mpc8xxx_gpio_ids) {
>+		id = of_match_node(mpc8xxx_gpio_ids, np);
>+		if (id)
>+			np->data = id->data;
> 		mpc8xxx_add_controller(np);
>+	}

Sorry, I miss led you.  id->data is fine, but don't use np->data.
Call of_match_node() inside mpc8xxx_add_controller() instead, or
change the function signature to pass it in explicitly...

Actually, there is absolutely no reason to keep mpc8xxx_add_gpiochip()
as a separate function with the simplification of
mpc8xxx_add_gpiochips().  I'd simplify the whole thing by merging the
two functions together.

g.
Anton Vorontsov - Aug. 8, 2010, 7:40 a.m.
On Sat, Aug 07, 2010 at 06:40:22PM -0600, Grant Likely wrote:
[...]
> > static int __init mpc8xxx_add_gpiochips(void)
> > {
> >+	const struct of_device_id *id;
> > 	struct device_node *np;
> >
> >-	for_each_compatible_node(np, NULL, "fsl,mpc8349-gpio")
> >-		mpc8xxx_add_controller(np);
> >-
> >-	for_each_compatible_node(np, NULL, "fsl,mpc8572-gpio")
> >-		mpc8xxx_add_controller(np);
> >-
> >-	for_each_compatible_node(np, NULL, "fsl,mpc8610-gpio")
> >+	for_each_matching_node(np, mpc8xxx_gpio_ids) {
> >+		id = of_match_node(mpc8xxx_gpio_ids, np);
> >+		if (id)
> >+			np->data = id->data;
> > 		mpc8xxx_add_controller(np);
> >+	}
[...]
> Actually, there is absolutely no reason to keep mpc8xxx_add_gpiochip()
> as a separate function with the simplification of
> mpc8xxx_add_gpiochips().  I'd simplify the whole thing by merging the
> two functions together.

You mean mpc8xxx_add_controller()? Putting 65-line function
on a second indentation level, inside the for loop... sounds
like a bad idea.
Grant Likely - Aug. 9, 2010, 5:31 a.m.
On Sun, Aug 8, 2010 at 1:40 AM, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> On Sat, Aug 07, 2010 at 06:40:22PM -0600, Grant Likely wrote:
> [...]
>> > static int __init mpc8xxx_add_gpiochips(void)
>> > {
>> >+    const struct of_device_id *id;
>> >     struct device_node *np;
>> >
>> >-    for_each_compatible_node(np, NULL, "fsl,mpc8349-gpio")
>> >-            mpc8xxx_add_controller(np);
>> >-
>> >-    for_each_compatible_node(np, NULL, "fsl,mpc8572-gpio")
>> >-            mpc8xxx_add_controller(np);
>> >-
>> >-    for_each_compatible_node(np, NULL, "fsl,mpc8610-gpio")
>> >+    for_each_matching_node(np, mpc8xxx_gpio_ids) {
>> >+            id = of_match_node(mpc8xxx_gpio_ids, np);
>> >+            if (id)
>> >+                    np->data = id->data;
>> >             mpc8xxx_add_controller(np);
>> >+    }
> [...]
>> Actually, there is absolutely no reason to keep mpc8xxx_add_gpiochip()
>> as a separate function with the simplification of
>> mpc8xxx_add_gpiochips().  I'd simplify the whole thing by merging the
>> two functions together.
>
> You mean mpc8xxx_add_controller()? Putting 65-line function
> on a second indentation level, inside the for loop... sounds
> like a bad idea.

That's a good point.  I was thinking about it from the wrong way
around (that the function could bail at the top on a non-match; which
obviously isn't the case).  Anatolij, I was wrong on this point.
Sorry I didn't reply sooner before you respun the patch.  :-(

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..87ad655 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 (h->of_node && h->of_node->data)
+		mpc8xxx_irq_chip.set_type = h->of_node->data;
+
 	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);
@@ -317,18 +366,25 @@  err:
 	return;
 }
 
+static struct of_device_id mpc8xxx_gpio_ids[] __initdata = {
+	{ .compatible = "fsl,mpc8349-gpio", },
+	{ .compatible = "fsl,mpc8572-gpio", },
+	{ .compatible = "fsl,mpc8610-gpio", },
+	{ .compatible = "fsl,mpc5121-gpio", .data = mpc512x_irq_set_type, },
+	{}
+};
+
 static int __init mpc8xxx_add_gpiochips(void)
 {
+	const struct of_device_id *id;
 	struct device_node *np;
 
-	for_each_compatible_node(np, NULL, "fsl,mpc8349-gpio")
-		mpc8xxx_add_controller(np);
-
-	for_each_compatible_node(np, NULL, "fsl,mpc8572-gpio")
-		mpc8xxx_add_controller(np);
-
-	for_each_compatible_node(np, NULL, "fsl,mpc8610-gpio")
+	for_each_matching_node(np, mpc8xxx_gpio_ids) {
+		id = of_match_node(mpc8xxx_gpio_ids, np);
+		if (id)
+			np->data = id->data;
 		mpc8xxx_add_controller(np);
+	}
 
 	return 0;
 }