Re: [PATCH 5/9] dt-bindings: dma: Convert fsl,elo*-dma bindings to YAML

[J. Neuschäfer]

On Wed, Jan 29, 2025 at 05:52:31PM -0500, Frank Li wrote:
> On Sun, Jan 26, 2025 at 07:59:00PM +0100, J. Neuschäfer wrote:
> > The devicetree bindings for Freescale DMA engines have so far existed as
> > a text file. This patch converts them to YAML, and specifies all the
> > compatible strings currently in use in arch/powerpc/boot/dts.
> >
> > Signed-off-by: J. Neuschäfer <j.ne@xxxxxxxxxx>
> > ---
> >  .../devicetree/bindings/dma/fsl,elo-dma.yaml       | 129 +++++++++++++
> >  .../devicetree/bindings/dma/fsl,elo3-dma.yaml      | 105 +++++++++++
> >  .../devicetree/bindings/dma/fsl,eloplus-dma.yaml   | 120 ++++++++++++
> >  .../devicetree/bindings/powerpc/fsl/dma.txt        | 204 ---------------------
> >  4 files changed, 354 insertions(+), 204 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/dma/fsl,elo-dma.yaml b/Documentation/devicetree/bindings/dma/fsl,elo-dma.yaml
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..d1f4978a672c1217c322c27f243470b2de8c99d4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/dma/fsl,elo-dma.yaml
> > @@ -0,0 +1,129 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/dma/fsl,elo-dma.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Freescale Elo DMA Controller
> > +
> > +maintainers:
> > +  - J. Neuschäfer <j.ne@xxxxxxxxxx>
> > +
> > +description: |
> 
> needn't | here

Will remove.

> 
> > +  This is a little-endian 4-channel DMA controller, used in Freescale mpc83xx
> > +  series chips such as mpc8315, mpc8349, mpc8379 etc.
> > +
> > +  Note on DMA channel compatible properties: The compatible property must say
> > +  "fsl,elo-dma-channel" or "fsl,eloplus-dma-channel" to be used by the Elo DMA
> 
> There are not 'fsl,eloplus-dma-channel' under "^dma-channel@.*$". I suggest
> remove this because 'compatible': items already show such information.

Good point, I'll trim this text down.

> > +  driver (fsldma).  Any DMA channel used by fsldma cannot be used by another
> > +  DMA driver, such as the SSI sound drivers for the MPC8610.  Therefore, any
> > +  DMA channel that should be used for another driver should not use
> > +  "fsl,elo-dma-channel" or "fsl,eloplus-dma-channel".  For the SSI drivers, for
> > +  example, the compatible property should be "fsl,ssi-dma-channel".  See
> > +  ssi.txt for more information.

I noticed fsl,ssi.txt has been converted to YAML since this text was
originally written, so I'll make reference to that.

[...]
> > +examples:
> > +  - |
> > +    dma@82a8 {
> > +        #address-cells = <1>;
> > +        #size-cells = <1>;
> > +        compatible = "fsl,mpc8349-dma", "fsl,elo-dma";
> > +        reg = <0x82a8 4>;
> 
> compatible and reg should be first two property.

Will fix.

> 
> > +        ranges = <0 0x8100 0x1a4>;
> > +        interrupt-parent = <&ipic>;
> > +        interrupts = <71 8>;
> > +        cell-index = <0>;
> 
> need space line here.

Will fix.

> > +        dma-channel@0 {
> > +            compatible = "fsl,mpc8349-dma-channel", "fsl,elo-dma-channel";
> > +            cell-index = <0>;
> > +            reg = <0 0x80>;
> > +            interrupt-parent = <&ipic>;
> > +            interrupts = <71 8>;
> > +        };
> 
> need space line here. check other's example dts

Will fix in all files.


[...]
> > +patternProperties:
> > +  "^dma-channel@.*$":
> > +    type: object
> > +
> > +    properties:
> > +      compatible:
> > +        items:
> > +          - enum:
> > +              - fsl,mpc8540-dma-channel
> > +              - fsl,mpc8541-dma-channel
> > +              - fsl,mpc8548-dma-channel
> > +              - fsl,mpc8555-dma-channel
> > +              - fsl,mpc8560-dma-channel
> > +              - fsl,mpc8572-dma-channel
> > +          - const: fsl,eloplus-dma-channel
> 
> I think you can merge this fsl,mpc83xx-dma yaml file
> 
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: fsl,elo-dma
> +    then:
> +      patternProperties:
> +        "^dma-channel@.*$":
> +          properties:
> +            compatible:
> +              items:
> +                - enum:
> 			....
> +    else
> +      patternProperties:
> +        "^dma-channel@.*$":
> +          properties:
> +            compatible:
> +              items:
> +                - enum:
>                         ....
> +                - const: fsl,eloplus-dma-channel

I suppose that works, but I'm not entirely convinced it would help with
readability, compared to leaving the three variants separate.


Thank you for your review!

Best regards,
J. Neuschäfer