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: Mon, 09 Mar 2026 07:56:46 +1000
Message-ID: <review-patch4-20260308064835.479356-5-mitltlatltl@gmail.com>
In-Reply-To: <20260308064835.479356-5-mitltlatltl@gmail.com>
References: <20260308064835.479356-1-mitltlatltl@gmail.com> <20260308064835.479356-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

This is the main patch, adding 4 DSI controllers (2 per MDSS) and 4 DSI PHYs, plus wiring up the dispcc clock inputs and DPU output ports.

**Bug: `mdss1_dsi0` has mismatched `assigned-clock-parents`**

At lines 1054-1055 of the mbox (corresponding to sc8280xp.dtsi lines 6399-6400):

```dts
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` but the pixel clock parent references `mdss1_dsi0_phy`. Comparing with all other DSI controller nodes in this patch:

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

This is almost certainly a copy-paste error. The byte clock parent should be `mdss1_dsi0_phy`, not `mdss1_dsi1_phy`:

```dts
assigned-clock-parents = <&mdss1_dsi0_phy DSI_BYTE_PLL_CLK>,
                         <&mdss1_dsi0_phy DSI_PIXEL_PLL_CLK>;
```

**Other observations (non-blocking):**

- The `dsi_opp_table` is defined inside `mdss0_dsi0` and referenced by all four DSI controllers including the mdss1 ones. This is a valid approach via phandle cross-referencing and was explicitly requested by Konrad in v2 review. It works but is slightly unusual to have mdss1 nodes reference a table nested inside an mdss0 child.
- The commit message honestly notes that "DSI won't work properly for now until we submit dispcc fixes" - this is good transparency, and the nodes default to `status = "disabled"`.
- The whitespace-only changes (adding blank lines between `reg` property and label in existing `port@` nodes) are reasonable DT style cleanup.
- The `#include <dt-bindings/clock/qcom,dsi-phy-28nm.h>` is correct despite the "28nm" in the name - this header simply defines the generic `DSI_BYTE_PLL_CLK`/`DSI_PIXEL_PLL_CLK` indices used across all DSI PHY generations.

---
Generated by Claude Code Patch Reviewer