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: Mon, 25 May 2026 16:56:34 +1000 Message-ID: In-Reply-To: <20260524194457.479681-3-biju.das.jz@bp.renesas.com> References: <20260524194457.479681-1-biju.das.jz@bp.renesas.com> <20260524194457.479681-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 **Issues:** **(1) `rst` and `arst` reset controls are fire-and-forget (medium severity)= ** In `rzg3l_lvds_probe()`, the `rstc` and `arstc` resets are acquired, assert= ed, and then the local variables go out of scope. They are never stored in = `struct rzg3l_lvds`, so they can never be deasserted: ```c struct reset_control *rstc, *arstc; ... rstc =3D devm_reset_control_get_optional_exclusive(dev, "rst"); ... arstc =3D devm_reset_control_get_optional_exclusive(dev, "arst"); ... ret =3D reset_control_assert(rstc); ... ret =3D reset_control_assert(arstc); ``` Per the cover letter, these resets "must be asserted before using the LVDS = module" and should stay asserted. If this is intentional (assert once, neve= r deassert), it works because `devm_*` will release the reset control on dr= iver remove, but a comment would clarify the intent. Additionally, using `_= exclusive` reset controls that are only asserted and never deasserted is un= usual =E2=80=94 if they truly must stay asserted for the lifetime of the de= vice, consider whether `_shared` would be more appropriate if the DSI drive= r also needs to manipulate them. **(2) `pm_runtime_get_sync` error path leaks PM refcount (medium severity)** In `rzg3l_lvds_atomic_enable()`: ```c if (WARN_ON(pm_runtime_get_sync(lvds->dev) < 0)) return; ``` `pm_runtime_get_sync()` increments the reference count even on failure. On = error, you must call `pm_runtime_put_noidle()` or `pm_runtime_put()` to avo= id a PM refcount leak. The matching `pm_runtime_put()` in `rzg3l_lvds_atomi= c_disable()` will never be called if enable bailed out. Consider: ```c if (WARN_ON(pm_runtime_get_sync(lvds->dev) < 0)) { pm_runtime_put_noidle(lvds->dev); return; } ``` Or alternatively, use `pm_runtime_resume_and_get()` which doesn't increment= the refcount on failure. **(3) Unused includes (minor)** Several headers appear unnecessary: - `linux/of_device.h` =E2=80=94 nothing from this header is used (no `of_de= vice_get_match_data()` etc.) - `linux/of_graph.h` =E2=80=94 no `of_graph_*` functions are called - `linux/bitfield.h` =E2=80=94 wait, `FIELD_PREP` is used, so this one is n= eeded - `linux/mfd/syscon.h` =E2=80=94 `syscon_node_to_regmap()` is used, so need= ed - `linux/regmap.h` =E2=80=94 `regmap_*` functions used, needed - `linux/media-bus-format.h` =E2=80=94 `MEDIA_BUS_FMT_*` used, needed - `drm/drm_panel.h` =E2=80=94 no panel functions called directly, but `DRM_= PANEL` is selected in Kconfig. This include is not needed in this source fi= le. - `drm/drm_probe_helper.h` =E2=80=94 no probe helper functions called. **(4) Register header mixes `BIT()` and raw shifts (minor style)** In `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) ``` These should use `BIT()` for consistency with the rest of the file which us= es `BIT(8)`, `BIT(9)`, `GENMASK()`: ```c #define LVDS_0_PHY_CH_EN_BGR BIT(8) #define LVDS_0_PHY_CH_EN_LDO BIT(9) ``` Also, `LVDS_CMN_RST_PHY0_SEL` and `LVDS_CMN_RST_PHY0_SEL_CH0` are defined t= o the same value `(1 << 24)` =E2=80=94 one of them appears redundant. **(5) `LVDS_0_PHY_CH_IO_EN` is redundant (minor)** ```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 just `LVDS_0_PHY_CH_IO_EN_MSK << 0`, which is the = same value. Having both is confusing. This could be simplified, or at least= the `<< 0` should be removed. **(6) `select DRM_PANEL` in Kconfig may be unnecessary (minor)** The driver doesn't use any panel API directly =E2=80=94 it uses `devm_drm_o= f_get_bridge()` which handles panel wrapping via `drm_panel_bridge`. The `D= RM_PANEL` symbol is typically pulled in transitively by the bridge infrastr= ucture. Check whether `DRM_PANEL` is really needed here. **(7) `select DRM_KMS_HELPER` =E2=80=94 check necessity (minor)** Similar to above, `DRM_KMS_HELPER` is often pulled in by `DRM_BRIDGE`. Veri= fy it's needed as a direct dependency. **(8) The `return ret;` at the end of probe could be `return 0;` (nitpick)** ```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 (the error case already returned). Using `r= eturn 0;` is clearer. **(9) Version numbering confusion (meta)** The cover letter changelog says "v2->v3" but the subject lines say `[PATCH = v2 0/2]`. This appears to actually be v3 based on the changelog content. **(10) Bridge attach returns 0 when there's no next_bridge (needs verificat= ion)** ```c static int rzg3l_lvds_attach(...) { 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); } ``` The `next_bridge` is set from `devm_drm_of_get_bridge()` in probe, which re= turns an error if not found. So `next_bridge` should always be non-NULL if = probe succeeded. This NULL check is harmless defensive code but shouldn't b= e reachable. Not a bug, but the check is dead code given the current probe = logic. --- Generated by Claude Code Patch Reviewer