Re: [PATCH v13 11/17] drm/bridge: analogix_dp: Apply drm_bridge_connector helper

[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@xxxxxxxxxxxxxx>
> > Tested-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> > Tested-by: Heiko Stuebner <heiko@xxxxxxxxx> # 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@xxxxxxxxxxx>

Ah, your previous commit added this nice reference handling. Thanks for 
the kind review.

I'll fix it in v14.

Best regards,
Damon