Re: [PATCH 01/10] drm/bridge: add of_drm_get_bridge_by_endpoint()

[Dmitry Baryshkov]

On Mon, Apr 13, 2026 at 07:07:14PM +0200, Luca Ceresoli wrote:
> Hi Dmitry, Maxime,
> 
> thanks Dmitry for the quick feedback!
> 
> On Mon Apr 13, 2026 at 4:58 PM CEST, Dmitry Baryshkov wrote:
> 
> >> --- a/drivers/gpu/drm/drm_bridge.c
> >> +++ b/drivers/gpu/drm/drm_bridge.c
> >> @@ -1581,6 +1581,52 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
> >>  	return bridge;
> >>  }
> >>  EXPORT_SYMBOL(of_drm_find_bridge);
> >> +
> >> +/**
> >> + * of_drm_get_bridge_by_endpoint - return DRM bridge connected to a port/endpoint
> >> + * @np: device tree node containing output ports
> >> + * @port: port in the device tree node, or -1 for the first port found
> >> + * @endpoint: endpoint in the device tree node, or -1 for the first endpoint found
> >> + * @bridge: pointer to hold returned drm_bridge, must not be NULL
> >> + *
> >> + * Given a DT node's port and endpoint number, find the connected node and
> >> + * return the associated drm_bridge device.
> >> + *
> >> + * The refcount of the returned bridge is incremented. Use drm_bridge_put()
> >> + * when done with it.
> >> + *
> >> + * Returns zero (and sets *bridge to a valid bridge pointer) if successful,
> >> + * or one of the standard error codes (and the value in *bridge is
> >> + * unspecified) if it fails.
> >
> > Can we return drm_bridge or error cookie instead?
> 
> (while replying I realized there is a design flaw in my implementation, but
> see below)
> 
> I initially thought I'd do it, but I don't like returning an error cookie
> for functions getting a bridge pointer. The main reason is that with bridge
> refcounting the __free() cleanup actions are handy in a lot of places, so we
> are introdugin a lot of code like:
> 
>   struct drm_bridge *foo __free(drm_bridge_put) = some_func(...);
> 
> Where some_func can be one of of_drm_find_bridge(),
> drm_bridge_get_next_bridge(), drm_bridge_chain_get_{first,last}_bridge()
> etc.

This is fine even with the functions returning a cookie: the free
function can explicitly check and return early if IS_ERR() pointer is
passed to it.

> 
> Such code is very handy exactly because these functions return either a
> valid pointer or NULL, and thus the cleanup actions always does the right
> thing. If an error cookie were returned, the caller would have to be very
> careful in inhibiting the cleanup action by clearing the pointer before
> returning. This originate for example this discussion: [0]
> 
> [0] https://lore.kernel.org/lkml/4cd29943-a8d0-4706-b0c5-01d6b33863e4@xxxxxxxxxxx/
> 
> So I think never having a negative error value in the bridge pointer is
> useful to prevent bugs slipping in drivers. For this we should take one of
> these two opposite approaches:
> 
>  1. don't return a bridge pointer which can be an ERR_PTR; return an int
>     with the error code and take a **drm_bridge and:
>       - on success, set the valid pointer in *bridge
>       - on failure, set *bridge = NULL (*)
>  2. like the above-mentioned functions (of_drm_find_bridge(),
>     drm_bridge_get_next_bridge() etc) return a drm_bridge pointer which is
>     either a valid pointer or NULL

3. Return pointer or cookie, ignore cookie in the release function.

> 
> (*) I didn't do it in this patch, that's a design flaw, I'll fix in case
>     approach 1 is taken
> 
> Clearly option 2 is the simplest to use, but it loses information about
> which error happened.
> 
> What do you think about these options?
> 
> >> + */
> >> +int of_drm_get_bridge_by_endpoint(const struct device_node *np,
> >> +				  int port, int endpoint,
> >> +				  struct drm_bridge **bridge)
> >
> > Nit: can it be drm_of_get_bridge_by_endpoint?
> 
> Argh, this convention is changing periodically it seems! :-)
> 
> I previous discussions I was asked to do either drm_of_ [1] of of_drm_ [2],
> but since the latter was the last one requested I sticked on it.
> 
> @Maxime, Dmitry, I'm OK with either, just let me know if I need to change.

I missed Maxime's response, sorry. I'm fine with the suggested
convention of using the first argument.

> 
> [1] https://lore.kernel.org/dri-devel/20250319-stylish-lime-mongoose-0a18ad@houat/
>     -> search "called drm_of_find_bridge"
> [2] https://lore.kernel.org/all/DEH1VJUEJ8HQ.MIS45UOLCPXL@xxxxxxxxxxx/
>     -> search "What about"
> 
> Luca
> 
> --
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

-- 
With best wishes
Dmitry