[PATCH] gpio: Device tree support for LPC32xx

[PATCH] gpio: Device tree support for LPC32xx

[Roland Stigge]

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

Signed-off-by: Roland Stigge <stigge@antcom.de>

---

 Applies to v3.4-rc1

 Can add this patch to the LPC32xx series for an update, if necessary.

 Thanks to Arnd Bergmann for the help with registering GPIO via OF!

 Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt |   71 ++++++++++++++++
 arch/arm/mach-lpc32xx/include/mach/gpio.h               |    9 +-
 drivers/gpio/gpio-lpc32xx.c                             |   45 +++++++++-
 3 files changed, 120 insertions(+), 5 deletions(-)

--- /dev/null
+++ linux-2.6/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt
@@ -0,0 +1,71 @@
+NXP LPC32xx SoC GPIO controller
+
+Required properties:
+- compatible: "nxp,lpc32xx-gpio"
+- reg: Physical base address and length of the controller's registers.
+- #address-cells: For indexing of the subnodes (GPIO groups of the SoC)
+- #size-cells: Always 0
+- #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)
+
+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
+- gpio-lines: Number of GPIOs in that subnode/GPIO group
+
+Example:
+
+	gpio: gpio@40028000 {
+		compatible = "nxp,lpc32xx-gpio";
+		reg = <0x40028000 0x1000>;
+		/* create a private address space for enumeration */
+		#address-cells = <1>;
+		#size-cells = <0>;
+		#gpio-cells = <2>;
+
+		gpio_p0: gpio-bank@0 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-lines = <8>;
+			reg = <0>;
+		};
+
+		gpio_p1: gpio-bank@1 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-lines = <24>;
+			reg = <1>;
+		};
+
+		gpio_p2: gpio-bank@2 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-lines = <13>;
+			reg = <2>;
+		};
+
+		gpio_p3: gpio-bank@3 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-lines = <6>;
+			reg = <3>;
+		};
+
+		gpi_p3: gpio-bank@4 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-lines = <28>;
+			reg = <4>;
+		};
+
+		gpo_p3: gpio-bank@5 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-lines = <24>;
+			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,44 @@ static struct lpc32xx_gpio_chip lpc32xx_
 	},
 };
 
-void __init lpc32xx_gpio_init(void)
+static int __devinit lpc32xx_gpio_probe(struct platform_device *pdev)
 {
-	int i;
+	struct device_node *node;
 
-	for (i = 0; i < ARRAY_SIZE(lpc32xx_gpiochip); i++)
-		gpiochip_add(&lpc32xx_gpiochip[i].chip);
+	for_each_child_of_node(pdev->dev.of_node, node) {
+		if (of_device_is_available(node)) {
+			u32 index;
+			struct gpio_chip *chip;
+			if (of_property_read_u32(node, "reg", &index) < 0)
+				continue;
+			if (index >= ARRAY_SIZE(lpc32xx_gpiochip))
+				continue;
+			chip = &lpc32xx_gpiochip[index].chip;
+			chip->of_node = of_node_get(node);
+			gpiochip_add(chip);
+		}
+	}
+
+	return 0;
 }
+
+static struct of_device_id lpc32xx_gpio_of_match[] __devinitdata = {
+	{ .compatible = "nxp,lpc32xx-gpio", },
+	{ },
+};
+
+static struct platform_driver lpc32xx_gpio_driver = {
+	.driver		= {
+		.name	= "lpc32xx-gpio",
+		.owner	= THIS_MODULE,
+		.of_match_table = lpc32xx_gpio_of_match,
+	},
+	.probe		= lpc32xx_gpio_probe,
+};
+
+static int __init lpc32xx_gpio_init(void)
+{
+	return platform_driver_register(&lpc32xx_gpio_driver);
+}
+postcore_initcall(lpc32xx_gpio_init);
+

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

[Arnd Bergmann]

On Monday 02 April 2012, Roland Stigge wrote:
> This patch adds device tree support for gpio-lpc32xx.c
> 
> Signed-off-by: Roland Stigge <stigge@antcom.de>

Hi Roland,

I'm glad it worked. Some more comments:

> --- /dev/null
> +++ linux-2.6/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt
> @@ -0,0 +1,71 @@
> +NXP LPC32xx SoC GPIO controller
> +
> +Required properties:
> +- compatible: "nxp,lpc32xx-gpio"
> +- reg: Physical base address and length of the controller's registers.
> +- #address-cells: For indexing of the subnodes (GPIO groups of the SoC)
> +- #size-cells: Always 0
> +- #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)

The description for #address-cells should mention that it's always <1>.
There should be no #gpio-cells in this node.

> +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
> +- gpio-lines: Number of GPIOs in that subnode/GPIO group

While I suggested the gpio-lines property, I'm not sure if it's worth
including it when you don't actually use it.

