[PATCH v2] gpio: Device tree support for LPC32xx

[PATCH v2] 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

 Changes since last version:
 * Documented property #address-cells (always 1)
 * Documented property status="disabled"
 * Removed gpio-lines property
 * Removed #gpio-cells from the parent node
 * Support both DT and non-DT to make patch less disruptive and independent
   from DT switch of mach-lpc32xx
 * Use module_platform_driver()

 Thanks to Arnd Bergmann and Grant Likely for reviewing!

 Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt |   65 ++++++++++++++++
 arch/arm/mach-lpc32xx/include/mach/gpio.h               |    9 +-
 drivers/gpio/gpio-lpc32xx.c                             |   52 ++++++++++++
 3 files changed, 123 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: "nxp,lpc32xx-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 LPC32 SoC
+
+Example:
+
+	gpio: gpio at 40028000 {
+		compatible = "nxp,lpc32xx-gpio";
+		reg = <0x40028000 0x1000>;
+		/* create a private address space for enumeration */
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		gpio_p0: gpio-bank at 0 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			reg = <0>;
+		};
+
+		gpio_p1: gpio-bank at 1 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			reg = <1>;
+		};
+
+		gpio_p2: gpio-bank at 2 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			reg = <2>;
+		};
+
+		gpio_p3: gpio-bank at 3 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			reg = <3>;
+		};
+
+		gpi_p3: gpio-bank at 4 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			reg = <4>;
+		};
+
+		gpo_p3: gpio-bank at 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,55 @@ 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 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) {
+			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);
+	}
+
+	return 0;
 }
+
+#ifdef CONFIG_OF
+static struct of_device_id lpc32xx_gpio_of_match[] __devinitdata = {
+	{ .compatible = "nxp,lpc32xx-gpio", },
+	{ },
+};
+#endif
+
+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,
+};
+
+module_platform_driver(lpc32xx_gpio_driver);

[PATCH v2] gpio: Device tree support for LPC32xx

[Arnd Bergmann]

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

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

Just one more comment:

> +#ifdef CONFIG_OF
> +static struct of_device_id lpc32xx_gpio_of_match[] __devinitdata = {
> +	{ .compatible = "nxp,lpc32xx-gpio", },
> +	{ },
> +};
> +#endif
> +
> +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,
> +};

The #ifdef above is not helpful, it will cause a build error in the
of_match_table= assignment. Since the plan for this driver is to
be devicetree-only, it would be appropriate to just remove the
#ifdef, but adding of_match_ptr() is ok too.

	Arnd

[PATCH v2] gpio: Device tree support for LPC32xx

[Roland Stigge]

On 04/04/2012 10:48 AM, Arnd Bergmann wrote:
>> +#ifdef CONFIG_OF
>> +static struct of_device_id lpc32xx_gpio_of_match[] __devinitdata = {
>> +	{ .compatible = "nxp,lpc32xx-gpio", },
>> +	{ },
>> +};
>> +#endif
>> +
>> +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,
>> +};
> 
> The #ifdef above is not helpful, it will cause a build error in the
> of_match_table= assignment. Since the plan for this driver is to
> be devicetree-only, it would be appropriate to just remove the
> #ifdef, but adding of_match_ptr() is ok too.

I think I just forgot the of_match_ptr(). Will post an update with a
repo for gpio to pull from.

Thanks for the note!

Roland

[PATCH v2] gpio: Device tree support for LPC32xx

[Grant Likely]

On Wed,  4 Apr 2012 01:58:38 +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
> 
>  Changes since last version:
>  * Documented property #address-cells (always 1)
>  * Documented property status="disabled"
>  * Removed gpio-lines property
>  * Removed #gpio-cells from the parent node
>  * Support both DT and non-DT to make patch less disruptive and independent
>    from DT switch of mach-lpc32xx
>  * Use module_platform_driver()
> 
>  Thanks to Arnd Bergmann and Grant Likely for reviewing!
> 
>  Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt |   65 ++++++++++++++++
>  arch/arm/mach-lpc32xx/include/mach/gpio.h               |    9 +-
>  drivers/gpio/gpio-lpc32xx.c                             |   52 ++++++++++++
>  3 files changed, 123 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: "nxp,lpc32xx-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

If these are merely contiguous register banks of 32 gpio lines, then
established convention is pretty much to only use one node and make
the translate function decode bank and bit out of the gpio specifier.
There isn't a whole lot of value it having all the sub nodes when
there isn't anything significantly different between them.

