Re: [PATCH v1 25/30] dt-bindings: pinctrl: Add StarFive JH7110 pinctrl bindings

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

On 30/09/2022 09:38, Hal Feng wrote:
> From: Jianlong Huang <jianlong.huang@xxxxxxxxxxxxxxxx>
> 
> Add pinctrl bindings for StarFive JH7110 SoC.
> 
> Signed-off-by: Jianlong Huang <jianlong.huang@xxxxxxxxxxxxxxxx>
> Signed-off-by: Hal Feng <hal.feng@xxxxxxxxxxxxxxxxxxxxxx>
> ---
>  .../pinctrl/starfive,jh7110-pinctrl.yaml      | 202 ++++++++++++++++++
>  1 file changed, 202 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/starfive,jh7110-pinctrl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/starfive,jh7110-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/starfive,jh7110-pinctrl.yaml
> new file mode 100644
> index 000000000000..482012ad8a14
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/starfive,jh7110-pinctrl.yaml
> @@ -0,0 +1,202 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/starfive,jh7110-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive JH7110 Pin Controller Device Tree Bindings
> +
> +description: |
> +  Bindings for the JH7110 RISC-V SoC from StarFive Technology Ltd.
> +
> +maintainers:
> +  - Jianlong Huang <jianlong.huang@xxxxxxxxxxxxxxxx>
> +
> +properties:
> +  compatible:
> +    enum:
> +    - starfive,jh7110-sys-pinctrl
> +    - starfive,jh7110-aon-pinctrl

Wrong indentation.

