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/rockchip: dw_hdmi: Hold a reference to the dw-hdmi bridge Date: Sat, 16 May 2026 16:14:39 +1000 Message-ID: In-Reply-To: <20260510183114.1248840-6-jonas@kwiboo.se> References: <20260510183114.1248840-1-jonas@kwiboo.se> <20260510183114.1248840-6-jonas@kwiboo.se> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review **Status: Minor concern** This adds `of_drm_find_and_get_bridge(np)` after `dw_hdmi_bind()` succeeds and calls `drm_bridge_put()` in unbind. The purpose is to hold a reference on the bridge for the lifetime of the encoder component. **Concern**: The error path calls `dw_hdmi_remove(hdmi->hdmi)` on bridge lookup failure, but the bind function called `dw_hdmi_bind()` above. I'd expect `dw_hdmi_unbind()` to be used here for symmetry, matching what the `unbind` function does. Let me check: ```c hdmi->bridge = of_drm_find_and_get_bridge(np); if (!hdmi->bridge) { dw_hdmi_remove(hdmi->hdmi); return dev_err_probe(dev, -ENODEV, "failed to find bridge\n"); } ``` The use of `dw_hdmi_remove()` vs `dw_hdmi_unbind()` may be intentional if the APIs differ, but it's worth the author confirming this is the right teardown function. The `unbind` path uses `dw_hdmi_unbind()`. Also, `of_drm_find_and_get_bridge(np)` searches for a bridge associated with the device node. After `dw_hdmi_bind()` registers the bridge, this should find it. But the check `if (!hdmi->bridge)` only checks for NULL -- `of_drm_find_and_get_bridge` could also return an `ERR_PTR`. Checking the API: looking at the header, it returns `struct drm_bridge *` which can be NULL or ERR_PTR, so `IS_ERR_OR_NULL()` would be safer. --- --- Generated by Claude Code Patch Reviewer