| Message ID | 20181106152802.38599-1-tony@atomide.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/omap: dsi: Fix missing of_platform_depopulate() | expand |
Hi Tony, Thank you for the patch. On Tuesday, 6 November 2018 17:28:02 EET Tony Lindgren wrote: > We're missing a call to of_platform_depopulate() on errors for dsi. > Looks like dss is already doing this. > > Signed-off-by: Tony Lindgren <tony@atomide.com> This makes sense to me. You may however want to note in the commit message that this patch also turns the of_platform_populate() failures into fatal errors. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/omapdrm/dss/dsi.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c > b/drivers/gpu/drm/omapdrm/dss/dsi.c --- a/drivers/gpu/drm/omapdrm/dss/dsi.c > +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c > @@ -5429,15 +5429,19 @@ static int dsi_probe(struct platform_device *pdev) > } > > r = of_platform_populate(dev->of_node, NULL, NULL, dev); > - if (r) > + if (r) { > DSSERR("Failed to populate DSI child devices: %d\n", r); > + goto err_uninit_output; > + } > > r = component_add(&pdev->dev, &dsi_component_ops); > if (r) > - goto err_uninit_output; > + goto err_of_depopulate; > > return 0; > > +err_of_depopulate: > + of_platform_depopulate(dev); > err_uninit_output: > dsi_uninit_output(dsi); > err_pm_disable:
* Laurent Pinchart <laurent.pinchart@ideasonboard.com> [181107 05:30]: > Hi Tony, > > Thank you for the patch. > > On Tuesday, 6 November 2018 17:28:02 EET Tony Lindgren wrote: > > We're missing a call to of_platform_depopulate() on errors for dsi. > > Looks like dss is already doing this. > > > > Signed-off-by: Tony Lindgren <tony@atomide.com> > > This makes sense to me. You may however want to note in the commit message > that this patch also turns the of_platform_populate() failures into fatal > errors. Well of_platform_populate() failure should be fatal here,right? :) Tomi, let me know if you need a resend with updated comments or if you want to update the message yourself to your liking. Regards, Tony
Hi Tony, On Thursday, 8 November 2018 19:01:45 EET Tony Lindgren wrote: > * Laurent Pinchart <laurent.pinchart@ideasonboard.com> [181107 05:30]: > > On Tuesday, 6 November 2018 17:28:02 EET Tony Lindgren wrote: > > > We're missing a call to of_platform_depopulate() on errors for dsi. > > > Looks like dss is already doing this. > > > > > > Signed-off-by: Tony Lindgren <tony@atomide.com> > > > > This makes sense to me. You may however want to note in the commit message > > that this patch also turns the of_platform_populate() failures into fatal > > errors. > > Well of_platform_populate() failure should be fatal here,right? :) I'm not complaining about the functional change, just the lack of visibility :-) > Tomi, let me know if you need a resend with updated comments or > if you want to update the message yourself to your liking.
Hi, On Tue, Nov 06, 2018 at 07:28:02AM -0800, Tony Lindgren wrote: > We're missing a call to of_platform_depopulate() on errors for dsi. > Looks like dss is already doing this. > > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com> -- Sebastian > drivers/gpu/drm/omapdrm/dss/dsi.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c > --- a/drivers/gpu/drm/omapdrm/dss/dsi.c > +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c > @@ -5429,15 +5429,19 @@ static int dsi_probe(struct platform_device *pdev) > } > > r = of_platform_populate(dev->of_node, NULL, NULL, dev); > - if (r) > + if (r) { > DSSERR("Failed to populate DSI child devices: %d\n", r); > + goto err_uninit_output; > + } > > r = component_add(&pdev->dev, &dsi_component_ops); > if (r) > - goto err_uninit_output; > + goto err_of_depopulate; > > return 0; > > +err_of_depopulate: > + of_platform_depopulate(dev); > err_uninit_output: > dsi_uninit_output(dsi); > err_pm_disable: > -- > 2.19.1
On 06/11/18 17:28, Tony Lindgren wrote: > We're missing a call to of_platform_depopulate() on errors for dsi. > Looks like dss is already doing this. > > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > drivers/gpu/drm/omapdrm/dss/dsi.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c > --- a/drivers/gpu/drm/omapdrm/dss/dsi.c > +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c > @@ -5429,15 +5429,19 @@ static int dsi_probe(struct platform_device *pdev) > } > > r = of_platform_populate(dev->of_node, NULL, NULL, dev); > - if (r) > + if (r) { > DSSERR("Failed to populate DSI child devices: %d\n", r); > + goto err_uninit_output; > + } > > r = component_add(&pdev->dev, &dsi_component_ops); > if (r) > - goto err_uninit_output; > + goto err_of_depopulate; > > return 0; > > +err_of_depopulate: > + of_platform_depopulate(dev); > err_uninit_output: > dsi_uninit_output(dsi); > err_pm_disable: > Thanks, looks good to me. I'll push to drm-misc-fixes. Tomi
diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c --- a/drivers/gpu/drm/omapdrm/dss/dsi.c +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c @@ -5429,15 +5429,19 @@ static int dsi_probe(struct platform_device *pdev) } r = of_platform_populate(dev->of_node, NULL, NULL, dev); - if (r) + if (r) { DSSERR("Failed to populate DSI child devices: %d\n", r); + goto err_uninit_output; + } r = component_add(&pdev->dev, &dsi_component_ops); if (r) - goto err_uninit_output; + goto err_of_depopulate; return 0; +err_of_depopulate: + of_platform_depopulate(dev); err_uninit_output: dsi_uninit_output(dsi); err_pm_disable:
We're missing a call to of_platform_depopulate() on errors for dsi. Looks like dss is already doing this. Signed-off-by: Tony Lindgren <tony@atomide.com> --- drivers/gpu/drm/omapdrm/dss/dsi.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)