[v5,1/3] dt-bindings: pinctrl: Add bindings for pinctrl-microchip-sgpio driver

This adds DT bindings for the Microsemi/Microchip SGPIO controller,
bindings microchip,sparx5-sgpio, mscc,ocelot-sgpio and
mscc,luton-sgpio.

Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
---
 .../pinctrl/microchip,sparx5-sgpio.yaml       | 140 ++++++++++++++++++
 1 file changed, 140 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml

--
2.25.1

Hi Lars!

This is overall looking fine. Except for the 3 cell business. I just can't
wrap my head around why that is needed.

On Thu, Oct 8, 2020 at 3:05 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:

> +      '#gpio-cells':

> +        const: 3


So at the very least needs a description making it crystal clear why each
cell is needed, and used for since the standard bindings are not used.

+      sgpio_in2: gpio@0 {
+        reg = <0>;
+        compatible = "microchip,sparx5-sgpio-bank";
+        gpio-controller;
+        #gpio-cells = <3>;
+        ngpios = <96>;
+      };

So here reg = 0 and the out port has reg 1. Isn't that what you also put
in the second cell of the GPIO phandle? Then why? The driver
can very well just parse its own reg property and fill that in.

When you obtain a phandle like that:

gpios = <&sgpio_in2 11 0 GPIO_OUT_LOW>;

Isn't that 0 just duplicating the "reg"? Just parse reg when you set up
your driver state and put it as variable in the state container for your
driver state for this particular gpio_chip. No need to get it from
the phandle.

Yours,
Linus Walleij

On Thu, Oct 08, 2020 at 03:05:13PM +0200, Lars Povlsen wrote:
> This adds DT bindings for the Microsemi/Microchip SGPIO controller,
> bindings microchip,sparx5-sgpio, mscc,ocelot-sgpio and
> mscc,luton-sgpio.
> 
> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
> ---
>  .../pinctrl/microchip,sparx5-sgpio.yaml       | 140 ++++++++++++++++++
>  1 file changed, 140 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml b/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml
> new file mode 100644
> index 000000000000..fc41495800ed
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/microchip,sparx5-sgpio.yaml
> @@ -0,0 +1,140 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/microchip,sparx5-sgpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microsemi/Microchip Serial GPIO controller
> +
> +maintainers:
> +  - Lars Povlsen <lars.povlsen@microchip.com>
> +
> +description: |
> +  By using a serial interface, the SIO controller significantly extend
> +  the number of available GPIOs with a minimum number of additional
> +  pins on the device. The primary purpose of the SIO controllers is to
> +  connect control signals from SFP modules and to act as an LED
> +  controller.
> +
> +properties:
> +  $nodename:
> +    pattern: "^gpio@[0-9a-f]+$"
> +
> +  compatible:
> +    enum:
> +      - microchip,sparx5-sgpio
> +      - mscc,ocelot-sgpio
> +      - mscc,luton-sgpio
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  microchip,sgpio-port-ranges:
> +    description: This is a sequence of tuples, defining intervals of
> +      enabled ports in the serial input stream. The enabled ports must
> +      match the hardware configuration in order for signals to be
> +      properly written/read to/from the controller holding
> +      registers. Being tuples, then number of arguments must be
> +      even. The tuples mast be ordered (low, high) and are
> +      inclusive.
> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +    items:
> +      items:
> +        - description: |
> +            "low" indicates start bit number of range
> +          minimum: 0
> +          maximum: 31
> +        - description: |
> +            "high" indicates end bit number of range
> +          minimum: 0
> +          maximum: 31
> +    minItems: 1
> +    maxItems: 32
> +
> +  microchip,sgpio-frequency:
> +    description: The sgpio controller frequency (Hz). This dictates
> +      the serial bitstream speed, which again affects the latency in
> +      getting control signals back and forth between external shift
> +      registers. The speed must be no larger than half the system
> +      clock, and larger than zero.
> +    $ref: /schemas/types.yaml#/definitions/uint32

Perhaps use 'bus-frequency' here. If not, needs unit suffix. Either way, 
you can drop the type in those cases. 

With that,

Reviewed-by: Rob Herring <robh@kernel.org>

> +    minimum: 1
> +    default: 12500000
> +
> +patternProperties:
> +  "^gpio@[0-1]$":
> +    type: object
> +    properties:
> +      compatible:
> +        const: microchip,sparx5-sgpio-bank
> +
> +      reg:
> +        description: |
> +          The GPIO bank number. "0" is designates the input pin bank,
> +          "1" the output bank.
> +        maxItems: 1
> +
> +      gpio-controller: true
> +
> +      '#gpio-cells':
> +        const: 3
> +
> +      ngpios:
> +        minimum: 1
> +        maximum: 128
> +
> +    required:
> +      - compatible
> +      - reg
> +      - gpio-controller
> +      - '#gpio-cells'
> +      - ngpios
> +
> +    additionalProperties: false
> +
> +additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - microchip,sgpio-port-ranges
> +  - "#address-cells"
> +  - "#size-cells"
> +
> +examples:
> +  - |
> +    sgpio2: gpio@1101059c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      compatible = "microchip,sparx5-sgpio";
> +      clocks = <&sys_clk>;
> +      pinctrl-0 = <&sgpio2_pins>;
> +      pinctrl-names = "default";
> +      reg = <0x1101059c 0x100>;
> +      microchip,sgpio-port-ranges = <0 0>, <16 18>, <28 31>;
> +      microchip,sgpio-frequency = <25000000>;
> +      sgpio_in2: gpio@0 {
> +        reg = <0>;
> +        compatible = "microchip,sparx5-sgpio-bank";
> +        gpio-controller;
> +        #gpio-cells = <3>;
> +        ngpios = <96>;
> +      };
> +      sgpio_out2: gpio@1 {
> +        compatible = "microchip,sparx5-sgpio-bank";
> +        reg = <1>;
> +        gpio-controller;
> +        #gpio-cells = <3>;
> +        ngpios = <96>;
> +      };
> +    };
> --
> 2.25.1