Re: [PATCH] dt-bindings: dma: mv-xor-v2: Convert to dtschema
From: Krzysztof Kozlowski
Date: Mon Jun 24 2024 - 07:09:31 EST
On 24/06/2024 12:14, Shresth Prasad wrote:
> On Mon, Jun 24, 2024 at 10:47 AM Krzysztof Kozlowski <krzk@xxxxxxxxxx> 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@xxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxx>
>>
>> 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