[PATCH RESEND v8] gpio: Device tree support for LPC32xx

[PATCH RESEND v8] gpio: Device tree support for LPC32xx

[Roland Stigge]

This patch adds device tree support for gpio-lpc32xx.c.

The various GPIO groups correspond to the different irregular GPIO banks of the
LPC32xx hardware. Most importantly, there are the GPI(0-27), GPO(0-23) and
GPIO(0-5) banks. Each bank must be registered individually since they are
implemented with different functions. Registration is done via gpiochip_add()
which requires an individual gpio bank subnode in the device tree for each
call.

Signed-off-by: Roland Stigge <stigge@antcom.de>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>

---

Applies to v3.4-rc6

Changes since last version:
* Updated patch description (above)

Thanks to Jean-Christophe PLAGNIOL-VILLARD, Grant Likely, Arnd Bergmann, Jon
Smirl, Mark Brown for reviewing!

I can also provide a git branch to pull from. On top of which one should I
provide it, if not mainline rc?
 
 Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt |   65 ++++++++++++++++
 arch/arm/mach-lpc32xx/include/mach/gpio.h               |    9 +-
 drivers/gpio/gpio-lpc32xx.c                             |   56 +++++++++++++
 3 files changed, 127 insertions(+), 3 deletions(-)

--- /dev/null
+++ linux-2.6/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt
@@ -0,0 +1,65 @@
+NXP LPC32xx SoC GPIO controller
+
+Required properties:
+- compatible: must be "nxp,lpc3220-gpio"
+- reg: Physical base address and length of the controller's registers.
+- #address-cells: Always 1, for indexing of the subnodes (GPIO groups of the
+  SoC)
+- #size-cells: Always 0
+
+Required properties of sub-nodes which describe the GPIO groups of LPC32xx:
+- gpio-controller: Marks the device node as a GPIO controller.
+- #gpio-cells: Should be two. The first cell is the pin number and the
+  second cell is used to specify optional parameters:
+  - bit 0 specifies polarity (0 for normal, 1 for inverted)
+- reg: Index of the GPIO group
+
+Optional properties of sub-nodes which describe the GPIO groups of LPC32xx:
+- status: Set to "disabled" if you don't use the respective GPIO group
+  of the LPC32xx SoC
+
+Example:
+
+	gpio: gpio@40028000 {
+		compatible = "nxp,lpc3220-gpio";
+		reg = <0x40028000 0x1000>;
+		/* create a private address space for enumeration */
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		gpio_p0: gpio-bank@0 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			reg = <0>;
+		};
+
+		gpio_p1: gpio-bank@1 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			reg = <1>;
+		};
+
+		gpio_p2: gpio-bank@2 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			reg = <2>;
+		};
+
+		gpio_p3: gpio-bank@3 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			reg = <3>;
+		};
+
+		gpi_p3: gpio-bank@4 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			reg = <4>;
+		};
+
+		gpo_p3: gpio-bank@5 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			reg = <5>;
+		};
+	};
--- linux-2.6.orig/arch/arm/mach-lpc32xx/include/mach/gpio.h
+++ linux-2.6/arch/arm/mach-lpc32xx/include/mach/gpio.h
@@ -1 +1,8 @@
-/* empty */
+#ifndef __MACH_GPIO_H
+#define __MACH_GPIO_H
+
+#include "gpio-lpc32xx.h"
+
+#define ARCH_NR_GPIOS (LPC32XX_GPO_P3_GRP + LPC32XX_GPO_P3_MAX)
+
+#endif /* __MACH_GPIO_H */
--- linux-2.6.orig/drivers/gpio/gpio-lpc32xx.c
+++ linux-2.6/drivers/gpio/gpio-lpc32xx.c
@@ -21,6 +21,9 @@
 #include <linux/io.h>
 #include <linux/errno.h>
 #include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
 
 #include <mach/hardware.h>
 #include <mach/platform.h>
@@ -454,10 +457,59 @@ static struct lpc32xx_gpio_chip lpc32xx_
 	},
 };
 
+/* Empty now, can be removed later when mach-lpc32xx is finally switched over
+ * to DT support
+ */
 void __init lpc32xx_gpio_init(void)
 {
+}
+
+static void __devinit lpc32xx_gpiochip_add_dt(struct device_node *node)
+{
+	if (of_device_is_available(node)) {
+		u32 index;
+		struct gpio_chip *chip;
+
+		if (of_property_read_u32(node, "reg", &index) < 0)
+			return;
+		if (index >= ARRAY_SIZE(lpc32xx_gpiochip))
+			return;
+		chip = &lpc32xx_gpiochip[index].chip;
+		chip->of_node = of_node_get(node);
+		gpiochip_add(chip);
+	}
+}
+
+static int __devinit lpc32xx_gpio_probe(struct platform_device *pdev)
+{
+	struct device_node *node;
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(lpc32xx_gpiochip); i++)
-		gpiochip_add(&lpc32xx_gpiochip[i].chip);
+	if (pdev->dev.of_node) {
+		for_each_child_of_node(pdev->dev.of_node, node)
+			lpc32xx_gpiochip_add_dt(node);
+	} else {
+		for (i = 0; i < ARRAY_SIZE(lpc32xx_gpiochip); i++)
+			gpiochip_add(&lpc32xx_gpiochip[i].chip);
+	}
+
+	return 0;
 }
