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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAE8VWiLXS1gsxjk7aK335QtZJk7Se+k5VsFzmUpQHfaVJnKa7g@mail.gmail.com>
Date: Mon, 24 Jun 2024 15:44:37 +0530
From: Shresth Prasad <shresthprasad7@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: vkoul@...nel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, 
	dmaengine@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, skhan@...uxfoundation.org, 
	javier.carrasco.cruz@...il.com
Subject: Re: [PATCH] dt-bindings: dma: mv-xor-v2: Convert to dtschema

On Mon, Jun 24, 2024 at 10:47 AM Krzysztof Kozlowski <krzk@...nel.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@...il.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@...nel.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`.

>
> > +
> > +  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?

> > +then:
> > +  required:
> > +    - clock-names
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    xor0@...000 {
> > +        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...

Right, thank you for your feedback.
I'll most certainly read up more on this more.

Regards,
Shresth