From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/msm/dsi: Support dual panel use case with single CRTC Date: Mon, 25 May 2026 20:00:02 +1000 Message-ID: In-Reply-To: <20260521-sm8650-7-1-bonded-dsi-v4-3-a4dd5e0850f1@linaro.org> References: <20260521-sm8650-7-1-bonded-dsi-v4-0-a4dd5e0850f1@linaro.org> <20260521-sm8650-7-1-bonded-dsi-v4-3-a4dd5e0850f1@linaro.org> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Concept**: When `is_dual_panel` is set, halves `dsc->pic_width` and `pic_= width` in check_dsc so the DSI host configures DSC for a single panel's wid= th. **Issues:** 1. **Correct fix for `dsc->pic_width`**: In `dsi_timing_setup()`, the exist= ing code (line 1033 upstream) sets `dsc->pic_width =3D mode->hdisplay`, whi= ch is indeed wrong for bonded DSI since `mode->hdisplay` is the full double= d width. The patch changes this to use `hdisplay` (the local variable), whi= ch is correct for bonded DSI (`hdisplay` was already halved on line 1019). = However, the patch uses `msm_host->is_dual_panel` to decide, while the exis= ting `is_bonded_dsi` parameter already covers this case. This creates two o= verlapping concepts: - `is_bonded_dsi`: the existing bonded DSI path (already halves timings) - `is_dual_panel`: the new dual-panel path =20 The patch only sets `dsc->pic_width =3D hdisplay` when `is_dual_panel` i= s true, but `hdisplay` has already been halved for `is_bonded_dsi` as well.= So for the existing bonded-DSI-with-single-panel case, `dsc->pic_width` is= still set to `mode->hdisplay` (the full width), which appears to be a pre-= existing bug that this patch doesn't fix. The relationship between these tw= o flags needs clarification. 2. **`msm_dsi_host_check_dsc` fix is incomplete**: The patch adds: ```c if (msm_host->is_dual_panel) pic_width =3D mode->hdisplay / 2; ``` But `pic_width` was already initialized to `mode->hdisplay` on the previ= ous line. The fix works but note that `is_bonded_dsi` (the existing bonded = DSI flag, not dual_panel) also requires halving for the pre-existing single= -panel bonded case. This function currently has no concept of `is_bonded_ds= i` at all, which may be an existing limitation. 3. **Commit message typo**: "togehter" =E2=86=92 "together" in line "While = the DSI panel driver should manage two panels togehter." --- Generated by Claude Code Patch Reviewer