From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: arm64: dts: qcom: lemans: add mdss1 display device nodes Date: Fri, 27 Feb 2026 12:26:29 +1000 Message-ID: In-Reply-To: <20260226111322.250176-2-quic_mkuntuma@quicinc.com> References: <20260226111322.250176-1-quic_mkuntuma@quicinc.com> <20260226111322.250176-2-quic_mkuntuma@quicinc.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review This patch does three things: (1) moves the DP and MDP OPP tables from insi= de mdss0 child nodes to root level for sharing, (2) adds the full mdss1 nod= e hierarchy, and (3) wires up the dispcc1 clock controller to use the new m= dss1 PHY clocks and removes its `status =3D "disabled"`. **OPP table refactoring (good)** The OPP tables are moved out and renamed: - `mdss0_mdp_opp_table` =E2=86=92 `mdss_mdp_opp_table` (root level) - `dp_opp_table` stays at root level (was inside mdss0_dp0) - `dp1_opp_table` (was inside mdss0_dp1) is removed; mdss0_dp1 now shares `= dp_opp_table` This is clean. The OPP values are identical across all DP controllers and b= oth MDPs, so sharing makes sense. ``` - operating-points-v2 =3D <&mdss0_mdp_opp_table>; + operating-points-v2 =3D <&mdss_mdp_opp_table>; ``` ``` - operating-points-v2 =3D <&dp1_opp_table>; + operating-points-v2 =3D <&dp_opp_table>; ``` **mdss1 node structure (correct)** The mdss1 node mirrors mdss0 closely: - Base address `0x22000000` (vs mdss0's `0x0ae00000`) - GIC_SPI 865 (vs mdss0's 92) - IOMMU SID `0x1800` (vs mdss0's `0x1000`) - Uses `dispcc1` (vs `dispcc0`), `GCC_DISP1_HF_AXI_CLK` (vs `GCC_DISP_HF_AX= I_CLK`) - Interconnect masters `MASTER_MDP_CORE1_0`/`MASTER_MDP_CORE1_1` (vs `MASTE= R_MDP0`/`MASTER_MDP1`) The DPU, DP controllers, and PHY nodes all follow the mdss0 pattern with th= e correct address offsets for the second instance. **PHY "ref" clock inconsistency** The mdss1 PHY nodes include a third clock not present in the upstream mdss0= PHYs: ``` + clocks =3D <&dispcc1 MDSS_DISP_CC_MDSS_DPTX0_AUX_CLK>, + <&dispcc1 MDSS_DISP_CC_MDSS_AHB_CLK>, + <&gcc GCC_EDP_REF_CLKREF_EN>; + clock-names =3D "aux", + "cfg_ahb", + "ref"; ``` The current upstream mdss0 PHY nodes only have "aux" and "cfg_ahb". If the = dependency series adds this "ref" clock to mdss0 PHYs as well, this is fine= . If not, please either add it to mdss0 in this series or explain the diffe= rence. **DPU port numbering (correct)** ``` + port@0 { + reg =3D <0>; + dpu1_intf0_out: endpoint { + remote-endpoint =3D <&mdss1_dp0_in>; + }; + }; + + port@1 { + reg =3D <1>; + dpu1_intf4_out: endpoint { + remote-endpoint =3D <&mdss1_dp1_in>; + }; + }; ``` Port 0 (intf0) =E2=86=92 DP0 and port 1 (intf4) =E2=86=92 DP1 matches the m= dss0 DPU port numbering convention. Since mdss1 has no DSI, only ports 0 an= d 1 are defined. **dispcc1 changes (correct)** ``` - <0>, <0>, <0>, <0>, + <&mdss1_dp0_phy 0>, <&mdss1_dp0_phy 1>, + <&mdss1_dp1_phy 0>, <&mdss1_dp1_phy 1>, <0>, <0>, <0>, <0>; ``` Replacing the placeholder `<0>` entries for DP PHY clocks while leaving the= DSI PHY clock slots as `<0>` (no DSI on mdss1) is correct. This matches ho= w dispcc0 references mdss0 PHY clocks. ``` - status =3D "disabled"; ``` Removing `status =3D "disabled"` from dispcc1 makes it consistent with disp= cc0 (which has no status property and is implicitly enabled). **Minor style nit**: There is an extra blank line before the closing `};` o= f the `mdss1_dp1` node that isn't present in the `mdss1_dp0` node or any of= the mdss0 DP nodes: ``` + }; + + }; + }; ``` This is cosmetic but inconsistent. --- Generated by Claude Code Patch Reviewer