Messages in this thread |  | | Date | Fri, 10 Apr 2026 16:52:18 +0800 | | Subject | Re: [PATCH v13 11/17] drm/bridge: analogix_dp: Apply drm_bridge_connector helper | | From | Damon Ding <> |
| |
Hi Luca,
On 4/10/2026 3:41 PM, Luca Ceresoli wrote: > Hello Damon, > > On Thu Apr 9, 2026 at 8:52 AM CEST, Damon Ding wrote: >> Initialize bridge_connector for both Rockchip and Exynos encoder sides. >> Then, make DRM_BRIDGE_ATTACH_NO_CONNECTOR mandatory for Analogix bridge >> side, as the private &drm_connector is no longer created. >> >> The previous &drm_connector_funcs and &drm_connector_helper_funcs APIs >> are replaced by the corresponding &drm_bridge_funcs APIs: >> >> analogix_dp_atomic_check() -> analogix_dp_bridge_atomic_check() >> analogix_dp_detect() -> analogix_dp_bridge_detect() >> analogix_dp_get_modes() -> analogix_dp_bridge_get_modes() >> analogix_dp_bridge_edid_read() >> >> Additionally, the compatibilities of Analogix DP bridge based on whether >> the next bridge is a 'panel'. If it is, OP_MODES and OP_DETECT are >> supported; If not (the next bridge is a 'monitor' or a bridge chip), >> OP_EDID and OP_DETECT are supported. >> >> The devm_drm_bridge_add() is placed in analogix_dp_bind() instead of >> analogix_dp_probe(), because the type of next bridge (the panel, monitor >> or bridge chip) can only be determined after the probe process has fully >> completed. >> >> Signed-off-by: Damon Ding <damon.ding@rock-chips.com> >> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> >> Tested-by: Heiko Stuebner <heiko@sntech.de> # rk3588 > > I noticed two issues, sorry I haven't catched them before... > >> @@ -1580,7 +1533,8 @@ EXPORT_SYMBOL_GPL(analogix_dp_unbind); >> >> int analogix_dp_start_crc(struct drm_connector *connector) >> { >> - struct analogix_dp_device *dp = to_dp(connector); >> + struct analogix_dp_device *dp; >> + struct drm_bridge *bridge; >> >> if (!connector->state->crtc) { >> DRM_ERROR("Connector %s doesn't currently have a CRTC.\n", >> @@ -1588,13 +1542,26 @@ int analogix_dp_start_crc(struct drm_connector *connector) >> return -EINVAL; >> } >> >> + bridge = drm_bridge_chain_get_first_bridge(connector->encoder); >> + if (bridge->type != DRM_MODE_CONNECTOR_eDP) >> + return -EINVAL; >> + >> + dp = to_dp(bridge); >> + >> return drm_dp_start_crc(&dp->aux, connector->state->crtc); >> } > > drm_bridge_chain_get_first_bridge() takes a reference, you must call > drm_bridge_put(bridge) before returning. Given there are two return paths I > suggest: > > - struct drm_bridge *bridge; > > - bridge = drm_bridge_chain_get_first_bridge(connector->encoder); > + struct drm_bridge *bridge __free(drm_bridge_put) = > + drm_bridge_chain_get_first_bridge(connector->encoder); > >> EXPORT_SYMBOL_GPL(analogix_dp_start_crc); >> >> int analogix_dp_stop_crc(struct drm_connector *connector) >> { >> - struct analogix_dp_device *dp = to_dp(connector); >> + struct analogix_dp_device *dp; >> + struct drm_bridge *bridge; >> + >> + bridge = drm_bridge_chain_get_first_bridge(connector->encoder); >> + if (bridge->type != DRM_MODE_CONNECTOR_eDP) >> + return -EINVAL; >> + >> + dp = to_dp(bridge); >> >> return drm_dp_stop_crc(&dp->aux); >> } > > Same here: > > - struct drm_bridge *bridge; > > - bridge = drm_bridge_chain_get_first_bridge(connector->encoder); > + struct drm_bridge *bridge __free(drm-bridge_put) = > + drm_bridge_chain_get_first_bridge(connector->encoder); > > With the above two changes you can add to v14: > > +Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > >
Ah, your previous commit added this nice reference handling. Thanks for the kind review.
I'll fix it in v14.
Best regards, Damon
|  |