public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH v3] dt-bindings: display: ti, am65x-dss: Fix AM62L DSS reg and clock constraints
@ 2026-04-10 10:59 Swamil Jain
  2026-04-11 14:07 ` Krzysztof Kozlowski
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Swamil Jain @ 2026-04-10 10:59 UTC (permalink / raw)
  To: jyri.sarha, tomi.valkeinen, maarten.lankhorst, mripard,
	tzimmermann, airlied, simona, robh, krzk+dt, conor+dt, devarsht
  Cc: dri-devel, devicetree, linux-kernel, praneeth, vigneshr, s-jain1

The AM62L DSS [1] support incorrectly used the same register and
clock constraints as AM65x, but AM62L has a single video port

Fix this by adding conditional constraints that properly define the
register regions and clocks for AM62L DSS (single video port) versus
other AM65x variants (dual video port).

[1]: Section 12.7 (Display Subsystem and Peripherals)
Link : https://www.ti.com/lit/pdf/sprujb4

Fixes: cb8d4323302c ("dt-bindings: display: ti,am65x-dss: Add support for AM62L DSS")
Cc: stable@vger.kernel.org
Signed-off-by: Swamil Jain <s-jain1@ti.com>
---
Validated the changes with some examples:
https://gist.github.com/swamiljain/79f30568c9ece89f5a20218f52647486

Changelog:
v2->v3:
- Reduce redundancy and use constraints suggested by maintainers
- Remove blank line between the tags

Link to v2:
https://lore.kernel.org/all/20260129150601.185882-1-s-jain1@ti.com/

v1->v2:
- Remove oneOf from top level constraints, it makes bindings redundant
- Remove minItems from top level constraints
- "dma-coherent" property shouldn't be changed in v1 itself
- Add description for reg-names, clock and clock-names
- Add constraints specific to AM62L and for other SoCs within allOf
  check

Link to v1:
https://lore.kernel.org/all/20251224133150.2266524-1-s-jain1@ti.com/
---
 .../bindings/display/ti/ti,am65x-dss.yaml     | 66 ++++++++++++++-----
 1 file changed, 48 insertions(+), 18 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
index 38fcee91211e..d8a05bf62c2f 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
+++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
@@ -36,34 +36,50 @@ properties:
   reg:
     description:
       Addresses to each DSS memory region described in the SoC's TRM.
-    items:
-      - description: common DSS register area
-      - description: VIDL1 light video plane
-      - description: VID video plane
-      - description: OVR1 overlay manager for vp1
-      - description: OVR2 overlay manager for vp2
-      - description: VP1 video port 1
-      - description: VP2 video port 2
-      - description: common1 DSS register area
+    oneOf:
+      - items:
+          - description: common DSS register area
+          - description: VIDL1 light video plane
+          - description: VID video plane
+          - description: OVR1 overlay manager for vp1
+          - description: OVR2 overlay manager for vp2
+          - description: VP1 video port 1
+          - description: VP2 video port 2
+          - description: common1 DSS register area
+      - items:
+          - description: common DSS register area
+          - description: VIDL1 light video plane
+          - description: OVR1 overlay manager for vp1
+          - description: VP1 video port 1
+          - description: common1 DSS register area
 
   reg-names:
-    items:
-      - const: common
-      - const: vidl1
-      - const: vid
-      - const: ovr1
-      - const: ovr2
-      - const: vp1
-      - const: vp2
-      - const: common1
+    oneOf:
+      - items:
+          - const: common
+          - const: vidl1
+          - const: vid
+          - const: ovr1
+          - const: ovr2
+          - const: vp1
+          - const: vp2
+          - const: common1
+      - items:
+          - const: common
+          - const: vidl1
+          - const: ovr1
+          - const: vp1
+          - const: common1
 
   clocks:
+    minItems: 2
     items:
       - description: fck DSS functional clock
       - description: vp1 Video Port 1 pixel clock
       - description: vp2 Video Port 2 pixel clock
 
   clock-names:
+    minItems: 2
     items:
       - const: fck
       - const: vp1
@@ -179,6 +195,20 @@ allOf:
         ports:
           properties:
             port@1: false
+        clock-names:
+          maxItems: 2
+        clocks:
+          maxItems: 2
+        reg:
+          maxItems: 5
+    else:
+      properties:
+        clock-names:
+          minItems: 3
+        clocks:
+          minItems: 3
+        reg:
+          minItems: 8
 
   - if:
       properties:

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] dt-bindings: display: ti, am65x-dss: Fix AM62L DSS reg and clock constraints
  2026-04-10 10:59 [PATCH v3] dt-bindings: display: ti, am65x-dss: Fix AM62L DSS reg and clock constraints Swamil Jain
@ 2026-04-11 14:07 ` Krzysztof Kozlowski
  2026-04-11 23:43 ` Claude review: " Claude Code Review Bot
  2026-04-11 23:43 ` Claude Code Review Bot
  2 siblings, 0 replies; 4+ messages in thread
