diff mbox series

[v11,10/11] dt-bindings: display: vop2: Add rk3576 support

Message ID 20250111112614.432247-11-andyshrk@163.com (mailing list archive)
State New
Headers show
Series VOP Support for rk3576 | expand

Commit Message

Andy Yan Jan. 11, 2025, 11:26 a.m. UTC 
From: Andy Yan <andy.yan@rock-chips.com>

Add vop found on rk3576, the main difference between rk3576 and the
previous vop is that each VP has its own interrupt line.

Signed-off-by: Andy Yan <andy.yan@rock-chips.com>

---

Changes in v11:
- Remove redundant min/maxItems constraint

Changes in v10:
- Move interrupt-names back to top level
- Add constraint of interrupts for all platform
- Add constraint for all grf phandles
- Reorder some properties

Changes in v9:
- Drop 'vop-' prefix of interrupt-names.
- Add blank line between DT properties
- Remove list interrupt-names in top level

Changes in v8:
- Fix dt_binding_check errors
- ordered by soc name
- Link to the previous version:
  https://lore.kernel.org/linux-rockchip/6pn3qjxotdtpzucpul24yro7ppddezwuizneovqvmgdwyv2j7p@ztg4mqyiqmjf/T/#u

Changes in v4:
- describe constraint SOC by SOC, as interrupts of rk3576 is very
  different from others
- Drop Krzysztof's Reviewed-by, as this version changed a lot.

Changes in v3:
- ordered by soc name
- Add description for newly added interrupt

Changes in v2:
- Add dt bindings

 .../display/rockchip/rockchip-vop2.yaml       | 97 ++++++++++++++++---
 1 file changed, 83 insertions(+), 14 deletions(-)

Comments

Krzysztof Kozlowski Jan. 12, 2025, 9:27 a.m. UTC | #1 
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).


> +
> +        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
Andy Yan Jan. 12, 2025, 10:46 a.m. UTC | #2 
Hi Krzysztof,

At 2025-01-12 17:27:18, "Krzysztof Kozlowski" <krzk@kernel.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 ?


>
>
>> +
>> +        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@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-rockchip
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml
index 2531726af306..a2a6369c7b6f 100644
--- a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml
+++ b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml
@@ -14,12 +14,14 @@  description:
 maintainers:
   - Sandy Huang <hjc@rock-chips.com>
   - Heiko Stuebner <heiko@sntech.de>
+  - Andy Yan <andyshrk@163.com>
 
 properties:
   compatible:
     enum:
       - rockchip,rk3566-vop
       - rockchip,rk3568-vop
+      - rockchip,rk3576-vop
       - rockchip,rk3588-vop
 
   reg:
@@ -37,10 +39,21 @@  properties:
       - const: gamma-lut
 
   interrupts:
-    maxItems: 1
+    minItems: 1
+    maxItems: 4
     description:
-      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:
@@ -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
+
         clock-names:
-          minItems: 7
+          minItems: 5
+
+        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
+
+        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
+
         clock-names:
-          maxItems: 5
+          minItems: 7
+
+        interrupts:
+          maxItems: 1
+
+        interrupt-names: false
 
         ports:
           required:
             - port@0
             - port@1
             - port@2
+            - port@3
+
+      required:
+        - rockchip,grf
+        - rockchip,vo1-grf
+        - rockchip,vop-grf
+        - rockchip,pmu
 
 additionalProperties: false
 
@@ -184,6 +252,7 @@  examples:
                               "dclk_vp1",
                               "dclk_vp2";
                 power-domains = <&power RK3568_PD_VO>;
+                rockchip,grf = <&grf>;
                 iommus = <&vop_mmu>;
                 vop_out: ports {
                     #address-cells = <1>;