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

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

Hi Dmitry,

On Mon Apr 13, 2026 at 7:56 PM CEST, Dmitry Baryshkov wrote:
> 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@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
>
> 3. Return pointer or cookie, ignore cookie in the release function.

Ah, that's a good idea indeed!

It had been proposed recently by Laurent too, but in that case I didn't
take the suggestion because it was referring to a panel API which IIUC is
meant to be reworked anyway [0]. I must have archived the idea too much and
didn't think about using it now! :)

So yes, being of_drm_get_bridge_by_endpoint() a new API that is meant to
stay, I think it's worth doing.

Sadly, I guess that means I have to drop all the R-by you already gave to
various patches in the series.

[0] https://lore.kernel.org/all/DH624WYWKT14.5TSCJXZPVN0T@bootlin.com/

>> >> + */
>> >> +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.

OK, no problem. Based on current discussion, in v2 the new API will be:

/**
 * 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
 *
 * 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 a pointer to the connected drm_bridge, or a negative error on failure
 */
struct drm_bridge *int of_drm_get_bridge_by_endpoint(const struct device_node *np,
                                                     int port, int endpoint);

It would be nice to agree on the API before v2, to avoid the need to rework
many patches and drop any review tags multiple times.

Luca

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