lkml.org 
[lkml]   [2018]   [Jan]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH] drm/panel: lvds: Handle the optional regulator case properly
    Date
    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

    \
     
     \ /
      Last update: 2018-01-14 23:24    [W:3.863 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site