Re: [PATCH 1/5] dt-bindings: dma: ti: k3-bcdma: Add bindings for BCDMA CSI RX

From: Vignesh Raghavendra
Date: Wed Dec 07 2022 - 01:10:18 EST


Hi Krzysztof,

On 06/12/22 14:02, Krzysztof Kozlowski wrote:
> On 06/12/2022 05:35, Vignesh Raghavendra wrote:
>> AM62A SoC has a dedicated BCDMA that serves Camera Serial Interface
>> (CSI) IP. Add new compatible for the same. Unlike system
>> BCDMA, this instance only has RX DMA channels and lack TX or block copy
>> channel. Thus make those properties optional. Additionally CSI RX has
>> independent power domain, add the binding for the same.
>>
>> Signed-off-by: Vignesh Raghavendra <vigneshr@xxxxxx>
>> ---
>> .../devicetree/bindings/dma/ti/k3-bcdma.yaml | 87 ++++++++++++++-----
>> 1 file changed, 63 insertions(+), 24 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/dma/ti/k3-bcdma.yaml b/Documentation/devicetree/bindings/dma/ti/k3-bcdma.yaml
>> index 08627d91e607..d7b5adbb9b2e 100644
>> --- a/Documentation/devicetree/bindings/dma/ti/k3-bcdma.yaml
>> +++ b/Documentation/devicetree/bindings/dma/ti/k3-bcdma.yaml
>> @@ -32,9 +32,66 @@ allOf:
>> - $ref: /schemas/dma/dma-controller.yaml#
>> - $ref: /schemas/arm/keystone/ti,k3-sci-common.yaml#
>>
>
> When adding if:then:, please move entire allOf after "required:" part.

Sure, will do

>
>> + - if:
>> + properties:
>> + compatible:
>> + contains:
>> + const: ti,am62a-dmss-bcdma-csirx
>> + then:
>> + properties:
>> + ti,sci-rm-range-bchan: false
>> + ti,sci-rm-range-tchan: false
>> +
>> + reg:
>> + maxItems: 3
>> +
>> + reg-names:
>> + items:
>> + - const: gcfg
>> + - const: rchanrt
>> + - const: ringrt
>
> With my changes further this can be only "maxItems: 3"

Yes, but wont that mean any of the 3 reg-names out of the 5? Would it
not be better to further restrict specifically to above 3 reg-names (as
thats how the IP is)

>
>> +
>> + required:
>> + - compatible
>> + - "#dma-cells"
>> + - reg
>> + - reg-names
>> + - msi-parent
>> + - ti,sci
>> + - ti,sci-dev-id
>> + - ti,sci-rm-range-rchan
>> + - power-domains
>> +
>> + else:
>> + properties:
>> + reg:
>> + maxItems: 5
>> +
>> + reg-names:
>> + items:
>> + - const: gcfg
>> + - const: bchanrt
>> + - const: rchanrt
>> + - const: tchanrt
>> + - const: ringrt
>
> With my changes further this can be only "minItems: 5"

Ok.

>
>> +
>> + required:
>> + - compatible
>> + - "#dma-cells"
>> + - reg
>> + - reg-names
>> + - msi-parent
>> + - ti,sci
>> + - ti,sci-dev-id
>> + - ti,sci-rm-range-bchan
>> + - ti,sci-rm-range-tchan
>> + - ti,sci-rm-range-rchan
>> +
>> properties:
>> compatible:
>> - const: ti,am64-dmss-bcdma
>> + enum:
>> + - ti,am64-dmss-bcdma
>> + - ti,am62a-dmss-bcdma-csirx
>
> Keep some order, e.g. alphabetical. This reduces later conflicts on
> simultaneous edits.

Will fix!

>
>>
>> "#dma-cells":
>> const: 3
>> @@ -65,19 +122,13 @@ properties:
>>
>> cell 3: ASEL value for the channel
>>
>> - reg:
>> - maxItems: 5
>
> Keep it here with widest constrains - minItems: 3, maxItems: 5
>
>> -
>> - reg-names:
>> - items:
>> - - const: gcfg
>> - - const: bchanrt
>> - - const: rchanrt
>> - - const: tchanrt
>> - - const: ringrt
>
> Keep the list here with minItems: 3
>
>> -
>> msi-parent: true
>>
>> + power-domains:
>> + description:
>> + Power domain if available
>> + maxItems: 1
>> +
>> ti,asel:
>> $ref: /schemas/types.yaml#/definitions/uint32
>> description: ASEL value for non slave channels
>> @@ -115,18 +166,6 @@ properties:
>> items:
>> maximum: 0x3f
>>
>> -required:
>> - - compatible
>> - - "#dma-cells"
>> - - reg
>> - - reg-names
>> - - msi-parent
>> - - ti,sci
>> - - ti,sci-dev-id
>> - - ti,sci-rm-range-bchan
>> - - ti,sci-rm-range-tchan
>> - - ti,sci-rm-range-rchan
>
> Keep required here. Customize it if needed in if:then:else.

Got it, will fix accordingly...

>
>> -
>> unevaluatedProperties: false
>>
>> examples:
>
> Best regards,
> Krzysztof
>

Thanks for the review!


--
Regards
Vignesh