On Wed, Mar 10, 2021 at 11:03 AM Álvaro Fernández Rojas <noltari@xxxxxxxxx> wrote: > > Hi Rob, > > > El 10 mar 2021, a las 18:45, Rob Herring <robh+dt@xxxxxxxxxx> escribió: > > > > On Wed, Mar 10, 2021 at 5:55 AM Álvaro Fernández Rojas > > <noltari@xxxxxxxxx> wrote: > >> > >> Add binding documentation for the pincontrol core found in BCM6328 SoCs. > >> > >> Signed-off-by: Jonas Gorski <jonas.gorski@xxxxxxxxx> > >> Co-developed-by: Jonas Gorski <jonas.gorski@xxxxxxxxx> > >> Signed-off-by: Álvaro Fernández Rojas <noltari@xxxxxxxxx> > >> --- > >> v6: add changes suggested by Rob Herring > >> v5: change Documentation to dt-bindings in commit title > >> v4: no changes > >> v3: add new gpio node > >> v2: remove interrupts > >> > >> .../pinctrl/brcm,bcm6328-pinctrl.yaml | 174 ++++++++++++++++++ > >> 1 file changed, 174 insertions(+) > >> create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml > >> > >> diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml > >> new file mode 100644 > >> index 000000000000..471f6efa1754 > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml > >> @@ -0,0 +1,174 @@ > >> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > >> +%YAML 1.2 > >> +--- > >> +$id: http://devicetree.org/schemas/pinctrl/brcm,bcm6328-pinctrl.yaml# > >> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >> + > >> +title: Broadcom BCM6328 pin controller > >> + > >> +maintainers: > >> + - Álvaro Fernández Rojas <noltari@xxxxxxxxx> > >> + - Jonas Gorski <jonas.gorski@xxxxxxxxx> > >> + > >> +description: |+ > >> + The pin controller node should be the child of a syscon node. > >> + > >> + Refer to the the bindings described in > >> + Documentation/devicetree/bindings/mfd/syscon.yaml > >> + > >> +properties: > >> + compatible: > >> + const: brcm,bcm6328-pinctrl > >> + > >> + gpio: > >> + type: object > >> + properties: > >> + compatible: > >> + const: brcm,bcm6328-gpio > >> + > >> + data: > >> + $ref: /schemas/types.yaml#/definitions/uint32 > >> + description: | > >> + Offset in the register map for the data register (in bytes). > >> + > >> + dirout: > >> + $ref: /schemas/types.yaml#/definitions/uint32 > >> + description: | > >> + Offset in the register map for the dirout register (in bytes). > >> + > >> + gpio-controller: true > >> + > >> + "#gpio-cells": > >> + const: 2 > >> + > >> + gpio-ranges: > >> + maxItems: 1 > >> + > >> + required: > >> + - gpio-controller > >> + - gpio-ranges > >> + - '#gpio-cells' > >> + > >> + additionalProperties: false > >> + > >> +patternProperties: > >> + '^.*-pins$': > >> + if: > >> + type: object > >> + then: > >> + properties: > >> + function: > >> + $ref: "pinmux-node.yaml#/properties/function" > >> + enum: [ serial_led_data, serial_led_clk, inet_act_led, pcie_clkreq, > >> + led, ephy0_act_led, ephy1_act_led, ephy2_act_led, > >> + ephy3_act_led, hsspi_cs1, usb_device_port, usb_host_port ] > >> + > >> + pins: > >> + $ref: "pinmux-node.yaml#/properties/pins" > >> + enum: [ gpio6, gpio7, gpio11, gpio16, gpio17, gpio18, gpio19, > >> + gpio20, gpio25, gpio26, gpio27, gpio28, hsspi_cs1, > >> + usb_port1 ] > >> + > >> +required: > >> + - compatible > >> + - gpio > >> + > >> +additionalProperties: false > >> + > >> +examples: > >> + - | > >> + gpio_cntl@10000080 { > >> + compatible = "brcm,bcm6328-gpio-controller", "syscon", "simple-mfd"; > > > > You just added "brcm,bcm6328-gpio-controller", it would need to be documented. > > I just added that because you requested me to do it ¯\_(ツ)_/¯ I said 'syscon' by itself was not allowed, then asked about the multiple levels. > What should I do to document it? > I still don’t get most of this .yaml stuff... > > > > >> + reg = <0x10000080 0x80>; > >> + > >> + pinctrl: pinctrl { > >> + compatible = "brcm,bcm6328-pinctrl"; > >> + > >> + gpio { > >> + compatible = "brcm,bcm6328-gpio"; > > > > I'm still trying to understand why you need 3 levels of nodes here? > > The gpio controller contains a pin controller plus other undefined > > functions (because of 'syscon') and the pin controller contains a gpio > > controller? > > In previous versions the gpio controller was registered along with the pin controller, but @Linus requested me to register the gpio pin controller ranges through device tree by using gpio-ranges and I decided to use this approach, which was already used by other pin controllers. > However, there aren’t any pinctrl drivers using gpio-regmap, so this is kind of new… > > > > > I think "brcm,bcm6328-gpio-controller" and "brcm,bcm6328-pinctrl" > > should be a single node. > > I agree, but does it make sense to add gpio-ranges to a pinctrl node referencing itself? It wouldn't be. I wasn't saying the pinctrl and gpio controller are the same node. My suggestion was combining syscon and pinctrl. > Something like: > syscon { Again with the syscon. If pinctrl and GPIO are the only functions within this h/w block, then this is not a syscon. You are just abusing that having 'syscon' compatible means you get a regmap created automagically for you. Nothing here looks like a 'system controller' to me. A 'system controller' is a random collection of register bits with functions that don't fit anywhere else. > pinctrl: pinctrl { > compatible … > > gpio-controller; > gpio-ranges = <&pinctrl 0 0 32>; > #gpio-cells = <2>; I was assuming you have multiple GPIO controllers within 1 pinctlr? The pinctrl and gpio could be a single node like above if there's only 1 GPIO controller. But I'm still somewhat guessing what the h/w looks like because I have to go searching thru the driver to decipher. Please describe the h/w in the binding. If there's more than 1 GPIO controller, then I'd imagine you have something like this: pinctrl { ... reg = <base 0x80>; ranges = <0 base 0x80; gpio@4 { reg = <4 4>, <c 4>; reg-names = "dirout", "dat"; }; gpio@? {}; foo-pins {}; }; > > … > }; > }; > > > > >> + data = <0xc>; > >> + dirout = <0x4>; > > > > This looks similar to the brcm,bcm6345-gpio.txt binding which then > > uses the gpio-mmio driver. Defining addresses with 'reg' is much > > preferred over custom properties. That binding also captures the bank > > size. > > It’s similar, but Linus requested to use gpio regmap because we had a large amount of registers, so we’re not using it. Looks like you have 2 registers to me. Rob
