Re: [PATCH] dt-bindings: dma: convert text based binding to json schema

[Charan Pedumuru]

On 02-05-2025 13:28, Krzysztof Kozlowski wrote:
> On Thu, May 01, 2025 at 01:12:36PM GMT, Charan Pedumuru wrote:
> > Update text binding to YAML.
> > Changes during conversion:
> > - Add a fallback for "nvidia,tegra30-apbdma" as it is
> >    compatible with the IP core on "nvidia,tegra20-apbdma".
> > - Update examples and include appropriate file directives to resolve
> >    errors identified by `dt_binding_check` and `dtbs_check`.
> Please use subject prefixes matching the subsystem. You can get them for
> example with 'git log --oneline -- DIRECTORY_OR_FILE' on the directory
> your patch is touching. For bindings, the preferred subjects are
> explained here:
> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
>
> Missing final nvidia,tegra20-apbdma prefix.


Sure, will update that accordingly.


> > Signed-off-by: Charan Pedumuru <charan.pedumuru@xxxxxxxxx>
> > ---
> >   .../bindings/dma/nvidia,tegra20-apbdma.txt         | 44 -----------
> >   .../bindings/dma/nvidia,tegra20-apbdma.yaml        | 90 ++++++++++++++++++++++
> >   2 files changed, 90 insertions(+), 44 deletions(-)
> ...
>
> > +$id: http://devicetree.org/schemas/dma/nvidia,tegra20-apbdma.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NVIDIA Tegra APB DMA Controller
> > +
> > +description: |
> Do not need '|' unless you need to preserve formatting.
>
> > +  The NVIDIA Tegra APB DMA controller is a hardware component that
> > +  enables direct memory access (DMA) on Tegra systems. It facilitates
> > +  data transfer between I/O devices and main memory without constant
> > +  CPU intervention.
> > +
> > +maintainers:
> > +  - Jonathan Hunter <jonathanh@xxxxxxxxxx>
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - const: nvidia,tegra20-apbdma
> > +      - items:
> > +          - const: nvidia,tegra30-apbdma
> > +          - const: nvidia,tegra20-apbdma
> > +
> > +  "#dma-cells":
> > +    description:
> > +      Must be <1>. This dictates the length of DMA specifiers
> > +      in client node's dmas properties.
> Drop description, you are not telling here anything new except
> explaining basically DT syntax.
>
> > +    const: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  reg:
> > +    maxItems: 1
> reg is the second property. Old binding even had it correct.
>
> > +
> > +  interrupts:
> > +    description:
> > +      Should contain all of the per-channel DMA interrupts in
> > +      ascending order with respect to the DMA channel index.
> > +    minItems: 1
> > +    maxItems: 32
> > +
> > +  resets:
> > +    maxItems: 1
> > +
> > +  reset-names:
> > +    const: dma
> > +
> > +required:
> > +  - compatible
> > +  - reg
> And here the order is correct...
>
> > +  - interrupts
> > +  - clocks
> But here different. Keep the same order as in properties.
>
> > +  - resets
> > +  - reset-names
> > +  - "#dma-cells"
> > +
> missing allOf: to dma-controller


The reason, I didn't include dma-controller is the pattern under 
$nodename in dma-controller is not matching with the pattern present in 
dts files.

So, I excluded it.


> > +additionalProperties: false
> unevaluatedProperties instead. Just open any other DMA binding.


sure.


> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> You included this...


I checked the other DMA binding from nvidia, they included the same 
header file. I can change it to irq.h, if required


> > +    #include <dt-bindings/reset/tegra186-reset.h>
> > +    dma@6000a000 {
> Doesn't look like correct name. Schema requires specific name.


The dt_binding_check is successful when I included allOf to 
dma-controller and used dma-controller@600 as node name, but when I was 
checking for dtb, the pattern is not matching with the one

inside dma-controller, so I removed allOf and changed the node name in 
examples according to the node present inside the dts file.


> > +        compatible = "nvidia,tegra30-apbdma", "nvidia,tegra20-apbdma";
> > +        reg = <0x6000a000 0x1200>;
> > +        interrupts = <0 136 0x04>,
> ... so use it.
>
> Best regards,
> Krzysztof
--
Best Regards,
Charan.