On Thu, Jul 16, 2020 at 4:43 AM Cristian Ciocaltea
[off-list ref] wrote:

On Wed, Jul 15, 2020 at 02:03:09PM -0600, Rob Herring wrote:

quoted

On Thu, Jun 25, 2020 at 11:16:18PM +0300, Cristian Ciocaltea wrote:

quoted

Add pinctrl and gpio bindings for Actions Semi S500 SoC.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
---
 .../pinctrl/actions,s500-pinctrl.yaml         | 228 ++++++++++++++++++
 1 file changed, 228 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/actions,s500-pinctrl.yaml

diff --git a/Documentation/devicetree/bindings/pinctrl/actions,s500-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/actions,s500-pinctrl.yaml
new file mode 100644
index 000000000000..856947c70844
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/actions,s500-pinctrl.yaml

@@ -0,0 +1,228 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/actions,s500-pinctrl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Actions Semi S500 SoC pinmux & GPIO controller
+
+maintainers:
+  - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
+
+description: |
+  Pinmux & GPIO controller manages pin multiplexing & configuration including
+  GPIO function selection & GPIO attributes configuration. Please refer to
+  pinctrl-bindings.txt in this directory for common binding part and usage.
+
+properties:
+  compatible:
+    const: actions,s500-pinctrl
+
+  reg:
+    minItems: 1
+    maxItems: 4

Need to enumerate what each register range is.

Hi Rob,

Thanks for the review!

Would the update below suffice?

  reg:
    description: |
      Specifies the memory region(s) associated with the pin-controller.
      To improve granularity, up to four register ranges can be provided:

What does 'improve granularity' mean:

      * GPIO Output + GPIO Input + GPIO Data
      * Multiplexing Control
      * PAD Pull Control + PAD Schmitt Trigger enable + PAD Control
      * PAD Drive Capacity Select

The h/w sometimes has these and sometimes doesn't?

If they do stay, then you want:

items:
  - description: GPIO Output + GPIO Input + GPIO Data
  - description: ...

quoted

quoted

+
+  clocks:
+    maxItems: 1
+
+  gpio-controller: true
+
+  gpio-ranges:
+    maxItems: 1
+
+  '#gpio-cells':
+    description:
+      Specifies the pin number and flags, as defined in
+      include/dt-bindings/gpio/gpio.h
+    const: 2
+
+  interrupt-controller: true
+
+  '#interrupt-cells':
+    description:
+      Specifies the pin number and flags, as defined in
+      include/dt-bindings/interrupt-controller/irq.h
+    const: 2
+
+  interrupts:
+    description:
+      One interrupt per each of the 5 GPIO ports supported by the controller,
+      sorted by port number ascending order.
+    minItems: 5
+    maxItems: 5
+
+patternProperties:
+  '^.*$':
+    if:
+      type: object

For a new binding, can you do '-pins$' for the node names so we don't
need this if/then hack.

Right, the idea was to be consistent with the existing bindings for
S700 and S900, which allow free node names, although they are not yet
converted to yaml format.

If we want consistency, those should have their node names updated.

quoted

quoted

+    then:
+      patternProperties:
+        'pinmux$':

Is this really a pattern? Can't tell from the example.

pinmux and pinconf subnodes may appear multiple times, that's why I
decided to match their names based on the suffix.

The example is not complex enough, I will change it to the following:

    mmc0_default: mmc0_default {
        pinmux {
            groups = "sd0_d0_mfp", "sd0_d1_mfp", "sd0_d2_d3_mfp",
                     "sd0_cmd_mfp", "sd0_clk_mfp";
            function = "sd0";
        };

        drv_pinconf {

drv-pinconf

Make the pattern '-?pinconf' to enforce that. (that '-' may need escaping?)