Re: [PATCH] dt-bindings: dma: mv-xor-v2: Convert to dtschema

[Krzysztof Kozlowski <krzk@kernel.org>]

On 23/06/2024 14:45, Shresth Prasad wrote:
> Convert txt bindings of Marvell XOR v2 engines to dtschema to allow
> for validation.
> 
> Signed-off-by: Shresth Prasad <shresthprasad7@gmail.com>
> ---
> Tested against `marvell/armada-7040-db.dtb`, `marvell/armada-7040-mochabin.dtb`
> and `marvell/armada-8080-db.dtb`
> 
>  .../bindings/dma/marvell,xor-v2.yaml          | 69 +++++++++++++++++++
>  .../devicetree/bindings/dma/mv-xor-v2.txt     | 28 --------
>  2 files changed, 69 insertions(+), 28 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/dma/marvell,xor-v2.yaml
>  delete mode 100644 Documentation/devicetree/bindings/dma/mv-xor-v2.txt
> 
> diff --git a/Documentation/devicetree/bindings/dma/marvell,xor-v2.yaml b/Documentation/devicetree/bindings/dma/marvell,xor-v2.yaml
> new file mode 100644
> index 000000000000..3d7481c1917e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/marvell,xor-v2.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/dma/marvell,xor-v2.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell XOR v2 engines
> +
> +maintainers:
> +  - Vinod Koul <vkoul@kernel.org>

Should be rather platform maintainer.

> +
> +properties:
> +  compatible:
> +    contains:

This cannot be unspecific. Drop contains.

> +      enum:
> +        - marvell,armada-7k-xor
> +        - marvell,xor-v2
> +
> +  reg:
> +    items:
> +      - description: DMA registers location and length
> +      - description: global registers location and length

Drop "location and length", redundant.

> +
> +  clocks:
> +    minItems: 1
> +    maxItems: 2
> +
> +  clock-names:
> +    items:
> +      - const: core
> +      - const: reg

This does not match number of items in clocks:

> +
> +  msi-parent:
> +    description:
> +      Phandle to the MSI-capable interrupt controller used for
> +      interrupts.
> +    maxItems: 1
> +
> +  dma-coherent: true

This was not present in the binding and commit msg did not explain why
this is needed. Are devices really DMA coherent?

> +
> +required:
> +  - compatible
> +  - reg
> +  - msi-parent
> +  - dma-coherent
> +
> +if:

Put it under allOf: in this place.

> +  required:
> +    - clocks

This does not work and does not make much sense. Probably you want to
list the items per variant?


> +  properties:
> +    clocks:
> +      minItems: 2
> +      maxItems: 2

Instead list and describe the items.

> +then:
> +  required:
> +    - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    xor0@6a0000 {
> +        compatible = "marvell,armada-7k-xor", "marvell,xor-v2";

This totally does not match your binding.

Please, read example-schema, other bindings, my old talks and other
resources before doing conversions, so we can avoid such trivial
mistakes. You enumerated compatibles (enum), but here have a list. A
list is not an enumeration, obviously...

Best regards,
Krzysztof