From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id E74DEEE4992 for ; Fri, 18 Aug 2023 20:10:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1379237AbjHRUKF (ORCPT ); Fri, 18 Aug 2023 16:10:05 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36640 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1379981AbjHRUKD (ORCPT ); Fri, 18 Aug 2023 16:10:03 -0400 Received: from pandora.armlinux.org.uk (unknown [IPv6:2001:4d48:ad52:32c8:5054:ff:fe00:142]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id ACED43C0A for ; Fri, 18 Aug 2023 13:09:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=armlinux.org.uk; s=pandora-2019; h=Sender:In-Reply-To: Content-Transfer-Encoding:Content-Type:MIME-Version:References:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=mA8j5yOq1jqvSsJ5rvO02vK/LMHfAYuokMDir9h3uqs=; b=0bAtkkDRyIMUGj/v7PSG8ADq77 q8RpPHa+kPe1zQs2oxFLETrp3rzsMvnltfG/Ic4VZ2oSbR9DdVVINVwrC4JYKN/CmU9iTCdbDa0DJ K61tF9Q74SR4QE9eJMl5Z75BE8LPhxruOpVAuXZgxJJjsE+S8mjmj5d0gTAQi2rsxHfuIKiKGs3Y4 KTmzfVdfUzzfLnfVT8dk4OwSMMHQGzh6/hYgVNkEnWW53d7WyJgcQbIJU1yOnL/g6v0wF4zk3aoRY 57NCE5k1+y8wls6/7O3G/sxNCXwSIxDVoJl8vWkl5dwt0LqVcGWV2dUGlXu6RPgX6R0h5hrvq/SFc AF+QvaMA==; Received: from shell.armlinux.org.uk ([fd8f:7570:feb6:1:5054:ff:fe00:4ec]:47448) by pandora.armlinux.org.uk with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1qX5n5-00063U-13; Fri, 18 Aug 2023 21:09:51 +0100 Received: from linux by shell.armlinux.org.uk with local (Exim 4.94.2) (envelope-from ) id 1qX5n2-0001wC-7u; Fri, 18 Aug 2023 21:09:48 +0100 Date: Fri, 18 Aug 2023 21:09:48 +0100 From: "Russell King (Oracle)" To: Andi Shyti Cc: Linus Walleij , Ruan Jinjie , linux-i2c@vger.kernel.org, linux-arm-kernel@lists.infradead.org, o.rempel@pengutronix.de, nicolas.ferre@microchip.com, leoyang.li@nxp.com, s.hauer@pengutronix.de, Codrin Ciubotariu , Alexandre Belloni , Claudiu Beznea , Oleksij Rempel , Pengutronix Kernel Team , Shawn Guo , Fabio Estevam , NXP Linux Team , Wolfram Sang , Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= Subject: Re: [PATCH -next v2 RESEND] I2C: Fix return value check for devm_pinctrl_get() Message-ID: References: <20230818074509.295220-1-ruanjinjie@huawei.com> <20230818192034.bgjmurn2rp7ngyel@intel.intel> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20230818192034.bgjmurn2rp7ngyel@intel.intel> Sender: Russell King (Oracle) Precedence: bulk List-ID: X-Mailing-List: linux-i2c@vger.kernel.org On Fri, Aug 18, 2023 at 09:20:34PM +0200, Andi Shyti wrote: > Hi, > > On Fri, Aug 18, 2023 at 06:42:11PM +0200, Linus Walleij wrote: > > On Fri, Aug 18, 2023 at 10:20 AM Russell King (Oracle) > > wrote: > > > On Fri, Aug 18, 2023 at 03:45:08PM +0800, Ruan Jinjie wrote: > > > > > > i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev); > > > > - if (!i2c_imx->pinctrl || IS_ERR(i2c_imx->pinctrl)) { > > > > + if (IS_ERR(i2c_imx->pinctrl)) { > > > > dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n"); > > > > return PTR_ERR(i2c_imx->pinctrl); > > > > } > > > > > > I haven't looked at the AT91 version, but... isn't the original code > > > entirely correct? > > > > > > If pinctrl is not available (thus devm_pinctrl_get() returns NULL) then > > > recovery can't work, because we can't switch the I2C pins between the > > > I2C controller and GPIO. So, isn't it quite correct to print > > > "can't get pinctrl, bus recovery not supported" because the I2C bus > > > can't be recovered without pinctrl? > > > > > > The PTR_ERR() is also fine - because if pinctrl is not present and > > > returns NULL, we'll end up returning zero, which is exactly what we > > > want. > > > > Oh, you're probably absolutely right about that. > > > > > The alternative would be to open code that, maybe with a more accurate > > > message: > > > > > > if (!i2c_imx->pinctrl) { > > > dev_info(&pdev->dev, "pinctrl unavailable, bus recovery not supported\n"); > > > return 0; > > > } > > > if (IS_ERR(i2c_imx->pinctrl) { > > > ... > > > > This is a way better patch. It makes the implicit explicit. > > we could also use > > if (IS_ERR_OR_NULL(i2c_imx->pinctrl)) > ... > > without changing any logic in the driver. IS_ERR_OR_NULL() - is a macro I personally hate, it causes a lot of trouble. I have mutt setup to mark IS_ERR_OR_NULL with a red background so it stands out in patches. It is utterly evil, and I really wish we could get rid of that damn macro. It also looks wrong. if (IS_ERR_OR_NULL(x)) return PTR_ERR(x); rings alarm bells for some people, because if x is NULL, then PTR_ERR(x) is zero. While this may be what is intended in this case, for a great many places in the kernel, this is a bug. So I can guarantee that _someone_ will come along and want to "fix" that to make the NULL case return an error code, and in doing so end up breaking the driver. So... no, just don't. This is why having two if() statements are a good idea, and is what Linus means by "making the implicit explicit" - because it then becomes absolutely obvious what we want to do in the NULL case, and what we want to do in the error case. There is none of this ambiguity that I point out above. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 44B77EE498F for ; Fri, 18 Aug 2023 20:10:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=u5hPOU/vPYD3CVvJ7NinemISjDshpPS10RDOQDX4vm4=; b=iLR5nSTpOkSW9x c51maUS0YrUINFDFm1qYYhUXNvOMs0uVhluwNMm25yHmLEpZfaBmMDu0KUb2xm6C9dJ224NhDhjAp QKFvY+exZ4yRpedBCk/odsKdViAjNawt17qu+mNNP9fuKcKwJFk0Sl3d1Xw30KMZZWIJgZcE6SfnO yvLMuMdjXbIcvc3QaWspxYY8KgWXiB/Y4tam+tNgdeztsWoLKJEGhEB1QjNPRo3Fx6yM4FkEpuFgY AHvdacxk1h07Jmd+mgyv2SCvp9gkvEr90iTbkb/bm+o8E7hvm9QuMfpUjq+F1S7l0Eh9Z7XGjTKS/ /Gm6eCBCR+kGO+AK5seQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qX5nS-009yvk-0t; Fri, 18 Aug 2023 20:10:14 +0000 Received: from [2001:4d48:ad52:32c8:5054:ff:fe00:142] (helo=pandora.armlinux.org.uk) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qX5nN-009yul-25 for linux-arm-kernel@lists.infradead.org; Fri, 18 Aug 2023 20:10:12 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=armlinux.org.uk; s=pandora-2019; h=Sender:In-Reply-To: Content-Transfer-Encoding:Content-Type:MIME-Version:References:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=mA8j5yOq1jqvSsJ5rvO02vK/LMHfAYuokMDir9h3uqs=; b=0bAtkkDRyIMUGj/v7PSG8ADq77 q8RpPHa+kPe1zQs2oxFLETrp3rzsMvnltfG/Ic4VZ2oSbR9DdVVINVwrC4JYKN/CmU9iTCdbDa0DJ K61tF9Q74SR4QE9eJMl5Z75BE8LPhxruOpVAuXZgxJJjsE+S8mjmj5d0gTAQi2rsxHfuIKiKGs3Y4 KTmzfVdfUzzfLnfVT8dk4OwSMMHQGzh6/hYgVNkEnWW53d7WyJgcQbIJU1yOnL/g6v0wF4zk3aoRY 57NCE5k1+y8wls6/7O3G/sxNCXwSIxDVoJl8vWkl5dwt0LqVcGWV2dUGlXu6RPgX6R0h5hrvq/SFc AF+QvaMA==; Received: from shell.armlinux.org.uk ([fd8f:7570:feb6:1:5054:ff:fe00:4ec]:47448) by pandora.armlinux.org.uk with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1qX5n5-00063U-13; Fri, 18 Aug 2023 21:09:51 +0100 Received: from linux by shell.armlinux.org.uk with local (Exim 4.94.2) (envelope-from ) id 1qX5n2-0001wC-7u; Fri, 18 Aug 2023 21:09:48 +0100 Date: Fri, 18 Aug 2023 21:09:48 +0100 From: "Russell King (Oracle)" To: Andi Shyti Subject: Re: [PATCH -next v2 RESEND] I2C: Fix return value check for devm_pinctrl_get() Message-ID: References: <20230818074509.295220-1-ruanjinjie@huawei.com> <20230818192034.bgjmurn2rp7ngyel@intel.intel> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230818192034.bgjmurn2rp7ngyel@intel.intel> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230818_131009_685533_064CF555 X-CRM114-Status: GOOD ( 26.91 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Alexandre Belloni , s.hauer@pengutronix.de, Fabio Estevam , Linus Walleij , Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= , leoyang.li@nxp.com, Wolfram Sang , o.rempel@pengutronix.de, linux-i2c@vger.kernel.org, Pengutronix Kernel Team , Claudiu Beznea , Codrin Ciubotariu , Oleksij Rempel , Shawn Guo , Ruan Jinjie , linux-arm-kernel@lists.infradead.org, NXP Linux Team Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org T24gRnJpLCBBdWcgMTgsIDIwMjMgYXQgMDk6MjA6MzRQTSArMDIwMCwgQW5kaSBTaHl0aSB3cm90 ZToKPiBIaSwKPiAKPiBPbiBGcmksIEF1ZyAxOCwgMjAyMyBhdCAwNjo0MjoxMVBNICswMjAwLCBM aW51cyBXYWxsZWlqIHdyb3RlOgo+ID4gT24gRnJpLCBBdWcgMTgsIDIwMjMgYXQgMTA6MjDigK9B TSBSdXNzZWxsIEtpbmcgKE9yYWNsZSkKPiA+IDxsaW51eEBhcm1saW51eC5vcmcudWs+IHdyb3Rl Ogo+ID4gPiBPbiBGcmksIEF1ZyAxOCwgMjAyMyBhdCAwMzo0NTowOFBNICswODAwLCBSdWFuIEpp bmppZSB3cm90ZToKPiA+IAo+ID4gPiA+ICAgICAgIGkyY19pbXgtPnBpbmN0cmwgPSBkZXZtX3Bp bmN0cmxfZ2V0KCZwZGV2LT5kZXYpOwo+ID4gPiA+IC0gICAgIGlmICghaTJjX2lteC0+cGluY3Ry bCB8fCBJU19FUlIoaTJjX2lteC0+cGluY3RybCkpIHsKPiA+ID4gPiArICAgICBpZiAoSVNfRVJS KGkyY19pbXgtPnBpbmN0cmwpKSB7Cj4gPiA+ID4gICAgICAgICAgICAgICBkZXZfaW5mbygmcGRl di0+ZGV2LCAiY2FuJ3QgZ2V0IHBpbmN0cmwsIGJ1cyByZWNvdmVyeSBub3Qgc3VwcG9ydGVkXG4i KTsKPiA+ID4gPiAgICAgICAgICAgICAgIHJldHVybiBQVFJfRVJSKGkyY19pbXgtPnBpbmN0cmwp Owo+ID4gPiA+ICAgICAgIH0KPiA+ID4KPiA+ID4gSSBoYXZlbid0IGxvb2tlZCBhdCB0aGUgQVQ5 MSB2ZXJzaW9uLCBidXQuLi4gaXNuJ3QgdGhlIG9yaWdpbmFsIGNvZGUKPiA+ID4gZW50aXJlbHkg Y29ycmVjdD8KPiA+ID4KPiA+ID4gSWYgcGluY3RybCBpcyBub3QgYXZhaWxhYmxlICh0aHVzIGRl dm1fcGluY3RybF9nZXQoKSByZXR1cm5zIE5VTEwpIHRoZW4KPiA+ID4gcmVjb3ZlcnkgY2FuJ3Qg d29yaywgYmVjYXVzZSB3ZSBjYW4ndCBzd2l0Y2ggdGhlIEkyQyBwaW5zIGJldHdlZW4gdGhlCj4g PiA+IEkyQyBjb250cm9sbGVyIGFuZCBHUElPLiBTbywgaXNuJ3QgaXQgcXVpdGUgY29ycmVjdCB0 byBwcmludAo+ID4gPiAiY2FuJ3QgZ2V0IHBpbmN0cmwsIGJ1cyByZWNvdmVyeSBub3Qgc3VwcG9y dGVkIiBiZWNhdXNlIHRoZSBJMkMgYnVzCj4gPiA+IGNhbid0IGJlIHJlY292ZXJlZCB3aXRob3V0 IHBpbmN0cmw/Cj4gPiA+Cj4gPiA+IFRoZSBQVFJfRVJSKCkgaXMgYWxzbyBmaW5lIC0gYmVjYXVz ZSBpZiBwaW5jdHJsIGlzIG5vdCBwcmVzZW50IGFuZAo+ID4gPiByZXR1cm5zIE5VTEwsIHdlJ2xs IGVuZCB1cCByZXR1cm5pbmcgemVybywgd2hpY2ggaXMgZXhhY3RseSB3aGF0IHdlCj4gPiA+IHdh bnQuCj4gPiAKPiA+IE9oLCB5b3UncmUgcHJvYmFibHkgYWJzb2x1dGVseSByaWdodCBhYm91dCB0 aGF0Lgo+ID4gCj4gPiA+IFRoZSBhbHRlcm5hdGl2ZSB3b3VsZCBiZSB0byBvcGVuIGNvZGUgdGhh dCwgbWF5YmUgd2l0aCBhIG1vcmUgYWNjdXJhdGUKPiA+ID4gbWVzc2FnZToKPiA+ID4KPiA+ID4g ICAgICAgICBpZiAoIWkyY19pbXgtPnBpbmN0cmwpIHsKPiA+ID4gICAgICAgICAgICAgICAgIGRl dl9pbmZvKCZwZGV2LT5kZXYsICJwaW5jdHJsIHVuYXZhaWxhYmxlLCBidXMgcmVjb3Zlcnkgbm90 IHN1cHBvcnRlZFxuIik7Cj4gPiA+ICAgICAgICAgICAgICAgICByZXR1cm4gMDsKPiA+ID4gICAg ICAgICB9Cj4gPiA+ICAgICAgICAgaWYgKElTX0VSUihpMmNfaW14LT5waW5jdHJsKSB7Cj4gPiA+ ICAgICAgICAgICAgICAgICAuLi4KPiA+IAo+ID4gVGhpcyBpcyBhIHdheSBiZXR0ZXIgcGF0Y2gu IEl0IG1ha2VzIHRoZSBpbXBsaWNpdCBleHBsaWNpdC4KPiAKPiB3ZSBjb3VsZCBhbHNvIHVzZQo+ IAo+IAlpZiAoSVNfRVJSX09SX05VTEwoaTJjX2lteC0+cGluY3RybCkpCj4gCQkuLi4KPiAKPiB3 aXRob3V0IGNoYW5naW5nIGFueSBsb2dpYyBpbiB0aGUgZHJpdmVyLgoKSVNfRVJSX09SX05VTEwo KSAtIGlzIGEgbWFjcm8gSSBwZXJzb25hbGx5IGhhdGUsIGl0IGNhdXNlcyBhIGxvdCBvZgp0cm91 YmxlLiBJIGhhdmUgbXV0dCBzZXR1cCB0byBtYXJrIElTX0VSUl9PUl9OVUxMIHdpdGggYSByZWQg YmFja2dyb3VuZApzbyBpdCBzdGFuZHMgb3V0IGluIHBhdGNoZXMuIEl0IGlzIHV0dGVybHkgZXZp bCwgYW5kIEkgcmVhbGx5IHdpc2ggd2UKY291bGQgZ2V0IHJpZCBvZiB0aGF0IGRhbW4gbWFjcm8u CgpJdCBhbHNvIGxvb2tzIHdyb25nLgoKCWlmIChJU19FUlJfT1JfTlVMTCh4KSkKCQlyZXR1cm4g UFRSX0VSUih4KTsKCnJpbmdzIGFsYXJtIGJlbGxzIGZvciBzb21lIHBlb3BsZSwgYmVjYXVzZSBp ZiB4IGlzIE5VTEwsIHRoZW4KUFRSX0VSUih4KSBpcyB6ZXJvLgoKV2hpbGUgdGhpcyBtYXkgYmUg d2hhdCBpcyBpbnRlbmRlZCBpbiB0aGlzIGNhc2UsIGZvciBhIGdyZWF0IG1hbnkKcGxhY2VzIGlu IHRoZSBrZXJuZWwsIHRoaXMgaXMgYSBidWcuIFNvIEkgY2FuIGd1YXJhbnRlZSB0aGF0Cl9zb21l b25lXyB3aWxsIGNvbWUgYWxvbmcgYW5kIHdhbnQgdG8gImZpeCIgdGhhdCB0byBtYWtlIHRoZSBO VUxMCmNhc2UgcmV0dXJuIGFuIGVycm9yIGNvZGUsIGFuZCBpbiBkb2luZyBzbyBlbmQgdXAgYnJl YWtpbmcgdGhlCmRyaXZlci4KClNvLi4uIG5vLCBqdXN0IGRvbid0LgoKVGhpcyBpcyB3aHkgaGF2 aW5nIHR3byBpZigpIHN0YXRlbWVudHMgYXJlIGEgZ29vZCBpZGVhLCBhbmQgaXMKd2hhdCBMaW51 cyBtZWFucyBieSAibWFraW5nIHRoZSBpbXBsaWNpdCBleHBsaWNpdCIgLSBiZWNhdXNlIGl0CnRo ZW4gYmVjb21lcyBhYnNvbHV0ZWx5IG9idmlvdXMgd2hhdCB3ZSB3YW50IHRvIGRvIGluIHRoZSBO VUxMCmNhc2UsIGFuZCB3aGF0IHdlIHdhbnQgdG8gZG8gaW4gdGhlIGVycm9yIGNhc2UuCgpUaGVy ZSBpcyBub25lIG9mIHRoaXMgYW1iaWd1aXR5IHRoYXQgSSBwb2ludCBvdXQgYWJvdmUuCgotLSAK Uk1LJ3MgUGF0Y2ggc3lzdGVtOiBodHRwczovL3d3dy5hcm1saW51eC5vcmcudWsvZGV2ZWxvcGVy L3BhdGNoZXMvCkZUVFAgaXMgaGVyZSEgODBNYnBzIGRvd24gMTBNYnBzIHVwLiBEZWNlbnQgY29u bmVjdGl2aXR5IGF0IGxhc3QhCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fXwpsaW51eC1hcm0ta2VybmVsIG1haWxpbmcgbGlzdApsaW51eC1hcm0ta2VybmVs QGxpc3RzLmluZnJhZGVhZC5vcmcKaHR0cDovL2xpc3RzLmluZnJhZGVhZC5vcmcvbWFpbG1hbi9s aXN0aW5mby9saW51eC1hcm0ta2VybmVsCg==