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/bridge: analogix_dp: Apply drm_bridge_connector helper Date: Sun, 12 Apr 2026 11:37:41 +1000 Message-ID: In-Reply-To: <20260409065301.446670-12-damon.ding@rock-chips.com> References: <20260409065301.446670-1-damon.ding@rock-chips.com> <20260409065301.446670-12-damon.ding@rock-chips.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review This is the core patch of the series. Key changes: - **Removes the private `dp->connector`** from `analogix_dp_device`. - **Converts connector funcs to bridge funcs**: `analogix_dp_get_modes()` -> `analogix_dp_bridge_get_modes()`, `analogix_dp_detect()` -> `analogix_dp_bridge_detect()`, `analogix_dp_atomic_check()` -> `analogix_dp_bridge_atomic_check()`. - **Inverts the `DRM_BRIDGE_ATTACH_NO_CONNECTOR` logic** - now it's mandatory (required), not forbidden. - **Adds `devm_drm_bridge_add()`** in `analogix_dp_bind()` instead of probe time. - **Creates bridge_connector** on both Rockchip and Exynos sides. - **Reworks CRC functions** (`analogix_dp_start_crc`/`analogix_dp_stop_crc`) to get the dp device from the bridge chain rather than container_of on the connector. The CRC function rework deserves attention: ```c bridge = drm_bridge_chain_get_first_bridge(connector->encoder); if (bridge->type != DRM_MODE_CONNECTOR_eDP) return -EINVAL; dp = to_dp(bridge); ``` This correctly uses the bridge chain to find the analogix dp device, since the connector is now a bridge_connector and not directly embedded in `analogix_dp_device`. The `bridge->type` check is a reasonable guard. The `analogix_dp_bridge_detect()` correctly returns `connector_status_connected` for both panel and next_bridge cases. The bridge ops are set based on whether there's a panel or not: ```c if (dp->plat_data->panel) bridge->ops = DRM_BRIDGE_OP_MODES | DRM_BRIDGE_OP_DETECT; else bridge->ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT; ``` This is correct: panels provide modes (via `get_modes`), while monitors provide EDID. On the Exynos side, the error path for `analogix_dp_bind()` failure changes - it now returns immediately rather than falling through to `return ret`. This is a correction since the old code would always call `dp->encoder.funcs->destroy()` and then return `ret`, but if `ret == 0` it would still destroy the encoder. The new code only destroys the encoder on failure and returns immediately on success to proceed with bridge_connector init. No blocking issues. --- Generated by Claude Code Patch Reviewer