Re: [PATCH v3 6/8] drm/bridge: ps8640: Register and attach our DSI device at probe

[Andrzej Hajda <a.hajda@samsung.com>]

W dniu 23.08.2021 o 10:47, Maxime Ripard pisze:
> In order to avoid any probe ordering issue, the best practice is to move
> the secondary MIPI-DSI device registration and attachment to the
> MIPI-DSI host at probe time. Let's do this.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>   drivers/gpu/drm/bridge/parade-ps8640.c | 93 ++++++++++++++------------
>   1 file changed, 51 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 794c9516b05d..37f7d166a3c6 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -213,52 +213,10 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
>   				enum drm_bridge_attach_flags flags)
>   {
>   	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> -	struct device *dev = &ps_bridge->page[0]->dev;
> -	struct device_node *in_ep, *dsi_node;
> -	struct mipi_dsi_device *dsi;
> -	struct mipi_dsi_host *host;
> -	int ret;
> -	const struct mipi_dsi_device_info info = { .type = "ps8640",
> -						   .channel = 0,
> -						   .node = NULL,
> -						 };
>   
>   	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
>   		return -EINVAL;
>   
> -	/* port@0 is ps8640 dsi input port */
> -	in_ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
> -	if (!in_ep)
> -		return -ENODEV;
> -
> -	dsi_node = of_graph_get_remote_port_parent(in_ep);
> -	of_node_put(in_ep);
> -	if (!dsi_node)
> -		return -ENODEV;
> -
> -	host = of_find_mipi_dsi_host_by_node(dsi_node);
> -	of_node_put(dsi_node);
> -	if (!host)
> -		return -ENODEV;
> -
> -	dsi = devm_mipi_dsi_device_register_full(dev, host, &info);
> -	if (IS_ERR(dsi)) {
> -		dev_err(dev, "failed to create dsi device\n");
> -		ret = PTR_ERR(dsi);
> -		return ret;
> -	}
> -
> -	ps_bridge->dsi = dsi;
> -
> -	dsi->host = host;
> -	dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
> -			  MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
> -	dsi->format = MIPI_DSI_FMT_RGB888;
> -	dsi->lanes = DP_NUM_LANES;
> -	ret = devm_mipi_dsi_attach(dev, dsi);
> -	if (ret)
> -		return ret;
> -
>   	/* Attach the panel-bridge to the dsi bridge */
>   	return drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge,
>   				 &ps_bridge->bridge, flags);
> @@ -305,6 +263,53 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs = {
>   	.pre_enable = ps8640_pre_enable,
>   };
>   
> +static int ps8640_bridge_host_attach(struct device *dev, struct ps8640 *ps_bridge)
> +{
> +	struct device_node *in_ep, *dsi_node;
> +	struct mipi_dsi_device *dsi;
> +	struct mipi_dsi_host *host;
> +	int ret;
> +	const struct mipi_dsi_device_info info = { .type = "ps8640",
> +						   .channel = 0,
> +						   .node = NULL,
> +						 };
> +
> +	/* port@0 is ps8640 dsi input port */
> +	in_ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
> +	if (!in_ep)
> +		return -ENODEV;
> +
> +	dsi_node = of_graph_get_remote_port_parent(in_ep);
> +	of_node_put(in_ep);
> +	if (!dsi_node)
> +		return -ENODEV;
> +
> +	host = of_find_mipi_dsi_host_by_node(dsi_node);
> +	of_node_put(dsi_node);
> +	if (!host)
> +		return -EPROBE_DEFER;
> +
> +	dsi = devm_mipi_dsi_device_register_full(dev, host, &info);
> +	if (IS_ERR(dsi)) {
> +		dev_err(dev, "failed to create dsi device\n");
> +		return PTR_ERR(dsi);
> +	}
> +
> +	ps_bridge->dsi = dsi;
> +
> +	dsi->host = host;
> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
> +			  MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
> +	dsi->format = MIPI_DSI_FMT_RGB888;
> +	dsi->lanes = DP_NUM_LANES;
> +
> +	ret = devm_mipi_dsi_attach(dev, dsi);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
>   static int ps8640_probe(struct i2c_client *client)
>   {
>   	struct device *dev = &client->dev;
> @@ -371,6 +376,10 @@ static int ps8640_probe(struct i2c_client *client)
>   
>   	drm_bridge_add(&ps_bridge->bridge);
>   
> +	ret = ps8640_bridge_host_attach(dev, ps_bridge);
> +	if (ret)
> +		return ret;


I do not see drm_bridge_remove on error path, the same for sn65dsi83.


Regards

Andrzej


> +
>   	return 0;
>   }
>