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: bridge: ti-sn65dsi83: Add support for dual-link LVDS video mode Date: Fri, 13 Mar 2026 14:32:11 +1000 Message-ID: In-Reply-To: <20260312043743.261475-3-tessolveupstream@gmail.com> References: <20260312043743.261475-1-tessolveupstream@gmail.com> <20260312043743.261475-3-tessolveupstream@gmail.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 has several serious issues: **1. Hardcoded magic values overwrite mode-derived calculations.** The entire block starting at the `if (ctx->dual_link_video_mode)` writes fi= xed constants to registers that were *already programmed* with values deriv= ed from the actual display mode timing just lines above: ```c + if (ctx->dual_link_video_mode) { + regmap_write(ctx->regmap, REG_RC_LVDS_PLL, 0x05); + regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00); + regmap_write(ctx->regmap, REG_DSI_CLK, 0x53); + regmap_write(ctx->regmap, REG_LVDS_FMT, 0x6f); + regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x00); ``` These hardcoded values (e.g., `REG_DSI_CLK =3D 0x53`, `REG_LVDS_FMT =3D 0x6= f`) are specific to a particular panel resolution and pixel clock. They wil= l not work for any other panel. This is essentially dumping a register conf= iguration from a tuning tool into the driver as if it were generic. **2. Vertical display size zeroed out.** ```c + regmap_write(ctx->regmap, + REG_VID_CHA_VERTICAL_DISPLAY_SIZE_LOW, 0x00); + regmap_write(ctx->regmap, + REG_VID_CHA_VERTICAL_DISPLAY_SIZE_HIGH, 0x00); ``` This overwrites the correctly computed vertical display size (set earlier v= ia `regmap_bulk_write` from `mode->vdisplay`) with zero. Setting the vertic= al display size to 0 is almost certainly wrong for any panel and contradict= s the earlier programming. **3. Horizontal timing values are also hardcoded, not "divided by 2".** The cover letter and commit message claim horizontal timing should be divid= ed by 2 for dual-link mode, but the code doesn't divide anything =E2=80=94 = it writes fixed constants: ```c + regmap_write(ctx->regmap, + REG_VID_CHA_HSYNC_PULSE_WIDTH_LOW, 0x10); + regmap_write(ctx->regmap, + REG_VID_CHA_HORIZONTAL_BACK_PORCH, 0x28); ``` The correct approach would be something like: ```c hsync_width =3D (mode->hsync_end - mode->hsync_start) / (ctx->lvds_dual_lin= k ? 2 : 1); hbp =3D (mode->htotal - mode->hsync_end) / (ctx->lvds_dual_link ? 2 : 1); ``` **4. `REG_RC_LVDS_PLL` overwritten with `0x05` breaks the PLL clock range c= alculation.** Earlier in `sn65dsi83_atomic_pre_enable()`, the driver computes the LVDS PL= L register value using `sn65dsi83_get_lvds_range()` which selects the corre= ct clock range for the display mode. Writing `0x05` overwrites this with a = fixed clock range that only works for one specific pixel clock. **5. `REG_RC_PLL_EN` written to `0x00` is redundant and confusing.** The PLL is already disabled at the start of `sn65dsi83_atomic_pre_enable()`= (line 588 in the existing code). Writing it again inside this block, only = to then enable it a few lines later outside the block, is confusing but har= mless. **6. DSI mode flags change is too broad.** ```c + if (ctx->dual_link_video_mode) + dsi->mode_flags =3D MIPI_DSI_MODE_VIDEO; + else + dsi->mode_flags =3D MIPI_DSI_MODE_VIDEO | + MIPI_DSI_MODE_VIDEO_BURST | ... ``` Stripping `MIPI_DSI_MODE_VIDEO_BURST`, `MIPI_DSI_MODE_NO_EOT_PACKET`, etc. = with no explanation of why dual-link mode requires non-burst DSI video mode= . The commit message doesn't justify this change, and it may cause issues w= ith DSI hosts that prefer burst mode. **7. `of_property_read_bool()` should be `device_property_read_bool()`.** Modern kernel style prefers `device_property_read_bool(dev, ...)` over `of_= property_read_bool(dev->of_node, ...)` for better abstraction: ```c + ctx->dual_link_video_mode =3D + of_property_read_bool(dev->of_node, "ti,dual-link-video-mode"); ``` **Summary:** The patch as written is a panel-specific hack, not a generic d= ual-link improvement. The correct fix would modify the existing timing calc= ulations in `sn65dsi83_atomic_pre_enable()` to divide horizontal timing par= ameters by 2 when `ctx->lvds_dual_link` is already true, without requiring = any new DT property or hardcoded register values. --- Generated by Claude Code Patch Reviewer