Re: [PATCH v3 1/2] dt-bindings: gpio: gpio-mvebu: convert txt binding to YAML

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 12/05/2022 11:41, Chris Packham wrote:
> Convert the existing device tree binding to YAML format.
> 
> The old binding listed the interrupt-controller and related properties
> as required but there are sufficiently many existing usages without it
> that the YAML binding does not make the interrupt properties required.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> ---
> 
> Notes:
>     Changes in v3:
>     - Correct indent in example
>     - Move offset and marvell,pwm-offset to separate patch
>     - Correct some documentation cross references

Thank you for your patch. There is something to discuss/improve.

> diff --git a/Documentation/devicetree/bindings/gpio/gpio-mvebu.yaml b/Documentation/devicetree/bindings/gpio/gpio-mvebu.yaml
> new file mode 100644
> index 000000000000..2d95ef707f53
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-mvebu.yaml
> @@ -0,0 +1,143 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/gpio-mvebu.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell EBU GPIO controller
> +
> +maintainers:
> +  - Thierry Reding <thierry.reding@gmail.com>
> +  - Lee Jones <lee.jones@linaro.org>

These should be rather platform or driver maintainers, not subsystem
folks. Unless it happens that Thierry and Lee are for platform?

> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - enum:
> +          - marvell,armada-8k-gpio
> +          - marvell,orion-gpio
> +
> +      - items:
> +          - enum:
> +              - marvell,mv78200-gpio
> +              - marvell,armada-370-gpio
> +              - marvell,armadaxp-gpio
> +          - const: marvell,orion-gpio
> +
> +  reg:
> +    description: |
> +      Address and length of the register set for the device. Not used for
> +      marvell,armada-8k-gpio.
> +
> +      For the "marvell,armadaxp-gpio" variant a second entry is expected for
> +      the per-cpu registers. For other variants second entry can be provided,
> +      for the PWM function using the GPIO Blink Counter on/off registers.
> +    minItems: 1
> +    maxItems: 2
> +
> +  reg-names:
> +    items:
> +      - const: gpio
> +      - const: pwm
> +    minItems: 1
> +
> +  interrupts:
> +    description: |
> +      The list of interrupts that are used for all the pins managed by this
> +      GPIO bank. There can be more than one interrupt (example: 1 interrupt
> +      per 8 pins on Armada XP, which means 4 interrupts per bank of 32
> +      GPIOs).
> +    minItems: 1
> +    maxItems: 4
> +
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 2
> +
> +  gpio-controller: true
> +
> +  ngpios:
> +    minimum: 1
> +    maximum: 32
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +  "#pwm-cells":
> +    description:
> +      The first cell is the GPIO line number. The second cell is the period
> +      in nanoseconds.
> +    const: 2
> +
> +  clocks:
> +    description:
> +      Clock(s) used for PWM function.
> +    items:
> +      - description: Core clock
> +      - description: AXI bus clock
> +    minItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: core
> +      - const: axi
> +    minItems: 1
> +
> +required:
> +  - compatible
> +  - gpio-controller
> +  - ngpios
> +  - "#gpio-cells"
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: marvell,armada-8k-gpio
> +    then:
> +      required:
> +        - offset
> +    else:
> +      required:
> +        - reg

one blank line please

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: marvell,armadaxp-gpio

Original bindings are saying that second reg is optional for
marvell,armada-370-gpio. What about other cases, e.g. mv78200-gpio? Is
it also allowed (and optional) there?

> +    then:
> +      properties:
> +        reg:
> +          minItems: 2

Then you also should require two reg-names.


Best regards,
Krzysztof