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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 24/06/2024 12:14, Shresth Prasad wrote:
> On Mon, Jun 24, 2024 at 10:47 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> 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:
> 
> I'm not sure what you mean, the original txt stated that `clock-names`
> is only required if there are two `clocks`.

Exactly. It said "required", not "disallowed for 1 clock case". You
basically made it impossible to use for one case, so standard reply:
these should be always in sync.

> 
>>
>>> +
>>> +  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?
> 
> Sorry about that, I added this because all the nodes I looked at
> contained `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.
>>
> 
> I did it this way to allow for `clock-names` to only be required if there
> are two `clocks` present. Is there another way I should be doing this?

Why number of clocks would mean you need clock-names? Why does it
matter? If the driver is taking second clock by name, it does not mean
second clock name can be anything for other cases.


Best regards,
Krzysztof