How is documentation for this gpio controller written?  Is it one big
number space, or are the gpio numbers split up into banks?  You have
two choices here for the encoding; either just use #gpio-cells=<2> as
done now, or use #gpio-cells=<3> and using cell0 for the bank and
cell1 for the bit.  You should choose the option that best lines up
with how the hardware documents its gpio pins because that will be the
most user-friendly.

g.

> +
> +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 LPC32 SoC
> +
> +Example:
> +
> +	gpio: gpio at 40028000 {
> +		compatible = "nxp,lpc32xx-gpio";
> +		reg = <0x40028000 0x1000>;
> +		/* create a private address space for enumeration */
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		gpio_p0: gpio-bank at 0 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			reg = <0>;
> +		};
> +
> +		gpio_p1: gpio-bank at 1 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			reg = <1>;
> +		};
> +
> +		gpio_p2: gpio-bank at 2 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			reg = <2>;
> +		};
> +
> +		gpio_p3: gpio-bank at 3 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			reg = <3>;
> +		};
> +
> +		gpi_p3: gpio-bank at 4 {
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			reg = <4>;
> +		};
> +
> +		gpo_p3: gpio-bank at 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,55 @@ 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 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) {
> +			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);
> +	}
> +
> +	return 0;
>  }
> +
> +#ifdef CONFIG_OF
> +static struct of_device_id lpc32xx_gpio_of_match[] __devinitdata = {
> +	{ .compatible = "nxp,lpc32xx-gpio", },
> +	{ },
> +};
> +#endif
> +
> +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,
> +};
> +
> +module_platform_driver(lpc32xx_gpio_driver);

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies,Ltd.

[PATCH v2] gpio: Device tree support for LPC32xx

[Roland Stigge]

Hi Grant,

thanks for your suggestions about LPC32xx GPIO DT.

Regarding the numbering:

On 07/04/12 05:53, Grant Likely wrote:
>> +Required properties:
>> +- compatible: "nxp,lpc32xx-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
> 
> If these are merely contiguous register banks of 32 gpio lines, then
> established convention is pretty much to only use one node and make
> the translate function decode bank and bit out of the gpio specifier.
> There isn't a whole lot of value it having all the sub nodes when
> there isn't anything significantly different between them.

Please consider how the groups are specified in
drivers/gpio/gpio-lpc32xx.c. They each have different numbers of lines
and GPIO / GPI / GPO functionality. So they also have different callback
sets, and we need to do separate gpiochip_add()s which leads to the
separate gpio-bank specifications in the dtsi file. Separate enabling of
those banks via OF are a nice by-product.

So I would like to keep it that way.

What do you think?

Thanks in advance,

Roland

>> +	gpio: gpio at 40028000 {
>> +		compatible = "nxp,lpc32xx-gpio";
>> +		reg = <0x40028000 0x1000>;
>> +		/* create a private address space for enumeration */
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		gpio_p0: gpio-bank at 0 {
>> +			gpio-controller;
>> +			#gpio-cells = <2>;
>> +			reg = <0>;
>> +		};
>> +
>> +		gpio_p1: gpio-bank at 1 {
>> +			gpio-controller;
>> +			#gpio-cells = <2>;
>> +			reg = <1>;
>> +		};
>> +
>> +		gpio_p2: gpio-bank at 2 {
>> +			gpio-controller;
>> +			#gpio-cells = <2>;
>> +			reg = <2>;
>> +		};
>> +
>> +		gpio_p3: gpio-bank at 3 {
>> +			gpio-controller;
>> +			#gpio-cells = <2>;
>> +			reg = <3>;
>> +		};
>> +
>> +		gpi_p3: gpio-bank at 4 {
>> +			gpio-controller;
>> +			#gpio-cells = <2>;
>> +			reg = <4>;
>> +		};
>> +
>> +		gpo_p3: gpio-bank at 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,55 @@ 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 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) {
>> +			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);
>> +	}
>> +
>> +	return 0;
>>  }
>> +
>> +#ifdef CONFIG_OF
>> +static struct of_device_id lpc32xx_gpio_of_match[] __devinitdata = {
>> +	{ .compatible = "nxp,lpc32xx-gpio", },
>> +	{ },
>> +};
>> +#endif
>> +
>> +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,
>> +};
>> +
>> +module_platform_driver(lpc32xx_gpio_driver);
> 

