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: renesas: rz-du: Add support for RZ/G3L LVDS encoder Date: Wed, 27 May 2026 15:11:41 +1000 Message-ID: In-Reply-To: <20260526074746.20371-3-biju.das.jz@bp.renesas.com> References: <20260526074746.20371-1-biju.das.jz@bp.renesas.com> <20260526074746.20371-3-biju.das.jz@bp.renesas.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 Overall a solid driver. A few items to flag: **1. Potential refcount issue with `bridge.next_bridge` assignment (medium = concern):** ```c lvds->bridge.next_bridge =3D devm_drm_of_get_bridge(dev, dev->of_node, 1, 0= ); ``` The `next_bridge` field is documented as "automatically put when this bridg= e is freed" =E2=80=94 i.e., `drm_bridge_put()` is called on it at destroy t= ime. However, `devm_drm_of_get_bridge()` returns either a devm-managed pane= l bridge (refcount 1, also devm-cleaned) or an unrefcounted bridge pointer = from `of_drm_find_bridge()`. Storing it directly into `next_bridge` without= calling `drm_bridge_get()` risks a refcount mismatch =E2=80=94 the auto-pu= t at bridge destruction would underflow. All existing in-tree drivers that store into `bridge.next_bridge` use `of_d= rm_find_and_get_bridge()`, `of_drm_get_bridge_by_endpoint()`, or `drm_bridg= e_get()` =E2=80=94 functions that increment the refcount. Drivers that use = `devm_drm_of_get_bridge()` store the result in a private struct field inste= ad (like `rzg2l_mipi_dsi.c` does with `dsi->next_bridge`). Consider either storing in a private `lvds->next_bridge` field (matching th= e pattern in `rzg2l_mipi_dsi.c`), or using `of_drm_get_bridge_by_endpoint()= ` with a `drm_bridge_get()` for the `bridge.next_bridge` assignment. **2. Unnecessary include:** ```c #include ``` This header is not needed =E2=80=94 the driver doesn't use any `of_device_*= ` functions. The sibling `rzg2l_mipi_dsi.c` driver only includes `` and ``. This include can be dropped. **3. Register header: inconsistent bit-field style (`rzg3l_lvds_regs.h`):** The header mixes `(1 << N)` with `BIT()`: ```c #define LVDS_CMN_RST_PHY0_SEL (1 << 24) #define LVDS_CMN_RST_PHY0_SEL_CH0 (1 << 24) #define LVDS_CMN_PHY_RESET (1 << 0) ... #define LVDS_0_PHY_CH_EN_BGR BIT(8) #define LVDS_0_PHY_CH_EN_LDO BIT(9) ``` Prefer `BIT()` throughout for consistency with kernel style. **4. Register header: redundant definitions:** `LVDS_CMN_RST_PHY0_SEL` and `LVDS_CMN_RST_PHY0_SEL_CH0` are both defined as= `(1 << 24)` =E2=80=94 they are identical. Only `LVDS_CMN_RST_PHY0_SEL` is = used in the driver; `LVDS_CMN_RST_PHY0_SEL_CH0` is dead code. Similarly, `LVDS_0_PHY_CH_IO_EN` is defined as `(LVDS_0_PHY_CH_IO_EN_MSK <<= 0)` which equals `LVDS_0_PHY_CH_IO_EN_MSK` =E2=80=94 the `<< 0` is a no-op= and the two defines have the same value. This is confusing since one is us= ed as a mask and the other as a value, but they're identical. Consider clar= ifying the intent or consolidating. **5. Kconfig: `select DRM_PANEL` and `select DRM_KMS_HELPER`:** ``` config DRM_RZG3L_LVDS def_tristate DRM_RZG2L_DU depends on DRM_RZG3L_USE_LVDS select DRM_KMS_HELPER select DRM_PANEL ``` `DRM_KMS_HELPER` is already selected by `DRM_RZG2L_DU` (through `DRM_GEM_DM= A_HELPER` dependencies). And `DRM_PANEL` =E2=80=94 verify this is actually = needed. The driver doesn't directly use `drm_panel_*` APIs; it uses `devm_d= rm_of_get_bridge()` which internally handles panels via panel-bridge. The `= DRM_PANEL` select may be needed transitively for `devm_drm_of_get_bridge()`= to find panels, but it would be worth confirming. **6. Minor: `return ret` at end of probe:** ```c ret =3D devm_drm_bridge_add(dev, &lvds->bridge); if (ret) return dev_err_probe(dev, ret, "Failed to register drm bridge\n"); return ret; ``` Since the error is already handled, `return ret` here is always `return 0`.= Could be simplified to `return 0` for clarity, though functionally identic= al. **7. The `attach` callback accesses `next_bridge` through the bridge struct= :** ```c static int rzg3l_lvds_attach(struct drm_bridge *bridge, struct drm_encoder *encoder, enum drm_bridge_attach_flags flags) { struct rzg3l_lvds *lvds =3D bridge_to_rzg3l_lvds(bridge); if (!lvds->bridge.next_bridge) return 0; return drm_bridge_attach(encoder, lvds->bridge.next_bridge, bridge, fla= gs); } ``` This is correct for the `bridge.next_bridge` approach, but note that return= ing 0 when there's no next bridge means the encoder pipeline would be incom= plete (no connector). This only works if the bridge connector infrastructur= e handles it. If you switch to a private `next_bridge` field per item 1 abo= ve, this would need adjustment too. **No issues with:** - The `drm_atomic_commit` type usage (matches current in-tree bridge callba= ck signatures) - The `pm_runtime_get_sync` + `WARN_ON` pattern (appropriate for atomic ena= ble which shouldn't fail) - The reset handling (optional `rst`/`arst` asserted-and-left, shared `prst= `/`lvdrst` toggled via PM runtime) - The `devm_drm_bridge_alloc` usage (correct modern API) - The mode_valid clock range checking (25-87 MHz matching the binding descr= iption) --- Generated by Claude Code Patch Reviewer