Re: [PATCH v12 12/13] dt-bindings: display: vop2: Add rk3576 support

From: Krzysztof Kozlowski <krzk@kernel.org>
To: Andy Yan <andyshrk@163.com>
Cc: heiko@sntech.de, hjc@rock-chips.com, krzk+dt@kernel.org,
	devicetree@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org,
	derek.foreman@collabora.com, detlev.casanova@collabora.com,
	daniel@fooishbar.org, robh@kernel.org,
	sebastian.reichel@collabora.com,
	Andy Yan <andy.yan@rock-chips.com>
Subject: Re: [PATCH v12 12/13] dt-bindings: display: vop2: Add rk3576 support
Date: Wed, 22 Jan 2025 10:55:34 +0100	[thread overview]
Message-ID: <77ea5067-deb2-41d4-ab82-ce19ac018ba3@kernel.org> (raw)
In-Reply-To: <5eb4acaa.6df6.1948d68332d.Coremail.andyshrk@163.com>

On 22/01/2025 10:46, Andy Yan wrote:
>>> -      The VOP interrupt is shared by several interrupt sources, such as
>>> -      frame start (VSYNC), line flag and other status interrupts.
>>> +      For VOP version under rk3576, the interrupt is shared by several interrupt
>>> +      sources, such as frame start (VSYNC), line flag and other interrupt status.
>>> +      For VOP version from rk3576 there is a system interrupt for bus error, and
>>> +      every video port has it's independent interrupts for vsync and other video
>>> +      port related error interrupts.
>>> +
>>> +  interrupt-names:
>>> +    items:
>>> +      - const: sys
>>> +      - const: vp0
>>> +      - const: vp1
>>> +      - const: vp2
>>>  
>>>    # See compatible-specific constraints below.
>>>    clocks:
>>> @@ -135,6 +147,8 @@ allOf:
>>>          interrupts:
>>>            maxItems: 1
>>
>> So this change moves to this patch.
>>
>>>  
>>> +        interrupt-names: false
>>> +
>>>          ports:
>>>            required:
>>>              - port@0
>>> @@ -148,6 +162,39 @@ allOf:
>>>        required:
>>>          - rockchip,grf
>>>  
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - rockchip,rk3576-vop
>>> +    then:
>>> +      properties:
>>> +        clocks:
>>> +          minItems: 5
>>
>> No. You did not implement my comment at all.
>>
>> So again:
>> "Why minItems? Nothing in this patch makes sense for me. Neither changing
>> existing binding nor new binding for rk3576."
> 
> Do you mean because I already defined minItems of clocks is 5 on the top, so 
> there is no need to redefine the same minItems here ?

Lists must be constrained. This is not constrained from the max items
and you repeat existing constrain.

For every variable list you need to provide min and maxItems, except the
edge cases when dimension matches top level dimension.

Standard example is:

https://elixir.bootlin.com/linux/v6.11-rc6/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L127

which I mention on mailing lists multiple times. Also described this
case exactly on my two talks...

> 
>>
>> To address such comment, come with reasonable answer to "why". Not just
>> send the same. It's a waste of my time to keep reviewing the same.
> 
> Before sending this patch, I asked you what the next step should be, but you didn't respond.

You asked whether splitting is correct and I did not object that. I
already said: " You need to split reorganizing", then you asked if you
can split, so sorry, I am not going to keep repeating the same multiple
times.

But anyway this is not about the split, so you did not question last
time how to do it. You just skipped my paragraph asking for "Why?".

Best regards,
Krzysztof

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip