Message-ID: <652ec3b0.6e7b.1946e39b31e.Coremail.andyshrk@163.com>
Date: Thu, 16 Jan 2025 16:27:42 +0800 (CST)
From: "Andy Yan" <andyshrk@....com>
To: "Krzysztof Kozlowski" <krzk@...nel.org>
Cc: heiko@...ech.de, hjc@...k-chips.com, krzk+dt@...nel.org,
	devicetree@...r.kernel.org, dri-devel@...ts.freedesktop.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-rockchip@...ts.infradead.org, derek.foreman@...labora.com,
	detlev.casanova@...labora.com, daniel@...ishbar.org, robh@...nel.org,
	sebastian.reichel@...labora.com,
	"Andy Yan" <andy.yan@...k-chips.com>
Subject: Re:Re:Re: [PATCH v11 10/11] dt-bindings: display: vop2: Add rk3576
 support


Hi Krzysztof,

At 2025-01-12 18:46:28, "Andy Yan" <andyshrk@....com> wrote:
>
>Hi Krzysztof,
>
>At 2025-01-12 17:27:18, "Krzysztof Kozlowski" <krzk@...nel.org> wrote:
>>On Sat, Jan 11, 2025 at 07:26:08PM +0800, Andy Yan wrote:
>>>    # See compatible-specific constraints below.
>>>    clocks:
>>> @@ -120,43 +133,98 @@ allOf:
>>>        properties:
>>>          compatible:
>>>            contains:
>>> -            const: rockchip,rk3588-vop
>>> +            enum:
>>> +              - rockchip,rk3566-vop
>>> +              - rockchip,rk3568-vop
>>>      then:
>>>        properties:
>>>          clocks:
>>> -          minItems: 7
>>> +          minItems: 5
>>
>>That's wrong, why maxItems became minItems? How is this related to rk3576?
>>
>>> +
>>>          clock-names:
>>> -          minItems: 7
>>> +          minItems: 5
>>
>>You are doing here way too much. You need to split reorganizing, so we
>>will see what comes new.
>>
>>And of course you need to explain why you are changing EXISTING binding
>>(I am not talking about shuffling around - you change the binding).
>
>How about split this patch to two: One rework the existing binding,  make it
>more suitable for expanding to include new SoCs.
>Then add rk3576 in the second patch ?

Can you confirm whether this approach is acceptable?  I want to  clarify  with you
before sending the next version.


>
>
>>
>>
>>> +
>>> +        interrupts:
>>> +          maxItems: 1
>>> +
>>> +        interrupt-names: false
>>>  
>>>          ports:
>>>            required:
>>>              - port@0
>>>              - port@1
>>>              - port@2
>>> -            - port@3
>>> +
>>> +        rockchip,vo1-grf: false
>>> +        rockchip,vop-grf: false
>>> +        rockchip,pmu: false
>>>  
>>>        required:
>>>          - rockchip,grf
>>> -        - rockchip,vo1-grf
>>> -        - rockchip,vop-grf
>>> -        - rockchip,pmu
>>>  
>>> -    else:
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - rockchip,rk3576-vop
>>> +    then:
>>>        properties:
>>> +        clocks:
>>> +          minItems: 5
>>
>>
>>Why minItems? Nothing in this patch makes sense for me. Neither changing
>>existing binding nor new binding for rk3576.
>>
>>> +
>>> +        clock-names:
>>> +          minItems: 5
>>> +
>>> +        interrupts:
>>> +          minItems: 4
>>> +
>>> +        interrupt-names:
>>> +          minItems: 4
>>> +
>>> +        ports:
>>> +          required:
>>> +            - port@0
>>> +            - port@1
>>> +            - port@2
>>> +
>>>          rockchip,vo1-grf: false
>>>          rockchip,vop-grf: false
>>> -        rockchip,pmu: false
>>>  
>>> +      required:
>>> +        - rockchip,grf
>>> +        - rockchip,pmu
>>> +
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: rockchip,rk3588-vop
>>> +    then:
>>> +      properties:
>>>          clocks:
>>> -          maxItems: 5
>>> +          minItems: 7
>>
>>And again weird change to the binding.
>>
>>Best regards,
>>Krzysztof
>>
>>
>>_______________________________________________
>>Linux-rockchip mailing list
>>Linux-rockchip@...ts.infradead.org
>>http://lists.infradead.org/mailman/listinfo/linux-rockchip

Powered by blists - more mailing lists