Re: [PATCH 3/3] dt-bindings: pinctrl: at91-pio4: convert Atmel's PIO4 bindings to json-schema

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

On 29/02/2024 12:39, Balakrishnan Sambath wrote:
> Convert the existing text DT bindings of Atmel's PIO4 pincontroller to
> yaml based DT schema.
> 
> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@xxxxxxxxxxxxx>
> ---

Dependency shall be noted here, with lore link.

>  .../bindings/pinctrl/atmel,at91-pio4-pinctrl.txt   |  98 ---------------
>  .../bindings/pinctrl/atmel,sama5d2-pinctrl.yaml    | 140 +++++++++++++++++++++
>  2 files changed, 140 insertions(+), 98 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pio4-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/atmel,at91-pio4-pinctrl.txt
> deleted file mode 100644
> index 774c3c269c40..000000000000
> --- a/Documentation/devicetree/bindings/pinctrl/atmel,at91-pio4-pinctrl.txt
> +++ /dev/null
> @@ -1,98 +0,0 @@
> -* Atmel PIO4 Controller

...

> -...
> diff --git a/Documentation/devicetree/bindings/pinctrl/atmel,sama5d2-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/atmel,sama5d2-pinctrl.yaml
> new file mode 100644
> index 000000000000..8a2dee1d6dd3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/atmel,sama5d2-pinctrl.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/atmel,sama5d2-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip PIO4 Controller
> +
> +maintainers:
> +  - Balakrishnan Sambath <balakrishnan.s@xxxxxxxxxxxxx>
> +
> +description:
> +  The Microchip PIO4 controller is used to select the function of a pin and to
> +  configure it.
> +
> +

One blank line only.

> +properties:
> +  compatible:
> +    enum:
> +      - microchip,sama7g5-pinctrl
> +      - atmel,sama5d2-pinctrl

Keep them alphabetically ordered.

> +
> +  reg:
> +    minItems: 1
> +    maxItems: 2

You need to describe items instead. And why is this flexible?

> +
> +  interrupts:
> +    description:
> +      Interrupt outputs from the controller, one for each bank.

maxItems

> +
> +  interrupt-controller: true
> +
> +  '#interrupt-cells':
> +    const: 2
> +
> +  gpio-controller: true
> +
> +  '#gpio-cells':
> +    const: 2
> +
> +  clocks:
> +    maxItems: 1

Missing blank line.


> +if:

Missing allOf: and then put it how it is in example schema.

> +  properties:
> +    compatible:
> +      contains:
> +        const: microchip,sama7g5-pinctrl
> +then:
> +  patternProperties:
> +    '^.*([-_]default)?$':

No, properties must be defined in top level, not in if:then:. Also
underscore should not be allowed.

> +      anyOf:
> +        - $ref: "#/$defs/mchp-pio4-pincfg-node-1"
> +        - patternProperties:
> +            '^[a-z_-][a-z_-]*$':

Both regexes are way too permissive. Look how other bindings do it.
Usually these are -pins or -group.

> +              $ref: "#/$defs/mchp-pio4-pincfg-node-1"
> +else:
> +  patternProperties:
> +    '^.*([-_]default)?$':
> +      anyOf:
> +        - $ref: "#/$defs/mchp-pio4-pincfg-node-2"
> +        - patternProperties:
> +            '^[a-z_-][a-z_-]*$':
> +              $ref: "#/$defs/mchp-pio4-pincfg-node-2"
> +
> +$defs:
> +  mchp-pio4-pincfg-node-1:
> +    $ref: pincfg-node.yaml#properties
> +    properties:
> +      pinmux:
> +        $ref: pinmux-node.yaml#/properties/pinmux
> +      atmel,drive-strength:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 1, 2, 3]
> +        default: 0
> +    required:
> +      - pinmux
> +
> +  mchp-pio4-pincfg-node-2:
> +    $ref: pincfg-node.yaml#properties
> +    properties:
> +      pinmux:
> +        $ref: pinmux-node.yaml#/properties/pinmux
> +    required:
> +      - pinmux
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - interrupt-controller
> +  - '#interrupt-cells'
> +  - gpio-controller
> +  - '#gpio-cells'
> +
> +unevaluatedProperties: false

Nope, this must be additionalProperties: false. If you put here
unevaluated, it's a sign you define properties not in correct spot.

> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/clock/at91.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/pinctrl/sama5d2-pinfunc.h>
> +
> +    pinctrl@fc038000 {
> +        compatible = "atmel,sama5d2-pinctrl";
> +        reg = <0xfc038000 0x600>;
> +        interrupts = <18 IRQ_TYPE_LEVEL_HIGH 7>,
> +                     <68 IRQ_TYPE_LEVEL_HIGH 7>,
> +                     <69 IRQ_TYPE_LEVEL_HIGH 7>,
> +                     <70 IRQ_TYPE_LEVEL_HIGH 7>;
> +        interrupt-controller;
> +        #interrupt-cells = <2>;
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +        clocks = <&pioA_clk>;
> +
> +        pinctrl_i2c0_default: i2c0_default {

Underscores are not allowed. Please open DTS coding style. Or test your
DTS with W=2.



Best regards,
Krzysztof