From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: dri-devel@lists.freedesktop.org
Cc: Maxime Ripard <maxime.ripard@free-electrons.com>,
        linux-kernel@vger.kernel.org, Mark Brown <broonie@kernel.org>,
        Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH] drm/panel: lvds: Handle the optional regulator case properly
Date: Fri, 12 Jan 2018 00:06:06 +0200
Message-ID: <1579517.s7hJ1SlSuH@avalon>
Organization: Ideas on Board Oy
In-Reply-To: <20180111131256.bmr6qbgzcmyfg4mj@flea.lan>
References: <20180110155941.16109-1-maxime.ripard@free-electrons.com> <1944741.zEkzsdPTSS@avalon> <20180111131256.bmr6qbgzcmyfg4mj@flea.lan>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org

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