Re: [PATCH v5 12/12] dt-bindings: fsl-dma: fsl-edma: add edma3 compatible string

[Frank Li]

On Tue, Jun 13, 2023 at 11:43:31PM +0200, Krzysztof Kozlowski wrote:
> On 13/06/2023 23:31, Frank Li wrote:
> > Extend Freescale eDMA driver bindings to support eDMA3 IP blocks in
> > i.MX8QM and i.MX8QXP SoCs. In i.MX93, both eDMA3 and eDMA4 are now.
> > 
> > Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> > ---
> >  .../devicetree/bindings/dma/fsl,edma.yaml     | 118 +++++++++++++++---
> >  1 file changed, 100 insertions(+), 18 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/dma/fsl,edma.yaml b/Documentation/devicetree/bindings/dma/fsl,edma.yaml
> > index 5fd8fc604261..2f79492fb332 100644
> > --- a/Documentation/devicetree/bindings/dma/fsl,edma.yaml
> > +++ b/Documentation/devicetree/bindings/dma/fsl,edma.yaml
> > @@ -21,32 +21,20 @@ properties:
> >        - enum:
> >            - fsl,vf610-edma
> >            - fsl,imx7ulp-edma
> > +          - fsl,imx8qm-adma
> > +          - fsl,imx8qm-edma
> > +          - fsl,imx93-edma3
> > +          - fsl,imx93-edma4
> >        - items:
> >            - const: fsl,ls1028a-edma
> >            - const: fsl,vf610-edma
> >  
> > -  reg:
> > -    minItems: 2
> > -    maxItems: 3
> > -
> > -  interrupts:
> > -    minItems: 2
> > -    maxItems: 17
> 
> What is happening here?

I found dt_check always check this part firstly, then check allOf.

> 
> > -
> > -  interrupt-names:
> > -    minItems: 2
> > -    maxItems: 17
> > -
> > -  "#dma-cells":
> > -    const: 2
> > -
> > -  dma-channels:
> > -    const: 32
> 
> No, why all these are being removed?

I move common part ahead of if-then-else branch to read early.

> 
> > -
> >    clocks:
> > +    minItems: 1
> >      maxItems: 2
> >  
> >    clock-names:
> > +    minItems: 1
> >      maxItems: 2
> >  
> >    big-endian:
> > @@ -55,6 +43,43 @@ properties:
> >        eDMA are implemented in big endian mode, otherwise in little mode.
> >      type: boolean
> >  
> > +if:
> 
> This should not be outside of your allOf. This patch looks entirely
> different than your v4 and I don't really understand why.
> 

allOf looks like addtional constraints addition to previous define.
for example: 
    if previous interrupts is 17, I can't overwrite to bigger value 64
in this sesson. 

previous version: dts check report two error, 
first: interrupt is too long. (look like check top one)
then: interrupt is too short. (look like check allOf part)

> 
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        enum:
> > +          - fsl,imx8qm-adma
> > +          - fsl,imx8qm-edma
> > +          - fsl,imx93-edma3
> > +          - fsl,imx93-edma4
> > +then:
> > +  properties:
> > +    reg:
> > +      maxItems: 1
> > +    interrupts:
> > +      minItems: 1
> > +      maxItems: 64
> 
> What's more, you don't have these properties defined in top-level.
> Sorry, they should not be moved. I did not ask for this.

It is there. 
if-then-else before "required"

> 
> Best regards,
> Krzysztof
>