Claude review: drm: bridge: ti-sn65dsi83: Add support for dual-link LVDS video mode

[Claude Code Review Bot <claude-review@example.com>]

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 fixed constants to registers that were *already programmed* with values derived 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 = 0x53`, `REG_LVDS_FMT = 0x6f`) are specific to a particular panel resolution and pixel clock. They will not work for any other panel. This is essentially dumping a register configuration 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 via `regmap_bulk_write` from `mode->vdisplay`) with zero. Setting the vertical display size to 0 is almost certainly wrong for any panel and contradicts 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 divided by 2 for dual-link mode, but the code doesn't divide anything — 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 = (mode->hsync_end - mode->hsync_start) / (ctx->lvds_dual_link ? 2 : 1);
hbp = (mode->htotal - mode->hsync_end) / (ctx->lvds_dual_link ? 2 : 1);
```

**4. `REG_RC_LVDS_PLL` overwritten with `0x05` breaks the PLL clock range calculation.**

Earlier in `sn65dsi83_atomic_pre_enable()`, the driver computes the LVDS PLL register value using `sn65dsi83_get_lvds_range()` which selects the correct 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 harmless.

**6. DSI mode flags change is too broad.**

```c
+	if (ctx->dual_link_video_mode)
+		dsi->mode_flags = MIPI_DSI_MODE_VIDEO;
+	else
+		dsi->mode_flags = 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 with 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 =
+		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 dual-link improvement. The correct fix would modify the existing timing calculations in `sn65dsi83_atomic_pre_enable()` to divide horizontal timing parameters 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