[PATCH] drm/msm/dp: call dp_display_get_next_bridge() during probe

[Bjorn Andersson]

On Tue, Nov 07, 2023 at 02:43:33AM +0200, Dmitry Baryshkov wrote:
> The funcion dp_display_get_next_bridge() can return -EPROBE_DEFER if the
> next bridge is not (yet) available. However returning -EPROBE_DEFER from
> msm_dp_modeset_init() is not ideal. This leads to -EPROBE return from
> component_bind, which can easily result in -EPROBE_DEFR loops.
> 

Nice!

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> ---
> 
> Dependencies: https://patchwork.freedesktop.org/series/120375/
> 
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 36 +++++++++++++++++------------
>  1 file changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index d542db37763a..ddb3c84f39a2 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1197,15 +1197,27 @@ static const struct msm_dp_desc *dp_display_get_desc(struct platform_device *pde
>  	return NULL;
>  }
>  
> -static int dp_auxbus_done_probe(struct drm_dp_aux *aux)
> +static int dp_display_get_next_bridge(struct msm_dp *dp);
> +
> +static int dp_display_probe_tail(struct device *dev)
>  {
> -	int rc;
> +	struct msm_dp *dp = dev_get_drvdata(dev);
> +	int ret;
>  
> -	rc = component_add(aux->dev, &dp_display_comp_ops);
> -	if (rc)
> -		DRM_ERROR("eDP component add failed, rc=%d\n", rc);
> +	ret = dp_display_get_next_bridge(dp);
> +	if (ret)
> +		return ret;
>  
> -	return rc;
> +	ret = component_add(dev, &dp_display_comp_ops);
> +	if (ret)
> +		DRM_ERROR("component add failed, rc=%d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int dp_auxbus_done_probe(struct drm_dp_aux *aux)
> +{
> +	return dp_display_probe_tail(aux->dev);
>  }
>  
>  static int dp_display_probe(struct platform_device *pdev)
> @@ -1280,11 +1292,9 @@ static int dp_display_probe(struct platform_device *pdev)
>  			goto err;
>  		}
>  	} else {
> -		rc = component_add(&pdev->dev, &dp_display_comp_ops);
> -		if (rc) {
> -			DRM_ERROR("component add failed, rc=%d\n", rc);
> +		rc = dp_display_probe_tail(&pdev->dev);
> +		if (rc)
>  			goto err;
> -		}
>  	}
>  
>  	return rc;
> @@ -1415,7 +1425,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
>  	 * For DisplayPort interfaces external bridges are optional, so
>  	 * silently ignore an error if one is not present (-ENODEV).
>  	 */
> -	rc = devm_dp_parser_find_next_bridge(dp->drm_dev->dev, dp_priv->parser);
> +	rc = devm_dp_parser_find_next_bridge(&dp->pdev->dev, dp_priv->parser);

This transition worried me, but after reading the code the current model
of mixing devices for devres scares me more. So, nice cleanup! But I
think we have a few more of these...


That said, &dp->pdev->dev is dp_priv->parser->dev, the function no
longer relate to the "parser module", and we stash the return value of

  devm_drm_of_get_bridge(dev, dev->of_node, 1, 0)

in parser->next_brigde, so that we 5 lines below this call can move it
into dp->next_bridge.

As such, I'd like to propose that we change
devm_dp_parser_find_next_bridge() to just take &dp->pdev->dev and return
the next_bridge, in an ERR_PTR().

But that's follow-up-patch material.


Reviewed-by: Bjorn Andersson <quic_bjorande at quicinc.com>

Regards,
Bjorn

>  	if (!dp->is_edp && rc == -ENODEV)
>  		return 0;
>  
> @@ -1435,10 +1445,6 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
>  
>  	dp_priv = container_of(dp_display, struct dp_display_private, dp_display);
>  
> -	ret = dp_display_get_next_bridge(dp_display);
> -	if (ret)
> -		return ret;
> -
>  	ret = dp_bridge_init(dp_display, dev, encoder);
>  	if (ret) {
>  		DRM_DEV_ERROR(dev->dev,
> -- 
> 2.42.0
> 
>