Re: [PATCH v2 3/3] drm/mxsfb: add DRM_BRIDGE_ATTACH_NO_CONNECTOR flag to drm_bridge_attach

[Liu Ying <victor.liu@nxp.com>]

On 12/24/2024, Marek Vasut wrote:
> Commit a25b988ff83f ("drm/bridge: Extend bridge API to disable connector creation")
> added DRM_BRIDGE_ATTACH_NO_CONNECTOR bridge flag and all bridges handle
> this flag in some way since then.
> Newly added bridge drivers must no longer contain the connector creation and
> will fail probing if this flag isn't set.
> 
> In order to be able to connect to those newly added bridges as well,
> make use of drm_bridge_connector API and have the connector initialized
> by the display controller.
> 
> Based on 2e87bf389e13 ("drm/rockchip: add DRM_BRIDGE_ATTACH_NO_CONNECTOR flag to drm_bridge_attach")
> 
> This makes LT9611 work with i.MX8M Mini.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Liu Ying <victor.liu@nxp.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Robert Foss <rfoss@kernel.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: imx@lists.linux.dev
> Cc: linux-arm-kernel@lists.infradead.org
> ---
> V2: Cache connector from drm_bridge_connector_init()
> ---
>  drivers/gpu/drm/mxsfb/Kconfig     |  1 +
>  drivers/gpu/drm/mxsfb/mxsfb_drv.c | 37 ++++++++++++++++++++++---------
>  2 files changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/Kconfig b/drivers/gpu/drm/mxsfb/Kconfig
> index 07fb6901996ae..e67de148955b2 100644
> --- a/drivers/gpu/drm/mxsfb/Kconfig
> +++ b/drivers/gpu/drm/mxsfb/Kconfig
> @@ -12,6 +12,7 @@ config DRM_MXSFB
>  	select DRM_CLIENT_SELECTION
>  	select DRM_MXS
>  	select DRM_KMS_HELPER
> +	select DRM_BRIDGE_CONNECTOR

Select DRM_DISPLAY_HELPER.

>  	select DRM_GEM_DMA_HELPER
>  	select DRM_PANEL
>  	select DRM_PANEL_BRIDGE
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> index 59020862cf65e..2f205512f3105 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> @@ -20,6 +20,7 @@
>  #include <drm/clients/drm_client_setup.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_bridge_connector.h>
>  #include <drm/drm_connector.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_fbdev_dma.h>
> @@ -119,9 +120,9 @@ static const struct drm_mode_config_helper_funcs mxsfb_mode_config_helpers = {
>  static int mxsfb_attach_bridge(struct mxsfb_drm_private *mxsfb)
>  {
>  	struct drm_device *drm = mxsfb->drm;
> -	struct drm_connector_list_iter iter;
> -	struct drm_panel *panel;
> +	struct drm_connector *connector;
>  	struct drm_bridge *bridge;
> +	struct drm_panel *panel;
>  	int ret;
>  
>  	ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, &panel,
> @@ -139,21 +140,35 @@ static int mxsfb_attach_bridge(struct mxsfb_drm_private *mxsfb)
>  	if (!bridge)
>  		return -ENODEV;
>  
> -	ret = drm_bridge_attach(&mxsfb->encoder, bridge, NULL, 0);
> +	ret = drm_bridge_attach(&mxsfb->encoder, bridge, NULL,
> +				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>  	if (ret)
>  		return dev_err_probe(drm->dev, ret, "Failed to attach bridge\n");
>  
> -	mxsfb->bridge = bridge;
> +	connector = drm_bridge_connector_init(drm, &mxsfb->encoder);

Nit: Drop connector and use mxsfb->connector?

> +	if (IS_ERR(connector)) {
> +		ret = PTR_ERR(connector);
> +		dev_err_probe(drm->dev, ret,
> +			      "Failed to initialize bridge connector: %pe\n",
> +			      connector);
> +		return ret;

return dev_err_probe(drm->dev, PTR_ERR(connector),
"Failed to initialize bridge connector: %pe\n", mxsfb->connector);

> +	}
>  
> -	/*
> -	 * Get hold of the connector. This is a bit of a hack, until the bridge
> -	 * API gives us bus flags and formats.
> -	 */
> -	drm_connector_list_iter_begin(drm, &iter);
> -	mxsfb->connector = drm_connector_list_iter_next(&iter);
> -	drm_connector_list_iter_end(&iter);
> +	ret = drm_connector_attach_encoder(connector, &mxsfb->encoder);
> +	if (ret < 0) {
> +		dev_err_probe(drm->dev, ret,
> +			      "Failed to attach encoder.\n");

It looks like no one else calls dev_err_probe() when
drm_connector_attach_encoder() fails, plus drm_connector_attach_encoder()
doesn't return -EPROBE_DEFER at all.

> +		goto err_cleanup_connector;

Nit: Call drm_connector_cleanup() directly instead of using goto.

> +	}
> +
> +	mxsfb->bridge = bridge;
> +	mxsfb->connector = connector;
>  
>  	return 0;
> +
> +err_cleanup_connector:
> +	drm_connector_cleanup(connector);
> +	return ret;
>  }
>  
>  static irqreturn_t mxsfb_irq_handler(int irq, void *data)

-- 
Regards,
Liu Ying