Messages in this thread | | | Date | Sat, 07 Apr 2012 15:50:06 +0200 | From | Roland Stigge <> | Subject | Re: [PATCH v2] gpio: Device tree support for LPC32xx |
| |
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@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@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); >
| |