From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart 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> References: <20180110155941.16109-1-maxime.ripard@free-electrons.com> <1944741.zEkzsdPTSS@avalon> <20180111131256.bmr6qbgzcmyfg4mj@flea.lan> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from galahad.ideasonboard.com (galahad.ideasonboard.com [IPv6:2001:4b98:dc2:45:216:3eff:febb:480d]) by gabe.freedesktop.org (Postfix) with ESMTPS id 8DF4A6E478 for ; Thu, 11 Jan 2018 22:06:08 +0000 (UTC) In-Reply-To: <20180111131256.bmr6qbgzcmyfg4mj@flea.lan> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: dri-devel@lists.freedesktop.org Cc: Daniel Vetter , Maxime Ripard , Mark Brown , linux-kernel@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org SGkgTWF4aW1lLAoKT24gVGh1cnNkYXksIDExIEphbnVhcnkgMjAxOCAxNToxMjo1NiBFRVQgTWF4 aW1lIFJpcGFyZCB3cm90ZToKPiBPbiBUaHUsIEphbiAxMSwgMjAxOCBhdCAwMzowNTowMVBNICsw MjAwLCBMYXVyZW50IFBpbmNoYXJ0IHdyb3RlOgo+ID4gT24gV2VkbmVzZGF5LCAxMCBKYW51YXJ5 IDIwMTggMTc6NTk6NDEgRUVUIE1heGltZSBSaXBhcmQgd3JvdGU6Cj4gPj4gVGhlIGRldm1fcmVn dWxhdG9yX2dldF9vcHRpb25hbCBmdW5jdGlvbiwgdW5saWtlIGl0IHdhcyBhc3N1bWVkIGluIHRo ZQo+ID4+IGNvbW1pdCBhMWM1NWJjY2Y2MDAgKCJkcm0vcGFuZWw6IGx2ZHM6IEFkZCBzdXBwb3J0 IGZvciB0aGUgcG93ZXItc3VwcGx5Cj4gPj4gcHJvcGVydHkiKSwgaXMgYWN0dWFsbHkgcmV0dXJu aW5nIGFuIGVycm9yIHBvaW50ZXIgd2l0aCAtRU5PREVWIGluc3RlYWQKPiA+PiBvZiBOVUxMIHdo ZW4gdGhlcmUncyBubyByZWd1bGF0b3IgdG8gZmluZC4KPiA+PiAKPiA+PiBNYWtlIHN1cmUgd2Ug aGFuZGxlIHRoYXQgY2FzZSBwcm9wZXJseS4KPiA+PiAKPiA+PiBGaXhlczogYTFjNTViY2NmNjAw ICgiZHJtL3BhbmVsOiBsdmRzOiBBZGQgc3VwcG9ydCBmb3IgdGhlIHBvd2VyLXN1cHBseQo+ID4+ IHByb3BlcnR5IikgU2lnbmVkLW9mZi1ieTogTWF4aW1lIFJpcGFyZAo+ID4+IDxtYXhpbWUucmlw YXJkQGZyZWUtZWxlY3Ryb25zLmNvbT4KPiA+PiAtLS0KPiA+PiAKPiA+PiAgZHJpdmVycy9ncHUv ZHJtL3BhbmVsL3BhbmVsLWx2ZHMuYyB8IDkgKysrKysrKy0tCj4gPj4gIDEgZmlsZSBjaGFuZ2Vk LCA3IGluc2VydGlvbnMoKyksIDIgZGVsZXRpb25zKC0pCj4gPj4gCj4gPj4gZGlmZiAtLWdpdCBh L2RyaXZlcnMvZ3B1L2RybS9wYW5lbC9wYW5lbC1sdmRzLmMKPiA+PiBiL2RyaXZlcnMvZ3B1L2Ry bS9wYW5lbC9wYW5lbC1sdmRzLmMgaW5kZXggNTdlMzhhOWU3YWI0Li45ZjQ2ZTcwOTVjMGUKPiA+ PiAxMDA2NDQKPiA+PiAtLS0gYS9kcml2ZXJzL2dwdS9kcm0vcGFuZWwvcGFuZWwtbHZkcy5jCj4g Pj4gKysrIGIvZHJpdmVycy9ncHUvZHJtL3BhbmVsL3BhbmVsLWx2ZHMuYwo+ID4+IEBAIC0yMTUs OCArMjE1LDEzIEBAIHN0YXRpYyBpbnQgcGFuZWxfbHZkc19wcm9iZShzdHJ1Y3QgcGxhdGZvcm1f ZGV2aWNlCj4gPj4gKnBkZXYpCj4gPj4gCWx2ZHMtPnN1cHBseSA9IGRldm1fcmVndWxhdG9yX2dl dF9vcHRpb25hbChsdmRzLT5kZXYsICJwb3dlciIpOwo+ID4+ICAJaWYgKElTX0VSUihsdmRzLT5z dXBwbHkpKSB7Cj4gPj4gIAkJcmV0ID0gUFRSX0VSUihsdmRzLT5zdXBwbHkpOwo+ID4+IC0JCWRl dl9lcnIobHZkcy0+ZGV2LCAiZmFpbGVkIHRvIHJlcXVlc3QgcmVndWxhdG9yOiAlZFxuIiwgcmV0 KTsKPiA+PiAtCQlyZXR1cm4gcmV0Owo+ID4+ICsKPiA+PiArCQlpZiAocmV0ICE9IC1FTk9ERVYp IHsKPiA+PiArCQkJZGV2X2VycihsdmRzLT5kZXYsICJmYWlsZWQgdG8gcmVxdWVzdCByZWd1bGF0 b3I6ICVkXG4iLCByZXQpOwo+ID4+ICsJCQlyZXR1cm4gcmV0Owo+ID4gCj4gPiBJIHdvdWxkbid0 IHByaW50IGFuIGVycm9yIG1lc3NhZ2UgaWYgcmV0ID09IC1FUFJPQkVfREVGRVIuCj4gPiAKPiA+ PiArCQl9IGVsc2Ugewo+ID4+ICsJCQlsdmRzLT5zdXBwbHkgPSBOVUxMOwo+ID4+ICsJCX0KPiA+ PiAgCX0KPiA+IAo+ID4gSG93IGFib3V0Cj4gPiAKPiA+IAlsdmRzLT5zdXBwbHkgPSBkZXZtX3Jl Z3VsYXRvcl9nZXRfb3B0aW9uYWwobHZkcy0+ZGV2LCAicG93ZXIiKTsKPiA+IAlpZiAoSVNfRVJS KGx2ZHMtPnN1cHBseSkpIHsKPiA+IAkJcmV0ID0gUFRSX0VSUihsdmRzLT5zdXBwbHkpOwo+ID4g CQlpZiAocmV0ICE9IC1FTk9ERVYpIHsKPiA+IAkJCWlmIChyZXQgPT0gLUVQUk9CRV9ERUZFUikK PiAKPiBJIGd1ZXNzIHRoYXQgd291bGQgYmUgIT0gLUVQUk9CRV9ERUZFUgoKT2YgY291cnNlLCBt eSBiYWQuCgo+ID4gCQkJCWRldl9lcnIobHZkcy0+ZGV2LCAiZmFpbGVkIHRvIHJlcXVlc3QgcmVn dWxhdG9yOiAlZFxuIiwgcmV0KTsKPiA+IAkJCXJldHVybiByZXQ7Cj4gPiAJCX0KPiA+IAkJCj4g PiAJCWx2ZHMtPnN1cHBseSA9IE5VTEw7Cj4gPiAJfQo+IAo+IE90aGVyd2lzZSwgaXQgd29ya3Mg Zm9yIG1lLgo+IAo+ID4gTXkgcHJlZmVyZW5jZSwgaG93ZXZlciwgd291bGQgYmUgZm9yIGRldm1f cmVndWxhdG9yX2dldF9vcHRpb25hbCgpIHRvCj4gPiByZXR1cm4gTlVMTCB3aGVuIG5vIHJlZ3Vs YXRvciBpcyBwcmVzZW50LiBUaGUgY3VycmVudCBpbXBsZW1lbnRhdGlvbgo+ID4gcmV0dXJucyAt RU5PREVWIGluIG11bHRpcGxlIGNhc2VzLCBtYWtpbmcgaXQgaW1wb3NzaWJsZSB0byBwcm9wZXJs eQo+ID4gZGlzY3JpbWluYXRlIGJldHdlZW4gaGF2aW5nIG5vIHJlZ3VsYXRvciBhbmQgbm90IGJl aW5nIGFibGUgdG8gZ2V0IHRoZQo+ID4gcmVndWxhdG9yIGR1ZSB0byBhbiBlcnJvci4KPiAKPiBJ dCB3b3VsZCBmZWVsIG1vcmUgaW50dWl0aXZlIHRvIG1lIHRvbywgYnV0IGl0IHdvdWxkIGFsc28g cmVxdWlyZSB0bwo+IGZpeCBtb3N0IG9mIHRoZSBjYWxsIHNpdGVzIHRoYXQgd291bGQgaGF2ZSBh IHNpbWlsYXIgcGF0dGVybi4KCk9mIGNvdXJzZS4gSSBkb24ndCBtZWFuIHdlIG5lZWQgdG8gZGVs YXkgdGhpcyBwYXRjaCwgYnV0IEkgc3RpbGwgdGhpbmsgaXQgCndvdWxkIGJlIGEgZ29vZCBBUEkg aW1wcm92ZW1lbnQgdGhhdCBjb3VsZCBiZSBkZXZlbG9wZWQgc2VwYXJhdGVseSAoYW5kIG9mIApj b3Vyc2UgSSB3b3VsZG4ndCBjb21wbGFpbiBpZiB5b3Ugdm9sdW50ZWVyZWQgOy0pKS4KCi0tIApS ZWdhcmRzLAoKTGF1cmVudCBQaW5jaGFydAoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX18KZHJpLWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMu ZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlz dGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933279AbeAKWGJ (ORCPT + 1 other); Thu, 11 Jan 2018 17:06:09 -0500 Received: from galahad.ideasonboard.com ([185.26.127.97]:58385 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933009AbeAKWGI (ORCPT ); Thu, 11 Jan 2018 17:06:08 -0500 From: Laurent Pinchart To: dri-devel@lists.freedesktop.org Cc: Maxime Ripard , linux-kernel@vger.kernel.org, Mark Brown , Daniel Vetter 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 List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: 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 > >> > >> --- > >> > >> 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