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: Thu, 23 Apr 2026 08:20:38 +1000 Message-ID: In-Reply-To: <20260421172910.218497-4-biju.das.jz@bp.renesas.com> References: <20260421172910.218497-1-biju.das.jz@bp.renesas.com> <20260421172910.218497-4-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 This is the main driver patch. Several items to address: **1. Unnecessary includes** (`rzg3l_lvds.c` lines 9, 12, 16, 17, 27, 28): ```c #include #include #include #include ... #include #include ``` - `clk.h` =E2=80=94 no `clk_*` calls anywhere in the driver (clocks managed= by pm_runtime/syscon parent). - `io.h` =E2=80=94 no `ioread`/`iowrite`/`readl`/`writel` calls; registers = accessed via regmap. - `of_device.h` =E2=80=94 no `of_match_device()` or other `of_device` APIs = used. The sibling `rzg2l_mipi_dsi.c` doesn't include this. - `of_graph.h` =E2=80=94 no `of_graph_*` APIs called directly. - `drm_panel.h` =E2=80=94 no `drm_panel_*` functions called anywhere. - `drm_probe_helper.h` =E2=80=94 no probe helper functions called. Please drop these unused includes. **2. Register header: redundant and inconsistent definitions** (`rzg3l_lvds= _regs.h`): ```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) ``` `LVDS_CMN_RST_PHY0_SEL` and `LVDS_CMN_RST_PHY0_SEL_CH0` have the same value= and `_CH0` is never used. Drop `LVDS_CMN_RST_PHY0_SEL_CH0`. Also, the file mixes `(1 << N)` with `BIT(N)` (used for bits 8 and 9). Use = `BIT()` consistently throughout: ```c #define LVDS_CMN_RST_PHY0_SEL BIT(24) #define LVDS_CMN_PHY_RESET BIT(0) ``` Similarly: ```c #define LVDS_0_PHY_CH_IO_EN_MSK (0x1f) #define LVDS_0_PHY_CH_IO_EN (LVDS_0_PHY_CH_IO_EN_MSK << 0) ``` `LVDS_0_PHY_CH_IO_EN` is `0x1f << 0` which equals `LVDS_0_PHY_CH_IO_EN_MSK`= . These are the same value, and the `<< 0` is a no-op. Should be `GENMASK(4= , 0)` for the mask, and `LVDS_0_PHY_CH_IO_EN` can be dropped (just use the = mask directly, since the value written is the mask itself). **3. `pm_runtime_get_sync` error handling** (`rzg3l_lvds.c` lines 87-91): ```c ret =3D pm_runtime_get_sync(lvds->dev); if (ret < 0) { dev_err(lvds->dev, "pm_runtime_get_sync error\n"); return; } ``` The v2 changelog explains the switch from `pm_runtime_resume_and_get()` to = `pm_runtime_get_sync()` because "atomic_enable doesn't fail and for each en= able there always will be an atomic_disable() call." This rationale is vali= d =E2=80=94 the DRM framework always pairs enable/disable. However, `pm_run= time_get_sync()` increments the usage count even on failure. If it fails an= d `atomic_disable` later calls `pm_runtime_put`, the count rebalances but t= he hardware was never properly initialized, and `atomic_disable` will write= teardown register sequences to a device that never fully resumed. Consider either: - Using `pm_runtime_resume_and_get()` for cleaner semantics (the extra `pm_= runtime_put` in disable on failure path is harmless since it just decrement= s to 0), or - Dropping the error check entirely if this truly can't fail (since `atomic= _enable` returns void and can't propagate errors anyway). The current code is a hybrid that checks for errors but can't properly hand= le them. **4. Probe ordering** (`rzg3l_lvds.c` lines 242-254): ```c next_bridge =3D devm_drm_of_get_bridge(dev, dev->of_node, 1, 0); if (IS_ERR(next_bridge)) return dev_err_probe(dev, PTR_ERR(next_bridge), "failed to get next bridge\n"); lvds->bridge.next_bridge =3D next_bridge; ret =3D reset_control_assert(rstc); ... ret =3D reset_control_assert(arstc); ``` The `rst` and `arst` resets (MIPI_DSI_CMN_RSTB and MIPI_DSI_ARESET_N) must = be asserted as a precondition for LVDS operation. Currently they're asserte= d *after* `devm_drm_of_get_bridge`, which can return `-EPROBE_DEFER`. Consi= der moving the reset assertions earlier (right after obtaining them), so th= ey're asserted regardless of deferred probe. This avoids repeated assert/de= fer/assert cycles and ensures the hardware precondition is established as e= arly as possible. **5. Dead code in attach** (`rzg3l_lvds.c` lines 134-135): ```c if (!lvds->bridge.next_bridge) return 0; ``` Since `probe()` returns an error if `devm_drm_of_get_bridge()` fails, `next= _bridge` is always non-NULL by the time the bridge is registered and `attac= h` can be called. This check is dead code. It's a common defensive pattern = and not harmful, but worth noting. **6. Kconfig: `select DRM_PANEL` likely unnecessary**: ``` config DRM_RZG3L_LVDS def_tristate DRM_RZG2L_DU depends on DRM_RZG3L_USE_LVDS select DRM_KMS_HELPER select DRM_PANEL ``` The driver doesn't call any `drm_panel_*` functions. Panel support is handl= ed transparently through the bridge chain (via `devm_drm_of_get_bridge` whi= ch wraps panels as bridges). `select DRM_PANEL` and the `drm_panel.h` inclu= de appear to be unnecessary. `select DRM_KMS_HELPER` is also already select= ed by `DRM_RZG2L_DU` (which this depends on transitively via `def_tristate = DRM_RZG2L_DU`), though being explicit isn't wrong. **7. Minor: `return ret` at end of probe** (`rzg3l_lvds.c` line 261): ```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; ``` At this point `ret` is always 0. Clearer to write `return 0;`. **Summary for Patch 3:** The driver is functionally correct and follows est= ablished patterns. The main items to address are: cleaning up the unused in= cludes, making the register definitions consistent (use `BIT()`/`GENMASK()`= , drop redundant defs), and reconsidering the `pm_runtime_get_sync` error h= andling approach. The probe ordering of reset assertions could also be impr= oved. --- Generated by Claude Code Patch Reviewer