| Message ID | 20250121103500.2528258-1-andyshrk@163.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | VOP Support for rk3576 | expand |
On Tue, Jan 21, 2025 at 06:34:57PM +0800, Andy Yan wrote: > 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 v12: > - Split from patch 10/13 Order your patches finally. It's v12 and you still send binding after the user. Read carefully submitting bindings/patches. > > 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 | 55 ++++++++++++++++++- > 1 file changed, 52 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml > index 157a37ed84da..a2a6369c7b6f 100644 > --- a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml > +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml > @@ -21,6 +21,7 @@ properties: > enum: > - rockchip,rk3566-vop > - rockchip,rk3568-vop > + - rockchip,rk3576-vop > - rockchip,rk3588-vop > > reg: > @@ -38,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: > @@ -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." 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. Best regards, Krzysztof
Hi At 2025-01-22 16:04:59, "Krzysztof Kozlowski" <krzk@kernel.org> wrote: >On Tue, Jan 21, 2025 at 06:34:57PM +0800, Andy Yan wrote: >> 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 v12: >> - Split from patch 10/13 > >Order your patches finally. It's v12 and you still send binding after >the user. Read carefully submitting bindings/patches. > >> >> 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 | 55 ++++++++++++++++++- >> 1 file changed, 52 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml >> index 157a37ed84da..a2a6369c7b6f 100644 >> --- a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml >> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml >> @@ -21,6 +21,7 @@ properties: >> enum: >> - rockchip,rk3566-vop >> - rockchip,rk3568-vop >> + - rockchip,rk3576-vop >> - rockchip,rk3588-vop >> >> reg: >> @@ -38,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: >> @@ -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 ? > >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. I might indeed have failed to grasp your main point, I'm indeed not writing dt-schema. Hope you can explain some of the specific issues in more detail to avoid wasting the time of both of us. > >Best regards, >Krzysztof > > >_______________________________________________ >Linux-rockchip mailing list >Linux-rockchip@lists.infradead.org >http://lists.infradead.org/mailman/listinfo/linux-rockchip
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
Hi, At 2025-01-22 16:04:59, "Krzysztof Kozlowski" <krzk@kernel.org> wrote: >On Tue, Jan 21, 2025 at 06:34:57PM +0800, Andy Yan wrote: >> 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 v12: >> - Split from patch 10/13 > >Order your patches finally. It's v12 and you still send binding after >the user. Read carefully submitting bindings/patches. What do you mean by "sending binding after user" here? I think PATCH 1~9 are fix and preparations, PATCH 13("drm/rockchip: vop2: Add support for rk3576") is the user that uses the new binding. > >> >> 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 | 55 ++++++++++++++++++- >> 1 file changed, 52 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml >> index 157a37ed84da..a2a6369c7b6f 100644 >> --- a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml >> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml >> @@ -21,6 +21,7 @@ properties: >> enum: >> - rockchip,rk3566-vop >> - rockchip,rk3568-vop >> + - rockchip,rk3576-vop >> - rockchip,rk3588-vop >> >> reg: >> @@ -38,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: >> @@ -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." > >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. > >Best regards, >Krzysztof >
On 22/01/2025 11:14, Andy Yan wrote: > Hi, > > At 2025-01-22 16:04:59, "Krzysztof Kozlowski" <krzk@kernel.org> wrote: >> On Tue, Jan 21, 2025 at 06:34:57PM +0800, Andy Yan wrote: >>> 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 v12: >>> - Split from patch 10/13 >> >> Order your patches finally. It's v12 and you still send binding after >> the user. Read carefully submitting bindings/patches. > > What do you mean by "sending binding after user" here? > I think PATCH 1~9 are fix and preparations, PATCH 13("drm/rockchip: vop2: Add support for rk3576") > is the user that uses the new binding. Ah, my bad. I thought that's the last one. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml index 157a37ed84da..a2a6369c7b6f 100644 --- a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml @@ -21,6 +21,7 @@ properties: enum: - rockchip,rk3566-vop - rockchip,rk3568-vop + - rockchip,rk3576-vop - rockchip,rk3588-vop reg: @@ -38,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: @@ -135,6 +147,8 @@ allOf: interrupts: maxItems: 1 + 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 + + 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 + + required: + - rockchip,grf + - rockchip,pmu + - if: properties: compatible: @@ -164,6 +211,8 @@ allOf: interrupts: maxItems: 1 + interrupt-names: false + ports: required: - port@0