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:09:39 +0200 Message-ID: <5140293.enj6JbCqAe@avalon> References: <20180110155941.16109-1-maxime.ripard@free-electrons.com> <20180111131256.bmr6qbgzcmyfg4mj@flea.lan> <1579517.s7hJ1SlSuH@avalon> 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 80EED6E45A for ; Thu, 11 Jan 2018 22:09:41 +0000 (UTC) In-Reply-To: <1579517.s7hJ1SlSuH@avalon> 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 SGkgTWF4aW1lLAoKT24gRnJpZGF5LCAxMiBKYW51YXJ5IDIwMTggMDA6MDY6MDYgRUVUIExhdXJl bnQgUGluY2hhcnQgd3JvdGU6Cj4gT24gVGh1cnNkYXksIDExIEphbnVhcnkgMjAxOCAxNToxMjo1 NiBFRVQgTWF4aW1lIFJpcGFyZCB3cm90ZToKPiA+IE9uIFRodSwgSmFuIDExLCAyMDE4IGF0IDAz OjA1OjAxUE0gKzAyMDAsIExhdXJlbnQgUGluY2hhcnQgd3JvdGU6Cj4gPj4gT24gV2VkbmVzZGF5 LCAxMCBKYW51YXJ5IDIwMTggMTc6NTk6NDEgRUVUIE1heGltZSBSaXBhcmQgd3JvdGU6Cj4gPj4+ IFRoZSBkZXZtX3JlZ3VsYXRvcl9nZXRfb3B0aW9uYWwgZnVuY3Rpb24sIHVubGlrZSBpdCB3YXMg YXNzdW1lZCBpbiB0aGUKPiA+Pj4gY29tbWl0IGExYzU1YmNjZjYwMCAoImRybS9wYW5lbDogbHZk czogQWRkIHN1cHBvcnQgZm9yIHRoZSBwb3dlci1zdXBwbHkKPiA+Pj4gcHJvcGVydHkiKSwgaXMg YWN0dWFsbHkgcmV0dXJuaW5nIGFuIGVycm9yIHBvaW50ZXIgd2l0aCAtRU5PREVWIGluc3RlYWQK PiA+Pj4gb2YgTlVMTCB3aGVuIHRoZXJlJ3Mgbm8gcmVndWxhdG9yIHRvIGZpbmQuCj4gPj4+IAo+ ID4+PiBNYWtlIHN1cmUgd2UgaGFuZGxlIHRoYXQgY2FzZSBwcm9wZXJseS4KPiA+Pj4gCj4gPj4+ IEZpeGVzOiBhMWM1NWJjY2Y2MDAgKCJkcm0vcGFuZWw6IGx2ZHM6IEFkZCBzdXBwb3J0IGZvciB0 aGUgcG93ZXItc3VwcGx5Cj4gPj4+IHByb3BlcnR5IikgU2lnbmVkLW9mZi1ieTogTWF4aW1lIFJp cGFyZAo+ID4+PiA8bWF4aW1lLnJpcGFyZEBmcmVlLWVsZWN0cm9ucy5jb20+Cj4gPj4+IC0tLQo+ ID4+PiAKPiA+Pj4gIGRyaXZlcnMvZ3B1L2RybS9wYW5lbC9wYW5lbC1sdmRzLmMgfCA5ICsrKysr KystLQo+ID4+PiAgMSBmaWxlIGNoYW5nZWQsIDcgaW5zZXJ0aW9ucygrKSwgMiBkZWxldGlvbnMo LSkKPiA+Pj4gCj4gPj4+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dwdS9kcm0vcGFuZWwvcGFuZWwt bHZkcy5jCj4gPj4+IGIvZHJpdmVycy9ncHUvZHJtL3BhbmVsL3BhbmVsLWx2ZHMuYyBpbmRleCA1 N2UzOGE5ZTdhYjQuLjlmNDZlNzA5NWMwZQo+ID4+PiAxMDA2NDQKPiA+Pj4gLS0tIGEvZHJpdmVy cy9ncHUvZHJtL3BhbmVsL3BhbmVsLWx2ZHMuYwo+ID4+PiArKysgYi9kcml2ZXJzL2dwdS9kcm0v cGFuZWwvcGFuZWwtbHZkcy5jCj4gPj4+IEBAIC0yMTUsOCArMjE1LDEzIEBAIHN0YXRpYyBpbnQg cGFuZWxfbHZkc19wcm9iZShzdHJ1Y3QgcGxhdGZvcm1fZGV2aWNlCj4gPj4+ICpwZGV2KQo+ID4+ PiAJbHZkcy0+c3VwcGx5ID0gZGV2bV9yZWd1bGF0b3JfZ2V0X29wdGlvbmFsKGx2ZHMtPmRldiwg InBvd2VyIik7Cj4gPj4+ICAJaWYgKElTX0VSUihsdmRzLT5zdXBwbHkpKSB7Cj4gPj4+ICAJCXJl dCA9IFBUUl9FUlIobHZkcy0+c3VwcGx5KTsKPiA+Pj4gLQkJZGV2X2VycihsdmRzLT5kZXYsICJm YWlsZWQgdG8gcmVxdWVzdCByZWd1bGF0b3I6ICVkXG4iLCByZXQpOwo+ID4+PiAtCQlyZXR1cm4g cmV0Owo+ID4+PiArCj4gPj4+ICsJCWlmIChyZXQgIT0gLUVOT0RFVikgewo+ID4+PiArCQkJZGV2 X2VycihsdmRzLT5kZXYsICJmYWlsZWQgdG8gcmVxdWVzdCByZWd1bGF0b3I6ICVkXG4iLCByZXQp Owo+ID4+PiArCQkJcmV0dXJuIHJldDsKPiA+PiAKPiA+PiBJIHdvdWxkbid0IHByaW50IGFuIGVy cm9yIG1lc3NhZ2UgaWYgcmV0ID09IC1FUFJPQkVfREVGRVIuCj4gPj4gCj4gPj4+ICsJCX0gZWxz ZSB7Cj4gPj4+ICsJCQlsdmRzLT5zdXBwbHkgPSBOVUxMOwo+ID4+PiArCQl9Cj4gPj4+ICAJfQo+ ID4+IAo+ID4+IEhvdyBhYm91dAo+ID4+IAo+ID4+IAlsdmRzLT5zdXBwbHkgPSBkZXZtX3JlZ3Vs YXRvcl9nZXRfb3B0aW9uYWwobHZkcy0+ZGV2LCAicG93ZXIiKTsKPiA+PiAJaWYgKElTX0VSUihs dmRzLT5zdXBwbHkpKSB7Cj4gPj4gCQlyZXQgPSBQVFJfRVJSKGx2ZHMtPnN1cHBseSk7Cj4gPj4g CQlpZiAocmV0ICE9IC1FTk9ERVYpIHsKPiA+PiAJCQlpZiAocmV0ID09IC1FUFJPQkVfREVGRVIp Cj4gPiAKPiA+IEkgZ3Vlc3MgdGhhdCB3b3VsZCBiZSAhPSAtRVBST0JFX0RFRkVSCj4gCj4gT2Yg Y291cnNlLCBteSBiYWQuCj4gCj4gPj4gCQkJCWRldl9lcnIobHZkcy0+ZGV2LCAiZmFpbGVkIHRv IHJlcXVlc3QgcmVndWxhdG9yOiAlZFxuIiwgcmV0KTsKPiA+PiAJCQlyZXR1cm4gcmV0Owo+ID4+ IAkJfQo+ID4+IAkJCj4gPj4gCQlsdmRzLT5zdXBwbHkgPSBOVUxMOwo+ID4+IAl9Cj4gPiAKPiA+ IE90aGVyd2lzZSwgaXQgd29ya3MgZm9yIG1lLgoKV2l0aCB0aGUgYWJvdmUgY2hhbmdlLAoKUmV2 aWV3ZWQtYnk6IExhdXJlbnQgUGluY2hhcnQgPGxhdXJlbnQucGluY2hhcnRAaWRlYXNvbmJvYXJk LmNvbT4KVGVzdGVkLWJ5OiBMYXVyZW50IFBpbmNoYXJ0IDxsYXVyZW50LnBpbmNoYXJ0QGlkZWFz b25ib2FyZC5jb20+Cgo+ID4+IE15IHByZWZlcmVuY2UsIGhvd2V2ZXIsIHdvdWxkIGJlIGZvciBk ZXZtX3JlZ3VsYXRvcl9nZXRfb3B0aW9uYWwoKSB0bwo+ID4+IHJldHVybiBOVUxMIHdoZW4gbm8g cmVndWxhdG9yIGlzIHByZXNlbnQuIFRoZSBjdXJyZW50IGltcGxlbWVudGF0aW9uCj4gPj4gcmV0 dXJucyAtRU5PREVWIGluIG11bHRpcGxlIGNhc2VzLCBtYWtpbmcgaXQgaW1wb3NzaWJsZSB0byBw cm9wZXJseQo+ID4+IGRpc2NyaW1pbmF0ZSBiZXR3ZWVuIGhhdmluZyBubyByZWd1bGF0b3IgYW5k IG5vdCBiZWluZyBhYmxlIHRvIGdldCB0aGUKPiA+PiByZWd1bGF0b3IgZHVlIHRvIGFuIGVycm9y Lgo+ID4gCj4gPiBJdCB3b3VsZCBmZWVsIG1vcmUgaW50dWl0aXZlIHRvIG1lIHRvbywgYnV0IGl0 IHdvdWxkIGFsc28gcmVxdWlyZSB0bwo+ID4gZml4IG1vc3Qgb2YgdGhlIGNhbGwgc2l0ZXMgdGhh dCB3b3VsZCBoYXZlIGEgc2ltaWxhciBwYXR0ZXJuLgo+IAo+IE9mIGNvdXJzZS4gSSBkb24ndCBt ZWFuIHdlIG5lZWQgdG8gZGVsYXkgdGhpcyBwYXRjaCwgYnV0IEkgc3RpbGwgdGhpbmsgaXQKPiB3 b3VsZCBiZSBhIGdvb2QgQVBJIGltcHJvdmVtZW50IHRoYXQgY291bGQgYmUgZGV2ZWxvcGVkIHNl cGFyYXRlbHkgKGFuZCBvZgo+IGNvdXJzZSBJIHdvdWxkbid0IGNvbXBsYWluIGlmIHlvdSB2b2x1 bnRlZXJlZCA7LSkpLgoKLS0gClJlZ2FyZHMsCgpMYXVyZW50IFBpbmNoYXJ0CgpfX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBs aXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVz a3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933435AbeAKWJm (ORCPT + 1 other); Thu, 11 Jan 2018 17:09:42 -0500 Received: from galahad.ideasonboard.com ([185.26.127.97]:58449 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933147AbeAKWJl (ORCPT ); Thu, 11 Jan 2018 17:09:41 -0500 From: Laurent Pinchart To: dri-devel@lists.freedesktop.org Cc: Daniel Vetter , Maxime Ripard , Mark Brown , linux-kernel@vger.kernel.org Subject: Re: [PATCH] drm/panel: lvds: Handle the optional regulator case properly Date: Fri, 12 Jan 2018 00:09:39 +0200 Message-ID: <5140293.enj6JbCqAe@avalon> Organization: Ideas on Board Oy In-Reply-To: <1579517.s7hJ1SlSuH@avalon> References: <20180110155941.16109-1-maxime.ripard@free-electrons.com> <20180111131256.bmr6qbgzcmyfg4mj@flea.lan> <1579517.s7hJ1SlSuH@avalon> 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 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 > >>> > >>> --- > >>> > >>> 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 Tested-by: Laurent Pinchart > >> 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