Claude review: drm/bridge: add support for lontium lt9211c bridge

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

Patch Review

This patch has numerous problems:

**1. Broken indentation in `lt9211_atomic_enable()` (Critical)**

The LT9211 (non-C) code path has wrong indentation — it looks like it was meant to be inside an `else` block but isn't:

```c
	if (ctx->chip_type == LT9211C && ctx->wq) {
		drm_mode_copy(&ctx->mode, mode);
		/* LT9211C must enable after mipi clock enable */
		queue_delayed_work(ctx->wq, &ctx->lt9211_dw,
				   msecs_to_jiffies(100));
		dev_dbg(ctx->dev, "LT9211C enabled.\n");
		return;
	}
		ret = lt9211_system_init(ctx);
		if (ret)
			return;

		ret = lt9211_configure_rx(ctx);
		if (ret)
			return;
```

The code after the `if` block is indented with two tabs instead of one. While this still compiles and functions correctly (the `return` in the if-block prevents fall-through being an issue), the indentation is wrong and misleading. It should use single-tab indentation matching the original code. Same problem occurs for `lt9211_configure_tx`:

```c
	ret = lt9211_configure_tx(ctx, lvds_format_jeida, lvds_format_24bpp,
					  bus_flags & DRM_BUS_FLAG_DE_HIGH);
		if (ret)
			return;
```

Here the `if (ret)` is double-indented but `lt9211_configure_tx` is not — this is inconsistent and the `if (ret)` check is at the wrong indentation level.

**2. Same broken indentation in `lt9211_delayed_work_func()` (Critical)**

```c
	if (ctx->chip_type != LT9211C) {
		dev_err(ctx->dev, "LT9211: Delayed work called for non-LT9211C chip\n");
		return;
	}
		ret = lt9211c_configure_rx(ctx);
		if (ret)
			return;
```

Code after the guard clause is double-indented. Should be single-tab.

**3. Gratuitous whitespace changes (Style)**

The patch changes alignment of existing struct members and reformats existing `dev_warn` calls that are unrelated to the functional change:

```c
-	struct mipi_dsi_device		*dsi;
+	struct mipi_dsi_device	*dsi;
...
-	bool				lvds_dual_link;
-	bool				lvds_dual_link_even_odd_swap;
+	bool					lvds_dual_link;
+	bool					lvds_dual_link_even_odd_swap;
```

These are inconsistent with each other and with existing code style. Don't change alignment of existing fields.

**4. Workqueue usage is wrong (Design)**

- `create_workqueue()` is deprecated — use `alloc_ordered_workqueue()` or just use `system_wq` with `schedule_delayed_work()`. There's no reason for a private workqueue here.
- The workqueue is created unconditionally for both LT9211 and LT9211C in `lt9211_probe()`, even though only LT9211C uses it. The comment says "Initialize LT9211C-specific fields" but doesn't check `chip_type` first.
- Using a workqueue to delay initialization by 100ms because "LT9211C must enable after mipi clock enable" is a fragile design. This is a race condition — what if 100ms is not enough? There should be a proper synchronization mechanism or at minimum a poll for a ready status.

**5. `lvds_format_jeida`/`lvds_format_24bpp`/`de` not set for LT9211C path (Bug)**

In `lt9211_atomic_enable()`, the LT9211C path returns early before the bus format switch/case determines `lvds_format_jeida` and `lvds_format_24bpp`. The delayed work function uses `ctx->bpp24`, `ctx->jeida`, and `ctx->de` which are never set — they remain at their zero-initialized values. The bus format negotiation result is lost.

**6. `lt9211c_configure_tx` uses `ctx->de` without it being set (Bug)**

```c
	{ 0x856e, 0x10 | (ctx->de ? BIT(6) : 0) },
	{ 0x856f, 0x81 | (ctx->jeida ? BIT(6) : 0) |
			  (ctx->lvds_dual_link ? BIT(4) : 0) |
			  (ctx->bpp24 ? BIT(2) : 0) },
```

None of `ctx->de`, `ctx->jeida`, or `ctx->bpp24` are populated anywhere in the code path. They're always false/0.

