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/panel: Add BOE BF068MWM-TD0 panel driver Date: Thu, 04 Jun 2026 11:48:25 +1000 Message-ID: In-Reply-To: <20260603-asteroids-panel-support-v1-2-109c6ac81c8f@pm.me> References: <20260603-asteroids-panel-support-v1-0-109c6ac81c8f@pm.me> <20260603-asteroids-panel-support-v1-2-109c6ac81c8f@pm.me> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Bug: Missing cleanup on error paths in `prepare()`** The `boe_bf068mwm_td0_prepare()` function has resource leaks on two error p= aths. After `boe_bf068mwm_td0_on()` succeeds, regulators are enabled and th= e reset GPIO is deasserted. If `mipi_dsi_picture_parameter_set()` or `mipi_= dsi_compression_mode()` subsequently fails, the function returns without re= setting GPIO or disabling regulators: ```c ret =3D mipi_dsi_picture_parameter_set(ctx->dsi, &pps); if (ret < 0) { dev_err(panel->dev, "failed to transmit PPS: %d\n", ret); return ret; // <-- leaks regulators and GPIO } ret =3D mipi_dsi_compression_mode(ctx->dsi, true); if (ret < 0) { dev_err(dev, "failed to enable compression mode: %d\n", ret); return ret; // <-- same leak } ``` Compare with `panel-novatek-nt37801.c` (lines 128-170), which has nearly id= entical `prepare()` logic but correctly uses `goto err` for all failure pat= hs after regulator enable. The fix is to replace the bare `return ret` stat= ements with `goto err` and add: ```c err: gpiod_set_value_cansleep(ctx->reset_gpio, 1); regulator_bulk_disable(ARRAY_SIZE(boe_bf068mwm_td0_supplies), ctx->supplies); return ret; ``` Also note that the first error path already has this cleanup inline (after = `boe_bf068mwm_td0_on()` fails), so consolidating all three into a `goto err= ` pattern would also reduce duplication. **Build dependency: `MIPI_DSI_MODE_DSC_ALL_SLICES_IN_PKT`** ```c dsi->mode_flags =3D MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_NO_EOT_PACKET | MIPI_DSI_CLOCK_NON_CONTINUOUS | MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_DSC_ALL_SLICES_IN_PKT; ``` This flag does not exist in the current mainline tree. It comes from the pr= erequisite series mentioned in the cover letter. This will fail to compile = without that series applied first. The dependency should be clearly documen= ted (e.g., in the commit message or as a `Depends-on:` tag) to help maintai= ners apply the patches in the right order. **Minor: inconsistent `dev` vs `panel->dev` in error messages** In `prepare()`, most error messages use `dev` (which is `&ctx->dsi->dev`), = but the PPS failure uses `panel->dev`: ```c dev_err(panel->dev, "failed to transmit PPS: %d\n", ret); ``` while the surrounding errors use: ```c dev_err(dev, "Failed to enable regulators: %d\n", ret); dev_err(dev, "failed to enable compression mode: %d\n", ret); ``` These point to different `struct device` instances. Should be consistent = =E2=80=94 prefer `dev` throughout, matching the local variable. **Observation: First user of `MIPI_DSI_FMT_RGB101010` in a panel driver** ```c dsi->format =3D MIPI_DSI_FMT_RGB101010; ``` This format is defined in the MIPI DSI header but only handled by the Qualc= omm MSM DSI host driver (`dsi_host.c`). This is consistent with the Nothing= Phone (3a) being a Qualcomm platform, but means this driver effectively on= ly works on Qualcomm SoCs. Worth noting in the commit message or Kconfig he= lp text. **Observation: DSC `slice_height =3D 13`** ```c ctx->dsc.slice_height =3D 13; ``` 2392 / 13 =3D 184 slices, which divides evenly, so this is valid. It's an u= nusual choice (most drivers use values like 16, 20, 40, 60) but presumably = dictated by the panel hardware. **No other issues.** The driver otherwise follows established patterns well= : `devm_drm_panel_alloc`, `mipi_dsi_multi_context` for init sequences, `reg= ulator_bulk_get_const`, LPM toggle for HS backlight writes, `drm_connector_= helper_get_modes_fixed`, and proper probe/remove ordering. --- Generated by Claude Code Patch Reviewer