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 ¯\_(ツ)_/¯ 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? Something like: syscon { pinctrl: pinctrl { compatible … gpio-controller; gpio-ranges = <&pinctrl 0 0 32>; #gpio-cells = <2>; … }; }; > >> + 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. > > Rob Best regards, Álvaro.
