Date:	Fri, 06 Apr 2012 20:53:27 -0700
From:	Grant Likely <grant.likely@...retlab.ca>
To:	Roland Stigge <stigge@...com.de>, arm@...nel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linus.walleij@...ricsson.com
Cc:	Roland Stigge <stigge@...com.de>
Subject: Re: [PATCH v2] gpio: Device tree support for LPC32xx

On Wed,  4 Apr 2012 01:58:38 +0200, Roland Stigge <stigge@...com.de> wrote:
> This patch adds device tree support for gpio-lpc32xx.c
> 
> Signed-off-by: Roland Stigge <stigge@...com.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@...28000 {
> +		compatible = "nxp,lpc32xx-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,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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists