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

["Luca Ceresoli" <luca.ceresoli@bootlin.com>]

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.

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@bootlin.com/

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

(*) 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.

[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@bootlin.com/
    -> search "What about"

Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com