Re: [PATCHv2 1/6] drm: drm_bridge: add connector_attach/detach bridge ops

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

Hi Hans,

On 02/03/2021 18:23, Hans Verkuil wrote:
> Add bridge connector_attach/detach ops. These ops are called when a
> bridge is attached or detached to a drm_connector. These ops can be
> used to register and unregister an HDMI CEC adapter for a bridge that
> supports CEC.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>   drivers/gpu/drm/drm_bridge_connector.c |  9 +++++++++
>   include/drm/drm_bridge.h               | 27 ++++++++++++++++++++++++++
>   2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
> index 791379816837..07db71d4f5b3 100644
> --- a/drivers/gpu/drm/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/drm_bridge_connector.c
> @@ -203,6 +203,11 @@ static void drm_bridge_connector_destroy(struct drm_connector *connector)
>   {
>   	struct drm_bridge_connector *bridge_connector =
>   		to_drm_bridge_connector(connector);
> +	struct drm_bridge *bridge;
> +
> +	drm_for_each_bridge_in_chain(bridge_connector->encoder, bridge)
> +		if (bridge->funcs->connector_detach)
> +			bridge->funcs->connector_detach(bridge, connector);
>   
>   	if (bridge_connector->bridge_hpd) {
>   		struct drm_bridge *hpd = bridge_connector->bridge_hpd;
> @@ -375,6 +380,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>   		connector->polled = DRM_CONNECTOR_POLL_CONNECT
>   				  | DRM_CONNECTOR_POLL_DISCONNECT;
>   
> +	drm_for_each_bridge_in_chain(encoder, bridge)
> +		if (bridge->funcs->connector_attach)
> +			bridge->funcs->connector_attach(bridge, connector);
> +
>   	return connector;
>   }
>   EXPORT_SYMBOL_GPL(drm_bridge_connector_init);
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 2195daa289d2..3320a6ebd253 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -629,6 +629,33 @@ struct drm_bridge_funcs {
>   	 * the DRM_BRIDGE_OP_HPD flag in their &drm_bridge->ops.
>   	 */
>   	void (*hpd_disable)(struct drm_bridge *bridge);
> +
> +	/**
> +	 * @connector_attach:
> +	 *
> +	 * This callback is invoked whenever our bridge is being attached to a
> +	 * &drm_connector. This is where an HDMI CEC adapter can be registered.
> +	 * Note that this callback expects that this op always succeeds. Since
> +	 * HDMI CEC support is an optional feature, any failure to register a
> +	 * CEC adapter must be ignored since video output will still work
> +	 * without CEC.
> +	 *

Even if CEC support is optional, the callback itself is generic. 
Wouldn't it be better to make this function return an error, and for 
CEC, just return 0 if CEC won't get registered correctly?

Also, I personally like things to fail if something doesn't go right, 
instead of continuing, if that thing is never supposed to happen in 
normal situations. E.g. if CEC registration fails because we're out of 
memory, I think the op should fail too.

  Tomi