+
+#ifdef CONFIG_OF
+static struct of_device_id lpc32xx_gpio_of_match[] __devinitdata = {
+	{ .compatible = "nxp,lpc3220-gpio", },
+	{ },
+};
+#endif
+
+static struct platform_driver lpc32xx_gpio_driver = {
+	.driver		= {
+		.name	= "lpc32xx-gpio",
+		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(lpc32xx_gpio_of_match),
+	},
+	.probe		= lpc32xx_gpio_probe,
+};
+
+module_platform_driver(lpc32xx_gpio_driver);

Re: [PATCH RESEND v8] gpio: Device tree support for LPC32xx

[Grant Likely]

On Thu, 10 May 2012 21:57:18 +0200, Roland Stigge <stigge-uj/7R2tJ6VmzQB+pC5nmwQ@public.gmane.org> wrote:
> This patch adds device tree support for gpio-lpc32xx.c.
> 
> The various GPIO groups correspond to the different irregular GPIO banks of the
> LPC32xx hardware. Most importantly, there are the GPI(0-27), GPO(0-23) and
> GPIO(0-5) banks. Each bank must be registered individually since they are
> implemented with different functions. Registration is done via gpiochip_add()
> which requires an individual gpio bank subnode in the device tree for each
> call.
> 
> Signed-off-by: Roland Stigge <stigge-uj/7R2tJ6VmzQB+pC5nmwQ@public.gmane.org>
> Reviewed-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> 
> ---
> 
> Applies to v3.4-rc6
> 
> Changes since last version:
> * Updated patch description (above)
> 
> Thanks to Jean-Christophe PLAGNIOL-VILLARD, Grant Likely, Arnd Bergmann, Jon
> Smirl, Mark Brown for reviewing!
> 
> I can also provide a git branch to pull from. On top of which one should I
> provide it, if not mainline rc?
>  
>  Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt |   65 ++++++++++++++++
>  arch/arm/mach-lpc32xx/include/mach/gpio.h               |    9 +-
>  drivers/gpio/gpio-lpc32xx.c                             |   56 +++++++++++++
>  3 files changed, 127 insertions(+), 3 deletions(-)
> 
> --- /dev/null
> +++ linux-2.6/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt
> @@ -0,0 +1,65 @@
> +NXP LPC32xx SoC GPIO controller
> +
> +Required properties:
> +- compatible: must be "nxp,lpc3220-gpio"
> +- reg: Physical base address and length of the controller's registers.
> +- #address-cells: Always 1, for indexing of the subnodes (GPIO groups of the
> +  SoC)
> +- #size-cells: Always 0
> +
> +Required properties of sub-nodes which describe the GPIO groups of LPC32xx:
> +- gpio-controller: Marks the device node as a GPIO controller.
> +- #gpio-cells: Should be two. The first cell is the pin number and the
> +  second cell is used to specify optional parameters:
> +  - bit 0 specifies polarity (0 for normal, 1 for inverted)
> +- reg: Index of the GPIO group
> +
> +Optional properties of sub-nodes which describe the GPIO groups of LPC32xx:
> +- status: Set to "disabled" if you don't use the respective GPIO group
> +  of the LPC32xx SoC
> +
> +Example:
> +
> +	gpio: gpio@40028000 {
> +		compatible = "nxp,lpc3220-gpio";
> +		reg = <0x40028000 0x1000>;
> +		/* create a private address space for enumeration */
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		gpio_p0: gpio-bank@0 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			reg = <0>;
> +		};
> +
> +		gpio_p1: gpio-bank@1 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			reg = <1>;
> +		};
> +
> +		gpio_p2: gpio-bank@2 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			reg = <2>;
> +		};
> +
> +		gpio_p3: gpio-bank@3 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			reg = <3>;
> +		};
> +
> +		gpi_p3: gpio-bank@4 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			reg = <4>;
> +		};
> +
> +		gpo_p3: gpio-bank@5 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			reg = <5>;
> +		};
> +	};

Ugh.  I know this patch has been around for a while, but I can't help
it, this binding is just really ugly.  It results in a big chunk of
boilerplate for the gpio controller node just to enumerate the banks
which will *always* be there.

The real problem is that one of_xlate function cannot currently be
used for multiple banks, but there is no good reason why that should
be so.

How about the following instead (very rough, there should be a cleaner
way to update the gc pointer in of_xlate):

	gpio: gpio@40028000 {
		compatible = "nxp,lpc3220-gpio";
		reg = <0x40028000 0x1000>;
		gpio-controller;
		#gpio-cells = <3>; /* bank, pin, flags */
	}

Modify gpiolib-of to use:
of_xlate(struct gpio_chip **gc, const struct of_phandle_args *gpiospec, u32 *flags);
(so that the value of gc can be modified)

And then in the driver:

