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 10:53:55 +1000 Message-ID: In-Reply-To: <20260514-rcar-du-dsc-v1-3-d65f7a9e9841@ideasonboard.com> References: <20260514-rcar-du-dsc-v1-0-d65f7a9e9841@ideasonboard.com> <20260514-rcar-du-dsc-v1-3-d65f7a9e9841@ideasonboard.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review **Status: Has a bug and minor issues.** **Bug - Bridge reference leak:** ```c dsc->bridge.next_bridge = of_drm_find_and_get_bridge(remote); of_node_put(remote); if (!dsc->bridge.next_bridge) return -EPROBE_DEFER; ``` `of_drm_find_and_get_bridge()` takes a reference on the returned bridge (via `drm_bridge_get()`), but there is no corresponding `drm_bridge_put()` anywhere in the driver. There is no `.remove` callback and no devm cleanup for this reference. Compare with `rcar_du_encoder.c` which properly calls `drm_bridge_put()` on its stored bridge references during cleanup. The existing `rcar_mipi_dsi.c` uses `devm_drm_of_get_bridge()` for its `next_bridge`, which handles cleanup automatically. This driver should do the same: ```c dsc->bridge.next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0); if (IS_ERR(dsc->bridge.next_bridge)) return dev_err_probe(dev, PTR_ERR(dsc->bridge.next_bridge), "Failed to get next bridge\n"); ``` Note: `devm_drm_of_get_bridge()` returns `ERR_PTR` (not NULL), so the error check would need to change accordingly. **Minor - Error swallowing in atomic_enable:** ```c WARN_ON(clk_prepare_enable(dsc->clk)); WARN_ON(reset_control_deassert(dsc->rst)); ``` If `clk_prepare_enable()` fails, `reset_control_deassert()` still runs on a potentially non-clocked block. These should be sequenced with early return on failure. Though this pattern is common in bridge `atomic_enable` (where you can't return errors), at minimum the reset deassert should be conditional on clock enable succeeding. **Minor - mode_valid comment:** ```c if (mode->clock > 400000) /* Really 400 Mpixel/s */ return MODE_CLOCK_HIGH; ``` The comment "Really 400 Mpixel/s" is slightly confusing. `mode->clock` is in kHz, so 400000 kHz = 400 MHz. For 1 pixel/clock this is indeed 400 Mpixel/s. The comment could be clearer: perhaps "400 MHz pixel clock = 400 Mpixel/s". **Minor - mmio mapped but unused:** ```c dsc->mmio = devm_platform_ioremap_resource(pdev, 0); ``` The `mmio` field is mapped but never read from or written to in this driver (bypass mode doesn't touch any registers). This is fine as scaffolding for future DSC encoding support, but worth noting. --- --- Generated by Claude Code Patch Reviewer