From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: arm64: dts: qcom: sc8280xp: Add dsi nodes on SC8280XP
Date: Tue, 03 Mar 2026 14:28:32 +1000
Message-ID: <review-patch4-20260228101907.18043-5-mitltlatltl@gmail.com>
In-Reply-To: <20260228101907.18043-5-mitltlatltl@gmail.com>
References: <20260228101907.18043-1-mitltlatltl@gmail.com> <20260228101907.18043-5-mitltlatltl@gmail.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

**Status: Needs fixes - has critical errors**

This is the main patch adding DSI controller and PHY nodes for both MDSS instances (mdss0 and mdss1), each with two DSI controllers and two DSI PHYs (4 controllers + 4 PHYs total). It also wires up the dispcc clock sources properly.

#### Bug 1 (Critical): Missing comma in all PHY compatible strings

All four PHY nodes have a missing comma between the two compatible strings:

```dts
+			mdss0_dsi0_phy: phy@ae94400 {
+				compatible = "qcom,sc8280xp-dsi-phy-5nm"
+					     "qcom,sa8775p-dsi-phy-5nm";
```

This appears identically in `mdss0_dsi0_phy`, `mdss0_dsi1_phy`, `mdss1_dsi0_phy`, and `mdss1_dsi1_phy`. Without the comma, dtc will concatenate the two strings into a single nonsensical compatible string `"qcom,sc8280xp-dsi-phy-5nmqcom,sa8775p-dsi-phy-5nm"`, which won't match any driver.

It should be:
```dts
				compatible = "qcom,sc8280xp-dsi-phy-5nm",
					     "qcom,sa8775p-dsi-phy-5nm";
```

Note: the DSI controller nodes have the comma correctly:
```dts
+				compatible = "qcom,sc8280xp-dsi-ctrl",
+					     "qcom,sa8775p-dsi-ctrl",
+					     "qcom,mdss-dsi-ctrl";
```

So this is clearly an oversight in the PHY nodes only.

#### Bug 2 (Likely copy-paste error): mdss1_dsi0 assigned-clock-parents mismatch

The `mdss1_dsi0` node has inconsistent clock parent PHY references:

```dts
+			mdss1_dsi0: dsi@22094000 {
+				...
+				assigned-clock-parents = <&mdss1_dsi1_phy DSI_BYTE_PLL_CLK>,
+							 <&mdss1_dsi0_phy DSI_PIXEL_PLL_CLK>;
```

The byte clock parent references `mdss1_dsi1_phy` (DSI **1** PHY) while the pixel clock parent references `mdss1_dsi0_phy` (DSI **0** PHY). Comparing with the other three controllers:

- `mdss0_dsi0`: both from `mdss0_dsi0_phy` -- consistent
- `mdss0_dsi1`: both from `mdss0_dsi1_phy` -- consistent
- `mdss1_dsi0`: byte from `mdss1_dsi1_phy`, pixel from `mdss1_dsi0_phy` -- **inconsistent**
- `mdss1_dsi1`: both from `mdss1_dsi1_phy` -- consistent

This looks like a copy-paste error. It should likely be:
```dts
				assigned-clock-parents = <&mdss1_dsi0_phy DSI_BYTE_PLL_CLK>,
							 <&mdss1_dsi0_phy DSI_PIXEL_PLL_CLK>;
```

#### Observation: 28nm clock header for 5nm PHY

```diff
+#include <dt-bindings/clock/qcom,dsi-phy-28nm.h>
```

This is technically correct -- `DSI_BYTE_PLL_CLK` and `DSI_PIXEL_PLL_CLK` are generic clock index defines that live in the 28nm header and are reused across all PHY generations. Other recent SoC dtsi files (SA8775P, etc.) do the same. But it could confuse future readers given the PHY is 5nm.

#### Observation: Shared OPP table placement

The `dsi_opp_table` is defined inside the `mdss0_dsi0` node but referenced by all four DSI controllers via phandle. This works functionally but is slightly unusual -- typically shared tables are placed at a common parent level. This was apparently requested by Konrad in v1 review, so it's intentional.

#### Observation: Style-only blank line additions

The patch adds blank lines after `reg = <N>;` in several existing port nodes (port@0, port@4, port@5, port@6 in both MDSS instances). This improves style consistency with the new port nodes but mixes style cleanup with functional changes. Likely acceptable since it was probably requested in v1 review.

#### Overall patch 4 structure

The DSI controller nodes are well-structured with correct interrupt numbers (4 for DSI0, 5 for DSI1), correct register addresses, proper clock assignments, and correct endpoint wiring. The dispcc clock source replacements from `<0>` stubs to actual PHY clock references are correct and necessary.

---

**Summary of required fixes:**
1. Add missing comma in all 4 PHY compatible strings
2. Fix `mdss1_dsi0` assigned-clock-parents to consistently reference `mdss1_dsi0_phy`

---
Generated by Claude Code Patch Reviewer