**7. Register write logic bug in `lt9211c_configure_rx()`**

```c
	ret = regmap_write(ctx->regmap, 0x863f, (pval & 0xf8));
	...
	ret = regmap_write(ctx->regmap, 0x863f, 0x05);
```

Register `0x863f` is written with a masked value of `pval` from register `0x8680`, then immediately overwritten with `0x05`. The first write is pointless.

**8. Suspicious bitmask in `lt9211c_configure_rx()`**

```c
	ret = regmap_write(ctx->regmap, 0x8530, ((pval & 0xf8) | 0x11));
```

`0xf8` masks off bits [2:0], then `0x11` (binary `0001_0001`) sets bits 0 and 4. Bit 4 isn't cleared by the `0xf8` mask, so the OR with `0x11` on bit 4 would only matter if bit 4 was already 0. But bit 0 *is* cleared by the mask and then set — this looks intentional but confusing. The same register is later written differently in `lt9211c_configure_tx()` with `(pval & 0x3f) | 0x40`.

**9. PLL computation bug**

```c
	/* 0xd026: pcr_m */
	ret = regmap_write(ctx->regmap, 0xd026, (0x80 | (u8)pcr_m) & 0x7f);
```

`0x80 | x` sets bit 7, then `& 0x7f` clears bit 7. This is equivalent to just `(u8)pcr_m & 0x7f` — the `0x80 |` is dead code and suggests a copy-paste error from a vendor SDK.

**10. Error handling inconsistency**

The code uses `ret |= regmap_write(...)` pattern in several places within `lt9211c_configure_plls()`:

```c
		ret = regmap_write(ctx->regmap, 0x822f, 0x07);
		ret |= regmap_write(ctx->regmap, 0x822c, 0x01);
```

This ORs error codes together, which loses the original error value and can mask errors. Use proper sequential error checking.

**11. `dev_info` in `lt9211c_configure_tx()` should be `dev_dbg`**

```c
	dev_info(ctx->dev,
		 "dual_link=%d,even_odd_swap=%d,bpp24=%d,jeida=%d,de=%d\n",
```

This prints every time the bridge is enabled. Should be `dev_dbg`.

**12. Stale comment left in `lt9211_host_attach()`**

```c
	/* Check if the compatible string matches lt9211c */

	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
```

This comment describes nothing — it's a leftover with no corresponding code.

**13. Unnecessary duplication in `lt9211_host_attach()`**

The `mipi_dsi_device_info` differs only in `.type`. This could be simplified with a ternary:

```c
	const struct mipi_dsi_device_info info = {
		.type = ctx->chip_type == LT9211C ? "lt9211c" : "lt9211",
		.channel = 0,
		.node = NULL,
	};
```

**14. Blank line removed between mode assignment and chipid read**

```c
	mode = &crtc_state->adjusted_mode;
-
	ret = lt9211_read_chipid(ctx);
```

This removes a logical separator that existed in the original code for readability.

**15. Redundant `chip_type` initialization**

```c
	ctx->chip_type = LT9211;
```

in `lt9211_probe()`, followed later by:

```c
	if (of_device_is_compatible(dev->of_node, "lontium,lt9211c")) {
		ctx->chip_type = LT9211C;
```

The `devm_drm_bridge_alloc` zero-initializes the struct, and `LT9211 = 0`, so the explicit initialization is redundant (though harmless).

**16. `lt9211c_regmap_config` uses `REGCACHE_RBTREE`**

The original LT9211 uses `REGCACHE_MAPLE`. The LT9211C variant should also use `REGCACHE_MAPLE` for consistency (RBTREE is the older, less efficient implementation).

**17. `atomic_disable` doesn't cancel the delayed work**

If `atomic_disable` is called while the delayed work is still pending (within the 100ms window), the work will fire after the chip is put into reset, causing register writes to a reset chip. Need `cancel_delayed_work_sync()` in `lt9211_atomic_disable()`.

**Summary**: This patch needs significant rework. The indentation issues, missing LVDS format propagation to LT9211C, workqueue misuse, and missing work cancellation on disable are the most serious problems.

---
Generated by Claude Code Patch Reviewer