Does not look like you tested the bindings. Please run `make
dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).

> +
> +  reg:
> +    minItems: 2

No need.

> +    maxItems: 2
> +
> +  reg-names:
> +    items:
> +      - const: control

This does not match reg at all. Again - not tested.


> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +  interrupts:
> +    maxItems: 1
> +    description: The GPIO parent interrupt.
> +
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 2
> +
> +  ngpios:
> +    enum:
> +    - 64
> +    - 4

Wrong indentation. Increasing order.

> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - clocks
> +  - "#gpio-cells"
> +  - interrupts
> +  - interrupt-controller
> +  - "#interrupt-cells"
> +
> +patternProperties:

This goes before required block

> +  '-[0-9]+$':

Too loose pattern. Need some more specific pattern. What do you exactly
match here?

Missing description.

> +    type: object
> +    patternProperties:
> +      '-pins$':
> +        type: object
> +        description: |
> +          A pinctrl node should contain at least one subnode representing the
> +          pinctrl groups available on the machine. Each subnode will list the
> +          pins it needs, and how they should be configured, with regard to
> +          muxer configuration, system signal configuration, pin groups for
> +          vin/vout module, pin voltage, mux functions for output, mux functions
> +          for output enable, mux functions for input.
> +
> +        properties:
> +          starfive,pins:

No, use generic pinctrl bindings.

> +            description: |
> +              The list of pin identifiers that properties in the node apply to.
> +              This should be set using the PAD_GPIOX macros.
> +              This has to be specified.
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            minimum: 0
> +            maximum: 63
> +
> +          starfive,pinmux:

No, use generic pinctrl bindings.


> +            description: |
> +              The list of GPIOs and their mux functions that properties in the
> +              node apply to. This should be set using the PAD_GPIOX_FUNC_SEL
> +              macro with its value.
> +              This is optional for some pins.
> +              The value of PAD_GPIOX_FUNC_SEL macro can selects:
> +                0: GPIOX mux function 0,
> +                1: GPIOX mux function 1,
> +                2: GPIOX mux function 2.
> +
> +          starfive,pin-ioconfig:
> +            description: |
> +              This is used to configure the core settings of system signals.
> +              The combination of GPIO_IE or GPIO_DS or GPIO_PU or GPIO_PD or
> +              GPIO_SLEW or GPIO_SMT or GPIO_POS.
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +
> +          starfive,padmux:
> +            description: |
> +              The padmux is for vin/vout module to select pin groups.
> +              0: vout will be set at pins from PAD_GPIO7 to PAD_GPIO34,
> +                 when PAD_GPIOX_FUNC_SEL is set as 1.
> +                 vin will be set at pins from PAD_GPIO6 to PAD_GPIO20.
> +                 when PAD_GPIOX_FUNC_SEL is set as 2.
> +              1: vout will be set at pins from PAD_GPIO36 to PAD_GPIO63,
> +                 when PAD_GPIOX_FUNC_SEL is set as 1.
> +                 vin will be set at pins from PAD_GPIO21 to PAD_GPIO35.
> +                 when PAD_GPIOX_FUNC_SEL is set as 2.
> +              2: vin will be set at pins from PAD_GPIO36 to PAD_GPIO50,
> +                 when PAD_GPIOX_FUNC_SEL is set as 2
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            enum: [0, 1, 2]
> +
> +          starfive,pin-syscon:
> +            description: |
> +              This is used to set pin voltage,
> +              0: 3.3V, 1: 2.5V, 2: 1.8V.
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            enum: [0, 1, 2]
> +
> +          starfive,pin-gpio-dout:

No, use generic pinctrl bindings.


> +            description: |
> +              This is used to set their mux functions for output.
> +              This should be set using the GPO_XXX macro,
> +              such as GPO_LOW, GPO_UART0_SOUT.
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            minimum: 0
> +            maximum: 107
> +
> +          starfive,pin-gpio-doen:

No, use generic pinctrl bindings.


> +            description: |
> +              This is used to set their mux functions for output enable.
> +              This should be set using the OEN_XXX macro,
> +              such as OEN_LOW, OEN_I2C0_IC_CLK_OE.
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            minimum: 0
> +            maximum: 49
> +
> +          starfive,pin-gpio-din:

No, use generic pinctrl bindings.

> +            description: |
> +              This is used to set their mux functions for input.
> +              This should be set using the GPI_XXX macro,
> +              such as GPI_CAN0_CTRL_RXD, GPI_I2C0_IC_CLK_IN_A.
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            minimum: 0
> +            maximum: 90
> +
> +        additionalProperties: false
> +
> +    additionalProperties: false
> +
> +additionalProperties: false

Missing allof to generic pinctrl bindings.

> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/starfive-jh7110-sys.h>
> +    #include <dt-bindings/reset/starfive-jh7110.h>
> +    #include <dt-bindings/pinctrl/pinctrl-starfive-jh7110.h>
> +
> +    gpio: gpio@13040000 {
> +      compatible = "starfive,jh7110-sys-pinctrl";
> +      reg = <0x0 0x13040000 0x0 0x10000>;
> +      reg-names = "control";
> +      clocks = <&clkgen JH7110_SYS_IOMUX_PCLK>;
> +      resets = <&rstgen RSTN_U0_SYS_IOMUX_PRESETN>;
> +      interrupts = <86>;
> +      interrupt-controller;
> +      #gpio-cells = <2>;
> +      ngpios = <64>;
> +      status = "okay";
> +
> +      uart0_pins: uart0-pins {

Explain me please how it can possible match your pattern: '-[0-9]+$':

You really wrote something which was not tested and cannot work.
> +        uart0-pins-tx {
> +          starfive,pins = <PAD_GPIO5>;
> +          starfive,pin-ioconfig = <IO(GPIO_IE(1) | GPIO_DS(3))>;
> +          starfive,pin-gpio-dout = <GPO_UART0_SOUT>;
> +          starfive,pin-gpio-doen = <OEN_LOW>;
> +        };
> +
> +        uart0-pins-rx {
> +          starfive,pins = <PAD_GPIO6>;
> +          starfive,pinmux = <PAD_GPIO6_FUNC_SEL 0>;
> +          starfive,pin-ioconfig = <IO(GPIO_IE(1) | GPIO_PU(1))>;
> +          starfive,pin-gpio-doen = <OEN_HIGH>;
> +          starfive,pin-gpio-din =  <GPI_UART0_SIN>;
> +        };
> +      };
> +    };
> +
> +    &uart0 {
> +      pinctrl-names = "default";
> +      pinctrl-0 = <&uart0_pins>;
> +      status = "okay";

Drop, obvious.

> +    };
> +
> +...

Best regards,
Krzysztof