You could add code to the probe function that sets the respective
fields in the gpio_chip structure, which would be particularly
interesting when you disable the unused banks by adding a
status="disabled" property (you already check for of_device_is_available()
in the loop). That will only work after all your gpio using code has
been converted to device tree probing, because it means the
hardcoded numbers don't work any more.

You can also let the user change that number if only the first X gpios
in one bank are actually used, or if some lpc32xx variants have fewer
gpio pins than others.

	Arnd

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

[Roland Stigge]

Hi Arnd,

thanks for your further suggestions. I'm integrating them, comments below:

On 04/03/2012 10:29 AM, Arnd Bergmann wrote:
>> +- gpio-lines: Number of GPIOs in that subnode/GPIO group
> 
> While I suggested the gpio-lines property, I'm not sure if it's worth
> including it when you don't actually use it.

Right. I'm actually removing it now since for enabling individual GPIO 
lines selectively, it's not flexible enough, anyway: only the first n 
lines of a GPIO "group" could be chosen.

However, I will keep (and document) the possible status="disabled" 
property to disable whole GPIO groups of LPC32xx, if really necessary.

Looks like all LPC32xx variants have and will have the same GPIO layout.

To fix disruptiveness of this patch, I'll make it support both DT and 
non-DT so it doesn't depend on LPC32xx being switched together 
atomically. (Posting below).

Roland



        if (pdev->dev.of_node) {
                for_each_child_of_node(pdev->dev.of_node, node) {
                        if (of_device_is_available(node)) {
                                u32 index;
                                struct gpio_chip *chip;
                                if (of_property_read_u32(node, "reg", &index) <  0)
                                        continue;
                                if (index >= ARRAY_SIZE(lpc32xx_gpiochip))
                                        continue;
                                chip = &lpc32xx_gpiochip[index].chip;
                                chip->of_node = of_node_get(node);
                                gpiochip_add(chip);
                        }
                }
        } else {
                for (i = 0; i < ARRAY_SIZE(lpc32xx_gpiochip); i++)
                	gpiochip_add(&lpc32xx_gpiochip[i].chip);
        }

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

[Grant Likely]

On Tue,  3 Apr 2012 00:58:33 +0200, Roland Stigge <stigge@antcom.de> wrote:
> This patch adds device tree support for gpio-lpc32xx.c
> 
> Signed-off-by: Roland Stigge <stigge@antcom.de>
> 
> ---
> 
>  Applies to v3.4-rc1
> 
>  Can add this patch to the LPC32xx series for an update, if necessary.
> 
>  Thanks to Arnd Bergmann for the help with registering GPIO via OF!

Hi Roland,

Comments below.

> 
>  Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt |   71 ++++++++++++++++
>  arch/arm/mach-lpc32xx/include/mach/gpio.h               |    9 +-
>  drivers/gpio/gpio-lpc32xx.c                             |   45 +++++++++-
>  3 files changed, 120 insertions(+), 5 deletions(-)
> 
> --- /dev/null
> +++ linux-2.6/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt
> @@ -0,0 +1,71 @@
> +NXP LPC32xx SoC GPIO controller
> +
> +Required properties:
> +- compatible: "nxp,lpc32xx-gpio"
> +- reg: Physical base address and length of the controller's registers.
> +- #address-cells: For indexing of the subnodes (GPIO groups of the SoC)
> +- #size-cells: Always 0
> +- #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)

Having #gpio-cells in the parent node doesn't make much sense when it
looks like the users reference the child node banks directly and which
have their own #gpio-cells properties.

> +
> +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
> +- gpio-lines: Number of GPIOs in that subnode/GPIO group

The driver doesn't appear to be using the gpio-lines property.  Is it
really necessary?

> +
> +Example:
> +
> +	gpio: gpio@40028000 {
> +		compatible = "nxp,lpc32xx-gpio";
> +		reg = <0x40028000 0x1000>;
> +		/* create a private address space for enumeration */
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		#gpio-cells = <2>;
> +
> +		gpio_p0: gpio-bank@0 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			gpio-lines = <8>;
> +			reg = <0>;
> +		};
> +
> +		gpio_p1: gpio-bank@1 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			gpio-lines = <24>;
> +			reg = <1>;
> +		};
> +
> +		gpio_p2: gpio-bank@2 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			gpio-lines = <13>;
> +			reg = <2>;
> +		};
> +
> +		gpio_p3: gpio-bank@3 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			gpio-lines = <6>;
> +			reg = <3>;
> +		};
> +
> +		gpi_p3: gpio-bank@4 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			gpio-lines = <28>;
> +			reg = <4>;
> +		};
> +
> +		gpo_p3: gpio-bank@5 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			gpio-lines = <24>;
> +			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,44 @@ static struct lpc32xx_gpio_chip lpc32xx_
>  	},
>  };
>  
> -void __init lpc32xx_gpio_init(void)
> +static int __devinit lpc32xx_gpio_probe(struct platform_device *pdev)
>  {
> -	int i;
> +	struct device_node *node;
>  
> -	for (i = 0; i < ARRAY_SIZE(lpc32xx_gpiochip); i++)
> -		gpiochip_add(&lpc32xx_gpiochip[i].chip);
> +	for_each_child_of_node(pdev->dev.of_node, node) {
> +		if (of_device_is_available(node)) {
> +			u32 index;
> +			struct gpio_chip *chip;
> +			if (of_property_read_u32(node, "reg", &index) < 0)
> +				continue;
> +			if (index >= ARRAY_SIZE(lpc32xx_gpiochip))
> +				continue;
> +			chip = &lpc32xx_gpiochip[index].chip;
> +			chip->of_node = of_node_get(node);
> +			gpiochip_add(chip);
> +		}
> +	}
> +
> +	return 0;
>  }
> +
> +static struct of_device_id lpc32xx_gpio_of_match[] __devinitdata = {
> +	{ .compatible = "nxp,lpc32xx-gpio", },
> +	{ },
> +};
> +
> +static struct platform_driver lpc32xx_gpio_driver = {
> +	.driver		= {
> +		.name	= "lpc32xx-gpio",
> +		.owner	= THIS_MODULE,
> +		.of_match_table = lpc32xx_gpio_of_match,
> +	},
> +	.probe		= lpc32xx_gpio_probe,
> +};
> +
> +static int __init lpc32xx_gpio_init(void)
> +{
> +	return platform_driver_register(&lpc32xx_gpio_driver);
> +}
> +postcore_initcall(lpc32xx_gpio_init);

module_platform_driver() please.  Also, now that deferred probe is
merged, there should no longer be any need to mess around with
initcall levels to get gpio drivers probed early.

g.

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

[Roland Stigge]

Hi Grant,

thanks for your notes!

On 03/04/12 17:04, Grant Likely wrote:
>> +postcore_initcall(lpc32xx_gpio_init);
> 
> module_platform_driver() please.  Also, now that deferred probe is
> merged, there should no longer be any need to mess around with
> initcall levels to get gpio drivers probed early.

With module_platform_driver(), I get a new error reported after this
change, coming much earlier before the GPIO registration (which is
deferred now, of course):

===============================================================
Error requesting gpio 50
...
gpiochip_add: registered GPIOs 0 to 7 on device: gpio_p0
gpiochip_add: registered GPIOs 8 to 31 on device: gpio_p1
gpiochip_add: registered GPIOs 32 to 44 on device: gpio_p2
gpiochip_add: registered GPIOs 45 to 50 on device: gpio_p3
gpiochip_add: registered GPIOs 51 to 78 on device: gpi_p3
gpiochip_add: registered GPIOs 79 to 102 on device: gpo_p3
===============================================================

Seems to be caused by this device tree:

        leds {
                compatible = "gpio-leds";
                led0 {  
                        gpios = <&gpo_p3 1 1>; /* GPO_P3 1, GPIO 80, active low */
                        linux,default-trigger = "heartbeat";
                        default-state = "keep";
                };
        };

Are you sure that module_platform_driver() can already handle
this in v3.4-rc1?

Thanks in advance,

Roland

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

[Roland Stigge]

On 04/04/12 01:39, Roland Stigge wrote:
> ===============================================================
> Error requesting gpio 50
> ...
> gpiochip_add: registered GPIOs 0 to 7 on device: gpio_p0
> gpiochip_add: registered GPIOs 8 to 31 on device: gpio_p1
> gpiochip_add: registered GPIOs 32 to 44 on device: gpio_p2
> gpiochip_add: registered GPIOs 45 to 50 on device: gpio_p3
> gpiochip_add: registered GPIOs 51 to 78 on device: gpi_p3
> gpiochip_add: registered GPIOs 79 to 102 on device: gpo_p3
> ===============================================================
> 
> Seems to be caused by this device tree:
> 
>         leds {
>                 compatible = "gpio-leds";
>                 led0 {  
>                         gpios = <&gpo_p3 1 1>; /* GPO_P3 1, GPIO 80, active low */
>                         linux,default-trigger = "heartbeat";
>                         default-state = "keep";
>                 };
>         };

No, it was "another" 50 (decimal, not hex). :-) Was caused by old early
board init code, eliminated anyway.

Thanks,

Roland

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

[Mark Brown]

On Wed, Apr 04, 2012 at 01:39:37AM +0200, Roland Stigge wrote:
> On 03/04/12 17:04, Grant Likely wrote:

> >> +postcore_initcall(lpc32xx_gpio_init);

> ===============================================================
> Error requesting gpio 50
> ...
> gpiochip_add: registered GPIOs 0 to 7 on device: gpio_p0

> Are you sure that module_platform_driver() can already handle
> this in v3.4-rc1?

For the present moment it needs explicit handling in each GPIO user to
translate a failure to get a GPIO into -EPROBE_DEFER.  I sent a patch
which would change the default return code for failed GPIO lookups to
that which should get most users doing the right thing but I've had no
response to that patch so far so it's not in any tree.