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: add support for lontium lt9211c bridge Date: Wed, 25 Mar 2026 08:04:41 +1000 Message-ID: In-Reply-To: <20260323-add-lt9211c-bridge-v5-2-9c63bb035c17@oss.qualcomm.com> References: <20260323-add-lt9211c-bridge-v5-0-9c63bb035c17@oss.qualcomm.com> <20260323-add-lt9211c-bridge-v5-2-9c63bb035c17@oss.qualcomm.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 numerous problems: **1. Broken indentation in `lt9211_atomic_enable()` (Critical)** The LT9211 (non-C) code path has wrong indentation =E2=80=94 it looks like = it was meant to be inside an `else` block but isn't: ```c if (ctx->chip_type =3D=3D 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 =3D lt9211_system_init(ctx); if (ret) return; ret =3D lt9211_configure_rx(ctx); if (ret) return; ``` The code after the `if` block is indented with two tabs instead of one. Whi= le this still compiles and functions correctly (the `return` in the if-bloc= k prevents fall-through being an issue), the indentation is wrong and misle= ading. It should use single-tab indentation matching the original code. Sam= e problem occurs for `lt9211_configure_tx`: ```c ret =3D 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 =E2= =80=94 this is inconsistent and the `if (ret)` check is at the wrong indent= ation level. **2. Same broken indentation in `lt9211_delayed_work_func()` (Critical)** ```c if (ctx->chip_type !=3D LT9211C) { dev_err(ctx->dev, "LT9211: Delayed work called for non-LT9211C chip\n"); return; } ret =3D 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 existi= ng `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 =E2=80=94 use `alloc_ordered_workqueue= ()` or just use `system_wq` with `schedule_delayed_work()`. There's no reas= on 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 "Initia= lize 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 conditi= on =E2=80=94 what if 100ms is not enough? There should be a proper synchron= ization 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` wh= ich are never set =E2=80=94 they remain at their zero-initialized values. T= he 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 =3D regmap_write(ctx->regmap, 0x863f, (pval & 0xf8)); ... ret =3D regmap_write(ctx->regmap, 0x863f, 0x05); ``` Register `0x863f` is written with a masked value of `pval` from register `0= x8680`, then immediately overwritten with `0x05`. The first write is pointl= ess. **8. Suspicious bitmask in `lt9211c_configure_rx()`** ```c ret =3D regmap_write(ctx->regmap, 0x8530, ((pval & 0xf8) | 0x11)); ``` `0xf8` masks off bits [2:0], then `0x11` (binary `0001_0001`) sets bits 0 a= nd 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 m= ask and then set =E2=80=94 this looks intentional but confusing. The same r= egister is later written differently in `lt9211c_configure_tx()` with `(pva= l & 0x3f) | 0x40`. **9. PLL computation bug** ```c /* 0xd026: pcr_m */ ret =3D regmap_write(ctx->regmap, 0xd026, (0x80 | (u8)pcr_m) & 0x7f); ``` `0x80 | x` sets bit 7, then `& 0x7f` clears bit 7. This is equivalent to ju= st `(u8)pcr_m & 0x7f` =E2=80=94 the `0x80 |` is dead code and suggests a co= py-paste error from a vendor SDK. **10. Error handling inconsistency** The code uses `ret |=3D regmap_write(...)` pattern in several places within= `lt9211c_configure_plls()`: ```c ret =3D regmap_write(ctx->regmap, 0x822f, 0x07); ret |=3D 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=3D%d,even_odd_swap=3D%d,bpp24=3D%d,jeida=3D%d,de=3D%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 =3D of_graph_get_endpoint_by_regs(dev->of_node, 0, -1); ``` This comment describes nothing =E2=80=94 it's a leftover with no correspond= ing code. **13. Unnecessary duplication in `lt9211_host_attach()`** The `mipi_dsi_device_info` differs only in `.type`. This could be simplifie= d with a ternary: ```c const struct mipi_dsi_device_info info =3D { .type =3D ctx->chip_type =3D=3D LT9211C ? "lt9211c" : "lt9211", .channel =3D 0, .node =3D NULL, }; ``` **14. Blank line removed between mode assignment and chipid read** ```c mode =3D &crtc_state->adjusted_mode; - ret =3D lt9211_read_chipid(ctx); ``` This removes a logical separator that existed in the original code for read= ability. **15. Redundant `chip_type` initialization** ```c ctx->chip_type =3D LT9211; ``` in `lt9211_probe()`, followed later by: ```c if (of_device_is_compatible(dev->of_node, "lontium,lt9211c")) { ctx->chip_type =3D LT9211C; ``` The `devm_drm_bridge_alloc` zero-initializes the struct, and `LT9211 =3D 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 i= mplementation). **17. `atomic_disable` doesn't cancel the delayed work** If `atomic_disable` is called while the delayed work is still pending (with= in 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, m= issing LVDS format propagation to LT9211C, workqueue misuse, and missing wo= rk cancellation on disable are the most serious problems. --- Generated by Claude Code Patch Reviewer