Hi Maxime, On Thursday, 11 January 2018 15:12:56 EET Maxime Ripard wrote: > On Thu, Jan 11, 2018 at 03:05:01PM +0200, Laurent Pinchart wrote: > > On Wednesday, 10 January 2018 17:59:41 EET Maxime Ripard wrote: > >> The devm_regulator_get_optional function, unlike it was assumed in the > >> commit a1c55bccf600 ("drm/panel: lvds: Add support for the power-supply > >> property"), is actually returning an error pointer with -ENODEV instead > >> of NULL when there's no regulator to find. > >> > >> Make sure we handle that case properly. > >> > >> Fixes: a1c55bccf600 ("drm/panel: lvds: Add support for the power-supply > >> property") Signed-off-by: Maxime Ripard > >> <maxime.ripard@xxxxxxxxxxxxxxxxxx> > >> --- > >> > >> drivers/gpu/drm/panel/panel-lvds.c | 9 +++++++-- > >> 1 file changed, 7 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/panel/panel-lvds.c > >> b/drivers/gpu/drm/panel/panel-lvds.c index 57e38a9e7ab4..9f46e7095c0e > >> 100644 > >> --- a/drivers/gpu/drm/panel/panel-lvds.c > >> +++ b/drivers/gpu/drm/panel/panel-lvds.c > >> @@ -215,8 +215,13 @@ static int panel_lvds_probe(struct platform_device > >> *pdev) > >> lvds->supply = devm_regulator_get_optional(lvds->dev, "power"); > >> if (IS_ERR(lvds->supply)) { > >> ret = PTR_ERR(lvds->supply); > >> - dev_err(lvds->dev, "failed to request regulator: %d\n", ret); > >> - return ret; > >> + > >> + if (ret != -ENODEV) { > >> + dev_err(lvds->dev, "failed to request regulator: %d\n", ret); > >> + return ret; > > > > I wouldn't print an error message if ret == -EPROBE_DEFER. > > > >> + } else { > >> + lvds->supply = NULL; > >> + } > >> } > > > > How about > > > > lvds->supply = devm_regulator_get_optional(lvds->dev, "power"); > > if (IS_ERR(lvds->supply)) { > > ret = PTR_ERR(lvds->supply); > > if (ret != -ENODEV) { > > if (ret == -EPROBE_DEFER) > > I guess that would be != -EPROBE_DEFER Of course, my bad. > > dev_err(lvds->dev, "failed to request regulator: %d\n", ret); > > return ret; > > } > > > > lvds->supply = NULL; > > } > > Otherwise, it works for me. > > > My preference, however, would be for devm_regulator_get_optional() to > > return NULL when no regulator is present. The current implementation > > returns -ENODEV in multiple cases, making it impossible to properly > > discriminate between having no regulator and not being able to get the > > regulator due to an error. > > It would feel more intuitive to me too, but it would also require to > fix most of the call sites that would have a similar pattern. Of course. I don't mean we need to delay this patch, but I still think it would be a good API improvement that could be developed separately (and of course I wouldn't complain if you volunteered ;-)). -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel