Re: [PATCH v2 4/5] dt-bindings: dma: fsl-edma: add fsl,imx8ulp-edma compatible string

[Frank Li]

On Mon, Mar 04, 2024 at 10:44:23AM -0600, Rob Herring wrote:
> On Thu, Feb 29, 2024 at 03:58:10PM -0500, Frank Li wrote:
> > From: Joy Zou <joy.zou@xxxxxxx>
> > 
> > Introduce the compatible string 'fsl,imx8ulp-edma' to enable support for
> > the i.MX8ULP's eDMA, alongside adjusting the clock numbering. The i.MX8ULP
> > eDMA architecture features one clock for each DMA channel and an additional
> > clock for the core controller. Given a maximum of 32 DMA channels, the
> > maximum clock number consequently increases to 33.
> > 
> > Signed-off-by: Joy Zou <joy.zou@xxxxxxx>
> > Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> > ---
> >  .../devicetree/bindings/dma/fsl,edma.yaml          | 26 ++++++++++++++++++++--
> >  1 file changed, 24 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/dma/fsl,edma.yaml b/Documentation/devicetree/bindings/dma/fsl,edma.yaml
> > index aa51d278cb67b..55cce79c759f8 100644
> > --- a/Documentation/devicetree/bindings/dma/fsl,edma.yaml
> > +++ b/Documentation/devicetree/bindings/dma/fsl,edma.yaml
> > @@ -23,6 +23,7 @@ properties:
> >            - fsl,imx7ulp-edma
> >            - fsl,imx8qm-adma
> >            - fsl,imx8qm-edma
> > +          - fsl,imx8ulp-edma
> >            - fsl,imx93-edma3
> >            - fsl,imx93-edma4
> >            - fsl,imx95-edma5
> > @@ -53,11 +54,11 @@ properties:
> >  
> >    clocks:
> >      minItems: 1
> > -    maxItems: 2
> > +    maxItems: 33
> >  
> >    clock-names:
> >      minItems: 1
> > -    maxItems: 2
> > +    maxItems: 33
> >  
> >    big-endian:
> >      description: |
> > @@ -108,6 +109,7 @@ allOf:
> >        properties:
> >          clocks:
> >            minItems: 2
> > +          maxItems: 2
> >          clock-names:
> >            items:
> >              - const: dmamux0
> > @@ -136,6 +138,7 @@ allOf:
> >        properties:
> >          clock:
> >            minItems: 2
> > +          maxItems: 2
> >          clock-names:
> >            items:
> >              - const: dma
> > @@ -151,6 +154,25 @@ allOf:
> >          dma-channels:
> >            const: 32
> >  
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: fsl,imx8ulp-edma
> > +    then:
> > +      properties:
> > +        clock:
> 
> clocks
> 
> > +          maxItems: 33
> 
> That is already the max. I think you want 'minItems: 33' here.
> 
> > +        clock-names:
> > +          items:
> > +            - const: dma
> > +            - pattern: "^CH[0-31]-clk$"
> 
> '-clk' is redundant. [0-31] is not how you do a range of numbers with 
> regex. 
> 
> This doesn't cover clocks 3-33. Not a great way to express in 
> json-schema, but this should do it:
> 
> allOf:
>   - items:
>       - const: dma
>   - items:
>       oneOf:
>         - const: dma
>         - pattern: "^ch([0-9]|[1-2][0-9]|[3[01])$"

I understand pattern is wrong. But I don't understand why need 'allOf'.
8ulp need clock 'dma" and "ch*". I think 

items:
    - const: dma 
    - pattern: "^CH[0-31]-clk$"

should be enough.

If you means put on top allOf, other platform use clock name such as
'dmamux0'.

Frank

> 
> That doesn't enforce the order of 'chN' entries though. Probably good 
> enough.
> 
> 
> > +        interrupt-names: false
> > +        interrupts:
> > +          maxItems: 32
> 
> minItems
> 
> > +        "#dma-cells":
> > +          const: 3
> 
> Is what is in each cell defined somewhere? If not, you need a 
> description with those details.
> 
> Rob