From: Krzysztof Kozlowski @ 2026-04-11 14:07 UTC (permalink / raw)
  To: Swamil Jain
  Cc: jyri.sarha, tomi.valkeinen, maarten.lankhorst, mripard,
	tzimmermann, airlied, simona, robh, krzk+dt, conor+dt, devarsht,
	dri-devel, devicetree, linux-kernel, praneeth, vigneshr

On Fri, Apr 10, 2026 at 04:29:55PM +0530, Swamil Jain wrote:
>    clocks:
> +    minItems: 2
>      items:
>        - description: fck DSS functional clock
>        - description: vp1 Video Port 1 pixel clock
>        - description: vp2 Video Port 2 pixel clock
>  
>    clock-names:
> +    minItems: 2
>      items:
>        - const: fck
>        - const: vp1
> @@ -179,6 +195,20 @@ allOf:
>          ports:
>            properties:
>              port@1: false
> +        clock-names:
> +          maxItems: 2
> +        clocks:
> +          maxItems: 2
> +        reg:
> +          maxItems: 5

Also constrain for reg-names,

> +    else:
> +      properties:
> +        clock-names:
> +          minItems: 3
> +        clocks:
> +          minItems: 3
> +        reg:
> +          minItems: 8

Same here, please.

And if you are sending new version: they should be listed in the same
order as in top-level properties, so reg, reg-names, clocks and
clock-names. (juging by the diff)

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Claude review: dt-bindings: display: ti, am65x-dss: Fix AM62L DSS reg and clock constraints
  2026-04-10 10:59 [PATCH v3] dt-bindings: display: ti, am65x-dss: Fix AM62L DSS reg and clock constraints Swamil Jain
  2026-04-11 14:07 ` Krzysztof Kozlowski
@ 2026-04-11 23:43 ` Claude Code Review Bot
  2026-04-11 23:43 ` Claude Code Review Bot
  2 siblings, 0 replies; 4+ messages in thread
From: Claude Code Review Bot @ 2026-04-11 23:43 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: dt-bindings: display: ti, am65x-dss: Fix AM62L DSS reg and clock constraints
Author: Swamil Jain <s-jain1@ti.com>
Patches: 2
Reviewed: 2026-04-12T09:43:11.719203

---

This is a single patch (v3) fixing the DT binding for the AM62L DSS in `ti,am65x-dss.yaml`. The AM62L DSS has a single video port with only a "lite plane" (no VID plane, no OVR2, no VP2), but the original AM62L binding addition incorrectly reused the same 8-register, 3-clock constraints as the dual-video-port AM65x/AM625/AM62A7 variants.

The fix is well-structured and uses two complementary mechanisms:
1. `oneOf` at the property level for `reg`/`reg-names` to define both valid register layouts (8-item for dual-VP and 5-item for AM62L).
2. `if`/`then`/`else` conditionals in `allOf` to bind the correct register/clock counts to the correct `compatible` string.

The approach is clean and follows established dt-binding conventions. The `Fixes:` and `Cc: stable` tags are appropriate since the original AM62L binding would have validated incorrectly (requiring 8 registers and 3 clocks that don't exist on AM62L hardware).

**Verdict: Patch looks correct and ready to apply, with minor observations noted below.**

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Claude review: dt-bindings: display: ti, am65x-dss: Fix AM62L DSS reg and clock constraints
  2026-04-10 10:59 [PATCH v3] dt-bindings: display: ti, am65x-dss: Fix AM62L DSS reg and clock constraints Swamil Jain
  2026-04-11 14:07 ` Krzysztof Kozlowski
  2026-04-11 23:43 ` Claude review: " Claude Code Review Bot
@ 2026-04-11 23:43 ` Claude Code Review Bot
  2 siblings, 0 replies; 4+ messages in thread
