Hi Linus, El 11/08/2016 a las 13:45, Linus Walleij escribió: > All devicetree binding patches must be sent to the devicetree@xxxxxxxxxxxxxxx > mailing list so include them on subsequent posts of this patch. > > On Wed, Aug 3, 2016 at 2:05 PM, Álvaro Fernández Rojas > <noltari@xxxxxxxxx> wrote: > >> This patch adds the device tree bindings for the Broadcom's BCM6345 >> memory-mapped GPIO controllers. >> >> The gpios will be supported by gpio-mmio code of the >> GPIO generic library. >> >> Signed-off-by: Álvaro Fernández Rojas <noltari@xxxxxxxxx> >> --- >> v2: add improvements suggested by Jonas Gorski: >> - use native-endian instead of big-endian. > > What is this? Does it come from some other existing binding? > I was feeling native endian is the default unless LE or BE is > explicitly specified... Not really, even if the kernel is configured as big endian, either native-endian or big-endian are needed. The default is little-endian if nothing else is specified: https://github.com/torvalds/linux/blob/master/drivers/of/base.c#L598 > >> +Required properties: >> + - compatible: should be "brcm,bcm6345-gpio" >> + - reg-names: must contain >> + "dat" - data register >> + "dirout" - direction (output) register > > I don't like this and don't understand why you can't just cover > all GPIO registers with a single reg property. Because we want to add a simple gpio driver for these old SoCs with the newer ones having a different and more complex gpio/pinctrl driver. > >> + - reg: address + size pairs describing the GPIO register sets; >> + order must correspond with the order of entries in reg-names >> + - #gpio-cells: must be set to 2. The first cell is the pin number and >> + the second cell is used to specify the gpio polarity: >> + 0 = active high >> + 1 = active low >> + - gpio-controller: Marks the device node as a gpio controller. > > Reference the standard bindings in gpio.txt for cells and controller. Sure, I just copy & pasted this from the other gpio mmio driver: https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/gpio/wd%2Cmbl-gpio.txt#L17 > >> +Optional properties: >> + - native-endian: use native endian memory. > > That is weird. Explain why this is needed. (explained above) > >> + - BCM6345: >> + gpio: gpio-controller@fffe0406 { >> + compatible = "brcm,bcm6345-gpio"; >> + reg-names = "dirout", "dat"; >> + reg = <0xfffe0406 2>, <0xfffe040a 2>; > > Also I do not understand this at all. Why pick out two 16bit registers? > Surely the rest of the registers at 0xfffe0400 must be GPIO registers > as well? Are they not? This is the layout for BCM6345: https://gitlab.labs.nic.cz/turris/openwrt/blob/8bfda76892a39c033b5abf2c17035a444fffad08/target/linux/brcm63xx-2.6/files/include/asm-mips/mach-bcm963xx/6345_map_part.h#L141 And this is the layout for BCM6338: https://gitlab.labs.nic.cz/turris/openwrt/blob/8bfda76892a39c033b5abf2c17035a444fffad08/target/linux/brcm63xx-2.6/files/include/asm-mips/mach-bcm963xx/6338_map_part.h#L202 > > If they are, cover them all with a single reg property. > > If they are not all GPIO registers, use MFD syscon for this and access > the constituent parts through that. I don't think that's the case since we're using a generic mmio binding. > > Yours, > Linus Walleij > Regards, Álvaro. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
