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/rcar-du: dsc: Add rudimentary Renesas R-Car V4H DSC driver Date: Sat, 16 May 2026 09:29:58 +1000 Message-ID: In-Reply-To: <20260515-rcar-du-dsc-v3-3-164157820498@ideasonboard.com> References: <20260515-rcar-du-dsc-v3-0-164157820498@ideasonboard.com> <20260515-rcar-du-dsc-v3-3-164157820498@ideasonboard.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 core driver and has several items worth addressing: **Bridge reference leak**: `of_drm_find_and_get_bridge()` increments the br= idge refcount (as documented: "The refcount of the returned bridge is incre= mented. Use drm_bridge_put() when done with it."). The driver never calls `= drm_bridge_put()` on `dsc->bridge.next_bridge`, leaking the reference. Comp= are with `rcar_lvds.c` which properly calls `drm_bridge_put(lvds->companion= )` in its destroy callback for bridges obtained via `of_drm_find_and_get_br= idge()`. This should be fixed. ```c + dsc->bridge.next_bridge =3D of_drm_find_and_get_bridge(remote); + of_node_put(remote); + if (!dsc->bridge.next_bridge) + return -EPROBE_DEFER; ``` **Use `devm_pm_runtime_enable`**: The probe/remove pair of `pm_runtime_enab= le` / `pm_runtime_disable` can be replaced with `devm_pm_runtime_enable(&pd= ev->dev)` in probe, eliminating the need for `rcar_dsc_remove()` entirely. = This is the modern pattern and avoids ordering bugs. ```c +static void rcar_dsc_remove(struct platform_device *pdev) +{ + pm_runtime_disable(&pdev->dev); +} ``` **`-EPROBE_DEFER` without `dev_err_probe`**: When `of_drm_find_and_get_brid= ge()` returns NULL, the driver returns `-EPROBE_DEFER` silently. Using `ret= urn dev_err_probe(dev, -EPROBE_DEFER, "failed to find next bridge\n")` woul= d suppress the message during deferred probing but log it if probe ultimate= ly fails, which aids debugging. **Unused MMIO mapping**: `dsc->mmio` is mapped but never accessed since thi= s is bypass-mode only. This is fine for a rudimentary/initial driver, but i= t would be helpful to add a brief note in the commit message that the MMIO = is mapped for future DSC encoding support. (No code change needed, just a d= ocumentation note.) **`mode_valid` clock check**: The comment says "Really 400 Mpixel/s" but `m= ode->clock` is in kHz, and the check `mode->clock > 400000` limits to 400 M= Hz pixel clock. Whether 400 MHz clock equals 400 Mpixel/s depends on the pi= xel format =E2=80=94 they're the same only for single-pixel-per-clock. If t= he hardware limit is truly 400 Mpixel/s and the DSC block always processes = one pixel per clock, the check is correct. Worth confirming this assumption. ```c + if (mode->clock > 400000) /* Really 400 Mpixel/s */ + return MODE_CLOCK_HIGH; ``` --- Generated by Claude Code Patch Reviewer