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/mediatek: Convert legacy DRM logging to drm_* helpers in mtk_dsi.c Date: Thu, 23 Apr 2026 10:21:29 +1000 Message-ID: In-Reply-To: <20260420052008.5417-1-abhiraj21put@gmail.com> References: <20260420052008.5417-1-abhiraj21put@gmail.com> <20260420052008.5417-1-abhiraj21put@gmail.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 **Commit message error:** > This change aligns the radeon driver with the DRM TODO item This should say "mediatek driver", not "radeon driver". Copy-paste error. **Bug: Declaration after statement (build break)** At `mtk_dsi_switch_to_cmd_mode()`, the new variable is declared after execu= table statements: ```c static s32 mtk_dsi_switch_to_cmd_mode(struct mtk_dsi *dsi, u8 irq_flag, u32= t) { mtk_dsi_irq_data_clear(dsi, irq_flag); mtk_dsi_set_cmd_mode(dsi); struct drm_device *drm =3D dsi->bridge.dev; // <-- after statements ``` The declaration must be moved to the top of the function, before the two fu= nction calls. This will fail to compile with `-Wdeclaration-after-statement= -Werror`. **Bug: NULL `bridge.dev` in `mtk_dsi_host_attach()` (potential NULL deref)** ```c static int mtk_dsi_host_attach(struct mipi_dsi_host *host, struct mipi_dsi_device *device) { struct mtk_dsi *dsi =3D host_to_dsi(host); struct device *dev =3D host->dev; struct drm_device *drm =3D dsi->bridge.dev; // <-- NULL here ... drm_bridge_add(&dsi->bridge); // does NOT set bridge.dev ret =3D component_add(host->dev, &mtk_dsi_component_ops); if (ret) { drm_err(drm, ...); // passes NULL drm_device ``` `bridge.dev` is set by `drm_bridge_attach()` (called inside `mtk_dsi_encode= r_init()` which runs during `mtk_dsi_bind()`, triggered by the component fr= amework *after* `component_add()`). At the point where `drm` is captured, i= t is NULL. The `drm_err(drm, ...)` call with a NULL first argument will cra= sh. This call site should either keep using `DRM_ERROR()` or use `dev_err(h= ost->dev, ...)` instead. **Similarly suspect: `mtk_dsi_host_transfer()` and `mtk_dsi_wait_for_idle()= `** These functions also access `dsi->bridge.dev`. While `bridge.dev` is likely= valid during normal runtime (the bridge should be attached by then), it's = worth verifying that these code paths cannot be reached before the bridge i= s fully attached, or during teardown after `drm_bridge_detach()` has cleare= d `bridge.dev` to NULL. **Minor: trailing `\n` in drm_* calls** The `drm_*()` helpers already append a newline. The existing `DRM_*()` macr= os also append a newline, so the `\n` in the format strings was always redu= ndant (producing double newlines). While this patch is just a mechanical co= nversion and isn't introducing the issue, it would be nice to clean up the = trailing `\n` as part of this conversion. **Correct conversions:** The conversions in `mtk_dsi_config_vdo_timing_per_line_lp()`, `mtk_dsi_wait= _for_irq_done()`, `mtk_dsi_bridge_atomic_pre_enable()`, and `mtk_dsi_encode= r_init()` all look correct =E2=80=94 `dsi->bridge.dev` or `bridge->dev` is = valid in those contexts. --- Generated by Claude Code Patch Reviewer