Hi Maxime,
On Friday, 12 January 2018 00:06:06 EET Laurent Pinchart wrote:
> 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.
With the above change,
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
Tested-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> >> 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
