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

Hi Rob,

quoted

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 ¯\_(ツ)_/¯

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

quoted

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…

quoted

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 {};
};

                …
        };
};

quoted

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.

Looks like you have 2 registers to me.

Rob