From: Claude Code Review Bot @ 2026-04-11 23:43 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Overall: Good patch. The logic is correct and the YAML schema structure is sound.**

**Positive aspects:**

- The `oneOf` for `reg` correctly differentiates AM62L's 5 registers (common, vidl1, ovr1, vp1, common1) from the dual-VP variants' 8 registers (adding vid, ovr2, vp2). This accurately reflects that AM62L lacks the full VID plane, the second overlay manager, and the second video port.

- The `reg-names` `oneOf` is kept in sync with the `reg` `oneOf`, maintaining the correct name-to-register mapping.

- The `if`/`then`/`else` conditional tightly binds the register and clock counts to the `compatible` string, preventing mismatched DTs from validating:

```yaml
    then:
      properties:
        clock-names:
          maxItems: 2
        clocks:
          maxItems: 2
        reg:
          maxItems: 5
    else:
      properties:
        clock-names:
          minItems: 3
        clocks:
          minItems: 3
        reg:
          minItems: 8
```

**Minor observations (not blocking):**

1. **Base `clocks`/`clock-names` still list 3 items:** The base `items` array still describes all 3 clocks (fck, vp1, vp2), with `minItems: 2` added to relax the requirement. For AM62L (maxItems: 2), the third item schema (vp2) becomes dead/unreachable. This is functionally correct — JSON Schema tuple validation simply won't exercise the third schema — but it could be slightly clearer if a comment or the description noted that vp2 only applies to dual-VP variants. This is purely a readability nit.

2. **Commit message formatting:** The subject line wraps oddly across two lines in the mbox:
   ```
   Subject: [PATCH v3] dt-bindings: display: ti,
    am65x-dss: Fix AM62L DSS reg and clock constraints
   ```
   This appears to be an artifact of the mail header folding (the comma after "ti" triggers the line wrap). It should be fine when applied via `git am`, but worth verifying the subject renders correctly.

3. **No example DT for AM62L:** The `examples:` section has DT snippets for `am65x-dss` and `am625-dss` but none for `am62l-dss`. Adding an AM62L example with the 5-register, 2-clock layout would be helpful for validation and documentation, but this could reasonably be done as a separate follow-up patch. The gist linked in the changelog shows the author has validated with examples externally.

4. **Schema cross-validation:** The `oneOf` on `reg`/`reg-names` and the `if`/`then`/`else` on counts are intentionally redundant — the `oneOf` constrains the valid layouts structurally, while the conditional constrains the counts per-compatible. Together they ensure that e.g. `ti,am62l-dss` with 8 registers would fail both the `maxItems: 5` check and (potentially) the `oneOf` matching. This belt-and-suspenders approach is good practice in dt-bindings.

**No functional issues found. The patch correctly fixes the AM62L DSS binding constraints.**

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-04-11 23:43 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-10 10:59 [PATCH v3] dt-bindings: display: ti, am65x-dss: Fix AM62L DSS reg and clock constraints Swamil Jain
2026-04-11 14:07 ` Krzysztof Kozlowski
2026-04-11 23:43 ` Claude review: " Claude Code Review Bot
2026-04-11 23:43 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox