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@xxxxxxxxxx> > --- > > 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@xxxxxxxxxxx> 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 > >
