Hi Rob,

El 10 mar 2021, a las 18:45, Rob Herring [off-list ref] escribió:

On Wed, Mar 10, 2021 at 5:55 AM Álvaro Fernández Rojas
[off-list ref] wrote:

quoted

Add binding documentation for the pincontrol core found in BCM6328 SoCs.

Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
Co-developed-by: Jonas Gorski <jonas.gorski@gmail.com>
Signed-off-by: Álvaro Fernández Rojas <redacted>
---
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@gmail.com>
+  - Jonas Gorski <jonas.gorski@gmail.com>
+
+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...

quoted

+      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>;

		…
	};
};

quoted

+          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.