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.rip...@free-electrons.com>
> >> ---
> >>
> >> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