[PATCH v2] gpio: Device tree support for LPC32xx

[Roland Stigge]

On 07/04/12 15:50, Roland Stigge wrote:
> Hi Grant,
...

> Please consider how the groups are specified in
> drivers/gpio/gpio-lpc32xx.c. They each have different numbers of lines
> and GPIO / GPI / GPO functionality. So they also have different callback
> sets, and we need to do separate gpiochip_add()s which leads to the
> separate gpio-bank specifications in the dtsi file. Separate enabling of
> those banks via OF are a nice by-product.

And BTW: This also corresponds nicely with how the LPC32xx hardware
calls its GPIOs, e.g. GPO 3 for GPO #3 in the gpo group.

Thanks for considering,

Roland

[PATCH v2] gpio: Device tree support for LPC32xx

[jonsmirl at gmail.com]

On Sat, Apr 7, 2012 at 9:50 AM, Roland Stigge <stigge@antcom.de> wrote:
> Hi Grant,
>
> thanks for your suggestions about LPC32xx GPIO DT.
>
> Regarding the numbering:
>
> On 07/04/12 05:53, Grant Likely wrote:
>>> +Required properties:
>>> +- compatible: "nxp,lpc32xx-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
>>
>> If these are merely contiguous register banks of 32 gpio lines, then
>> established convention is pretty much to only use one node and make
>> the translate function decode bank and bit out of the gpio specifier.
>> There isn't a whole lot of value it having all the sub nodes when
>> there isn't anything significantly different between them.
>
> Please consider how the groups are specified in
> drivers/gpio/gpio-lpc32xx.c. They each have different numbers of lines
> and GPIO / GPI / GPO functionality. So they also have different callback
> sets, and we need to do separate gpiochip_add()s which leads to the
> separate gpio-bank specifications in the dtsi file. Separate enabling of
> those banks via OF are a nice by-product.

When you have six banks of 32b registers with sparse, active GPIOs in
the banks, is there any advantage to saying bank one has 8 gpios, bank
2 has 14, bank 3 has 2, etc in the gpiochip?  Or just just register
them as six banks of 32 GPIOs without indicating which are
valid/invalid?   Maybe add a way for the DT to indicate the sparse map
of valid GPIOs?

We're introducing a lot of complexity around invalid GPIO numbers that
shouldn't matter in a working system.

LPC31xx is even worse. I have 12 banks of sparse GPIOs.

>
> So I would like to keep it that way.
>
> What do you think?
>
> Thanks in advance,
>
> Roland
>
>>> + ? ?gpio: gpio at 40028000 {
>>> + ? ? ? ? ? ?compatible = "nxp,lpc32xx-gpio";
>>> + ? ? ? ? ? ?reg = <0x40028000 0x1000>;
>>> + ? ? ? ? ? ?/* create a private address space for enumeration */
>>> + ? ? ? ? ? ?#address-cells = <1>;
>>> + ? ? ? ? ? ?#size-cells = <0>;
>>> +
>>> + ? ? ? ? ? ?gpio_p0: gpio-bank at 0 {
>>> + ? ? ? ? ? ? ? ? ? ?gpio-controller;
>>> + ? ? ? ? ? ? ? ? ? ?#gpio-cells = <2>;
>>> + ? ? ? ? ? ? ? ? ? ?reg = <0>;
>>> + ? ? ? ? ? ?};
>>> +
>>> + ? ? ? ? ? ?gpio_p1: gpio-bank at 1 {
>>> + ? ? ? ? ? ? ? ? ? ?gpio-controller;
>>> + ? ? ? ? ? ? ? ? ? ?#gpio-cells = <2>;
>>> + ? ? ? ? ? ? ? ? ? ?reg = <1>;
>>> + ? ? ? ? ? ?};
>>> +
>>> + ? ? ? ? ? ?gpio_p2: gpio-bank at 2 {
>>> + ? ? ? ? ? ? ? ? ? ?gpio-controller;
>>> + ? ? ? ? ? ? ? ? ? ?#gpio-cells = <2>;
>>> + ? ? ? ? ? ? ? ? ? ?reg = <2>;
>>> + ? ? ? ? ? ?};
>>> +
>>> + ? ? ? ? ? ?gpio_p3: gpio-bank at 3 {
>>> + ? ? ? ? ? ? ? ? ? ?gpio-controller;
>>> + ? ? ? ? ? ? ? ? ? ?#gpio-cells = <2>;
>>> + ? ? ? ? ? ? ? ? ? ?reg = <3>;
>>> + ? ? ? ? ? ?};
>>> +
>>> + ? ? ? ? ? ?gpi_p3: gpio-bank at 4 {
>>> + ? ? ? ? ? ? ? ? ? ?gpio-controller;
>>> + ? ? ? ? ? ? ? ? ? ?#gpio-cells = <2>;
>>> + ? ? ? ? ? ? ? ? ? ?reg = <4>;
>>> + ? ? ? ? ? ?};
>>> +
>>> + ? ? ? ? ? ?gpo_p3: gpio-bank at 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,55 @@ 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 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) {
>>> + ? ? ? ? ? ? ? ? ? ?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);
>>> + ? ?}
>>> +
>>> + ? ?return 0;
>>> ?}
>>> +
>>> +#ifdef CONFIG_OF
>>> +static struct of_device_id lpc32xx_gpio_of_match[] __devinitdata = {
>>> + ? ?{ .compatible = "nxp,lpc32xx-gpio", },
>>> + ? ?{ },
>>> +};
>>> +#endif
>>> +
>>> +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,
>>> +};
>>> +
>>> +module_platform_driver(lpc32xx_gpio_driver);
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/



-- 
Jon Smirl
jonsmirl at gmail.com

[PATCH v2] gpio: Device tree support for LPC32xx

[Roland Stigge]

On 07/04/12 19:17, jonsmirl at gmail.com wrote:
>> Please consider how the groups are specified in
>> drivers/gpio/gpio-lpc32xx.c. They each have different numbers of lines
>> and GPIO / GPI / GPO functionality. So they also have different callback
>> sets, and we need to do separate gpiochip_add()s which leads to the
>> separate gpio-bank specifications in the dtsi file. Separate enabling of
>> those banks via OF are a nice by-product.
> 
> When you have six banks of 32b registers with sparse, active GPIOs in
> the banks, is there any advantage to saying bank one has 8 gpios, bank
> 2 has 14, bank 3 has 2, etc in the gpiochip?  Or just just register
> them as six banks of 32 GPIOs without indicating which are
> valid/invalid?

I think it is a good idea to only register GPIOs that are real when we
already have many (~100) instead of registering hundreds of invalid
extra ones (that will _always_ be invalid for this very driver).

Also corresponds nicely to the hardware specs when there are banks of
odd sizes which can be found registered accordingly in the kernel.

Roland

[PATCH v2] gpio: Device tree support for LPC32xx

[jonsmirl at gmail.com]

On Sat, Apr 7, 2012 at 1:45 PM, Roland Stigge <stigge@antcom.de> wrote:
> On 07/04/12 19:17, jonsmirl at gmail.com wrote:
>>> Please consider how the groups are specified in
>>> drivers/gpio/gpio-lpc32xx.c. They each have different numbers of lines
>>> and GPIO / GPI / GPO functionality. So they also have different callback
>>> sets, and we need to do separate gpiochip_add()s which leads to the
>>> separate gpio-bank specifications in the dtsi file. Separate enabling of
>>> those banks via OF are a nice by-product.
>>
>> When you have six banks of 32b registers with sparse, active GPIOs in
>> the banks, is there any advantage to saying bank one has 8 gpios, bank
>> 2 has 14, bank 3 has 2, etc in the gpiochip? ?Or just just register
>> them as six banks of 32 GPIOs without indicating which are
>> valid/invalid?
>
> I think it is a good idea to only register GPIOs that are real when we
> already have many (~100) instead of registering hundreds of invalid
> extra ones (that will _always_ be invalid for this very driver).

I only want to apply this to identical controller banks. For example
the lpc31xx has 12 identical banks of 32 bits.  You can program all
384 of these locations identically, but only about 100 locations are
actually hooked to real hardware.

We could add something like this to the device tree for gpio-controller:
gpios-valid = <0x1234 0x1234 0x1234 0x1234 0x1234 0x1234 0x1234 0x1234 ...>
12 groups of bits indicating which gpios are valid.

Or we could just ignore the invalid bits. On the lpc31xx they all
work, just just don't do anything.

>
> Also corresponds nicely to the hardware specs when there are banks of
> odd sizes which can be found registered accordingly in the kernel.

I agree that if the structure of the GPIO banks is irregular they
should be different gpiochips.

>
> Roland



-- 
Jon Smirl
jonsmirl at gmail.com