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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 14/06/2023 00:01, Frank Li wrote:
> 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@nxp.com>
>>> ---
>>>  .../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. 

Yes, because the top-level had wrong constraint. Fix top-level, don't
remove it.

> 
> 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"

It's in if, not in top-level properties.


Best regards,
Krzysztof