On Thu, Jul 16, 2020 at 08:50:36AM -0600, Rob Herring wrote:

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

quoted

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:

Technically all the registers used by the driver could be specified via
a single contiguous range. However, there are a few unrelated registers
(i.e. PWM Output Control) which should be excluded in order to come up
with a more accurate specification. The 4 ranges below are basically
the result of this exclusion:

quoted

      * 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?

No, the h/w is fixed, the only reason of this approach was to allow a
precise memory region specification, as explained above.

I'm not sure if this should be made mandatory or it's also fine to let
(a lazy) user provide combined ranges or just a contiguous one (like
in the example), with the drawback of loosing the accuracy, of course.

If they do stay, then you want:

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

Would this be applicable even if we keep this flexible approach and
don't set 'minItems: 4'?

quoted

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.

Fair enough, I have already updated the node names to use the '-pins'
suffix.

quoted

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?)

Actually the pattern should be '^(.*-)?pinconf$', to restrict the names
to either 'pinconf' or '<label>-pinconf'.

I have just made some more validation tests and noticed I had missed an
'additionalProperties: false' line, for the 'pins' node. Should be fine
now!

Thanks,
Cristi


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel