Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <8e93f459-53e0-ee39-96cc-2c9e51a72547@ti.com>
Date:   Wed, 7 Dec 2022 11:39:56 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.0
Subject: Re: [PATCH 1/5] dt-bindings: dma: ti: k3-bcdma: Add bindings for
 BCDMA CSI RX
Content-Language: en-US
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        Peter Ujfalusi <peter.ujfalusi@gmail.com>,
        Vinod Koul <vkoul@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
CC:     <dmaengine@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20221206043554.1521522-1-vigneshr@ti.com>
 <20221206043554.1521522-2-vigneshr@ti.com>
 <63e1e565-b1e7-ecfc-009a-ee036108f160@linaro.org>
From:   Vignesh Raghavendra <vigneshr@ti.com>
In-Reply-To: <63e1e565-b1e7-ecfc-009a-ee036108f160@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@ti.com>
>> ---
>>  .../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