static int lpc32xx_of_xlate(struct gpio_chip **gc, const struct
                            of_phandle_args *gpiospec, u32 *flags)
{
	if (WARN_ON(gpiospec->args_count < 3))
		return -EINVAL;

	*gc = &lpc32xx_gpiochip[gpiospec->args[0]].chip;
	if (flags)
		*flags = gpiospec->args[2];
	return gpiospec->args[1];
}

Make all the gpiochips use the same of_xlate function and of_node
pointer so that it doesn't matter which one gets matched because the
xlate always completely resolves the gpio_chip.

Another way to do it would be to split the of_xlate functions out of
gpio_chip entirely and have a separate registry for gpio device_nodes
and their xlate functions.

g.

Re: [PATCH RESEND v8] gpio: Device tree support for LPC32xx

[Arnd Bergmann]

On Thursday 10 May 2012, Grant Likely wrote:
> How about the following instead (very rough, there should be a cleaner
> way to update the gc pointer in of_xlate):
> 
>         gpio: gpio@40028000 {
>                 compatible = "nxp,lpc3220-gpio";
>                 reg = <0x40028000 0x1000>;
>                 gpio-controller;
>                 #gpio-cells = <3>; /* bank, pin, flags */
>         }
> 

I thought about that when I suggested the current binding to Roland,
but the problem with this is that passing the bank as a number is
not very intuitive when the data sheet has separate number spaces
for gpio, gpi and gpo here, so it seemed more natural to go with what
the data sheet describes.

	Arnd

Re: [PATCH RESEND v8] gpio: Device tree support for LPC32xx

[Roland Stigge]

On 05/11/2012 02:19 PM, Arnd Bergmann wrote:
> On Thursday 10 May 2012, Grant Likely wrote:
>> How about the following instead (very rough, there should be a cleaner
>> way to update the gc pointer in of_xlate):
>>
>>         gpio: gpio@40028000 {
>>                 compatible = "nxp,lpc3220-gpio";
>>                 reg = <0x40028000 0x1000>;
>>                 gpio-controller;
>>                 #gpio-cells = <3>; /* bank, pin, flags */
>>         }
>>
> 
> I thought about that when I suggested the current binding to Roland,
> but the problem with this is that passing the bank as a number is
> not very intuitive when the data sheet has separate number spaces
> for gpio, gpi and gpo here, so it seemed more natural to go with what
> the data sheet describes.

Right. Personally, I would be fine with either of my v8 (all banks in
dt, referenced naturally by name) and v9 (one simple DT entry for the
whole GPIO controller, integer index for referencing banks) patches.

Consider the DT-documented mapping in the latter case:

      0: GPIO P0
      1: GPIO P1
      2: GPIO P2
      3: GPIO P3
      4: GPI P3
      5: GPO P3

Not too difficult and would also be acceptable, IMO.

So Arnd and Grant, please agree one of those and pick it. :-)

Thanks in advance,

Roland

Re: [PATCH RESEND v8] gpio: Device tree support for LPC32xx

[Arnd Bergmann]

On Friday 11 May 2012, Roland Stigge wrote:
> Right. Personally, I would be fine with either of my v8 (all banks in
> dt, referenced naturally by name) and v9 (one simple DT entry for the
> whole GPIO controller, integer index for referencing banks) patches.
> 
> Consider the DT-documented mapping in the latter case:
> 
>       0: GPIO P0
>       1: GPIO P1
>       2: GPIO P2
>       3: GPIO P3
>       4: GPI P3
>       5: GPO P3
> 
> Not too difficult and would also be acceptable, IMO.
> 
> So Arnd and Grant, please agree one of those and pick it. :-)
> 

Grant is maintainer for both GPIO and DT, so his opinion is what
counts in this case.
I was merely giving the background on how we got there so he
can make an informed decision.

	Arnd

Re: [PATCH RESEND v8] gpio: Device tree support for LPC32xx

[Grant Likely]

On Fri, 11 May 2012 13:22:14 +0000, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 11 May 2012, Roland Stigge wrote:
> > Right. Personally, I would be fine with either of my v8 (all banks in
> > dt, referenced naturally by name) and v9 (one simple DT entry for the
> > whole GPIO controller, integer index for referencing banks) patches.
> > 
> > Consider the DT-documented mapping in the latter case:
> > 
> >       0: GPIO P0
> >       1: GPIO P1
> >       2: GPIO P2
> >       3: GPIO P3
> >       4: GPI P3
> >       5: GPO P3
> > 
> > Not too difficult and would also be acceptable, IMO.
> > 
> > So Arnd and Grant, please agree one of those and pick it. :-)
> > 
> 
> Grant is maintainer for both GPIO and DT, so his opinion is what
> counts in this case.
> I was merely giving the background on how we got there so he
> can make an informed decision.

It adds a lot of boilerplate to the DT to split it up into nodes and
mapping index to a bank isn't onerous.  I think I like the single node
with #gpio-cells = <3> better.  When DTC handles named constants then
it will be even easier.

Just need to decide on the best way to implement it.  Arnd, can you
review Roland's v9 patches for me and give me your opinion?

g.