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 494ACC4332F for ; Wed, 12 Jan 2022 21:32:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232990AbiALVch (ORCPT ); Wed, 12 Jan 2022 16:32:37 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33126 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232989AbiALVcg (ORCPT ); Wed, 12 Jan 2022 16:32:36 -0500 Received: from metis.ext.pengutronix.de (metis.ext.pengutronix.de [IPv6:2001:67c:670:201:290:27ff:fe1d:cc33]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A80C8C061748 for ; Wed, 12 Jan 2022 13:32:36 -0800 (PST) Received: from drehscheibe.grey.stw.pengutronix.de ([2a0a:edc0:0:c01:1d::a2]) by metis.ext.pengutronix.de with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1n7lDU-0003cg-GC; Wed, 12 Jan 2022 22:31:36 +0100 Received: from [2a0a:edc0:0:900:1d::77] (helo=ptz.office.stw.pengutronix.de) by drehscheibe.grey.stw.pengutronix.de with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1n7lDG-009xGd-W3; Wed, 12 Jan 2022 22:31:22 +0100 Received: from ukl by ptz.office.stw.pengutronix.de with local (Exim 4.92) (envelope-from ) id 1n7lDF-0004hw-SN; Wed, 12 Jan 2022 22:31:21 +0100 Date: Wed, 12 Jan 2022 22:31:21 +0100 From: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= To: Geert Uytterhoeven Cc: Andrew Lunn , Ulf Hansson , Vignesh Raghavendra , KVM list , "Rafael J. Wysocki" , linux-iio@vger.kernel.org, Linus Walleij , Amit Kucheria , ALSA Development Mailing List , Andy Shevchenko , Joakim Zhang , Guenter Roeck , Thierry Reding , MTD Maling List , Linux I2C , Miquel Raynal , linux-phy@lists.infradead.org, netdev@vger.kernel.org, linux-spi , Jiri Slaby , openipmi-developer@lists.sourceforge.net, Khuong Dinh , Florian Fainelli , Matthias Schiffer , Kamal Dasu , Lee Jones , Bartosz Golaszewski , Daniel Lezcano , Kishon Vijay Abraham I , bcm-kernel-feedback-list , "open list:SERIAL DRIVERS" , Jakub Kicinski , Zhang Rui , Jaroslav Kysela , Linux PWM List , Hans de Goede , Robert Richter , Saravanan Sekar , Corey Minyard , Linux PM list , Mauro Carvalho Chehab , John Garry , Peter Korsgaard , William Breathitt Gray , Mark Gross , "open list:GPIO SUBSYSTEM" , Alex Williamson , Mark Brown , Borislav Petkov , Sebastian Reichel , Matthias Brugger , Takashi Iwai , platform-driver-x86@vger.kernel.org, Benson Leung , Linux ARM , linux-edac@vger.kernel.org, Tony Luck , Mun Yew Tham , Eric Auger , Greg Kroah-Hartman , Yoshihiro Shimoda , Cornelia Huck , Linux MMC List , Liam Girdwood , Linux Kernel Mailing List , Linux-Renesas , Sergey Shtylyov , Vinod Koul , James Morse , Zha Qipeng , Pengutronix Kernel Team , Richard Weinberger , Niklas =?utf-8?Q?S=C3=B6derlund?= , linux-mediatek@lists.infradead.org, Brian Norris , "David S. Miller" Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional Message-ID: <20220112213121.5ruae5mxwj6t3qiy@pengutronix.de> References: <20220110195449.12448-1-s.shtylyov@omp.ru> <20220110195449.12448-2-s.shtylyov@omp.ru> <20220110201014.mtajyrfcfznfhyqm@pengutronix.de> <20220112085009.dbasceh3obfok5dc@pengutronix.de> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="c63mbqxqltqrb5xh" Content-Disposition: inline In-Reply-To: X-SA-Exim-Connect-IP: 2a0a:edc0:0:c01:1d::a2 X-SA-Exim-Mail-From: ukl@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-spi@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-spi@vger.kernel.org --c63mbqxqltqrb5xh Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jan 12, 2022 at 11:27:02AM +0100, Geert Uytterhoeven wrote: > Hi Uwe, >=20 > On Wed, Jan 12, 2022 at 9:51 AM Uwe Kleine-K=F6nig > wrote: > > On Wed, Jan 12, 2022 at 09:33:48AM +0100, Geert Uytterhoeven wrote: > > > On Mon, Jan 10, 2022 at 10:20 PM Andrew Lunn wrote: > > > > On Mon, Jan 10, 2022 at 09:10:14PM +0100, Uwe Kleine-K=F6nig wrote: > > > > > On Mon, Jan 10, 2022 at 10:54:48PM +0300, Sergey Shtylyov wrote: > > > > > > This patch is based on the former Andy Shevchenko's patch: > > > > > > > > > > > > https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shev= chenko@linux.intel.com/ > > > > > > > > > > > > Currently platform_get_irq_optional() returns an error code eve= n if IRQ > > > > > > resource simply has not been found. It prevents the callers fro= m being > > > > > > error code agnostic in their error handling: > > > > > > > > > > > > ret =3D platform_get_irq_optional(...); > > > > > > if (ret < 0 && ret !=3D -ENXIO) > > > > > > return ret; // respect deferred probe > > > > > > if (ret > 0) > > > > > > ...we get an IRQ... > > > > > > > > > > > > All other *_optional() APIs seem to return 0 or NULL in case an= optional > > > > > > resource is not available. Let's follow this good example, so t= hat the > > > > > > callers would look like: > > > > > > > > > > > > ret =3D platform_get_irq_optional(...); > > > > > > if (ret < 0) > > > > > > return ret; > > > > > > if (ret > 0) > > > > > > ...we get an IRQ... > > > > > > > > > > The difference to gpiod_get_optional (and most other *_optional) = is that > > > > > you can use the NULL value as if it were a valid GPIO. > > > > > > > > > > As this isn't given with for irqs, I don't think changing the ret= urn > > > > > value has much sense. > > > > > > > > We actually want platform_get_irq_optional() to look different to a= ll > > > > the other _optional() methods because it is not equivalent. If it > > > > looks the same, developers will assume it is the same, and get > > > > themselves into trouble. > > > > > > Developers already assume it is the same, and thus forget they have > > > to check against -ENXIO instead of zero. I agree that -ENXIO is unfortunate and -ENOENT would be more in line with other functions. I assume it's insane to want to change that. > > Is this an ack for renaming platform_get_irq_optional() to > > platform_get_irq_silent()? >=20 > No it isn't ;-) >=20 > If an optional IRQ is not present, drivers either just ignore it (e.g. > for devices that can have multiple interrupts or a single muxed IRQ), > or they have to resort to polling. For the latter, fall-back handling > is needed elsewhere in the driver. I think irq are not suitable for such a dummy handling. For clocks or GPIOs there are cases where just doing nothing in the absence of a certain optional clock or GPIO is fine. I checked a few users of platform_get_irq_optional() and I didn't find a single one that doesn't need to differentiate the irq and the no-irq case later. Do you know one? If you do, isn't that so exceptional that it doesn't justify the idea of a dummy irq value? So until proven otherwise I think platform_get_irq_optional() just isn't in the spirit of clk_get_optional() and gpiod_get_optional() because there are no use cases where a dummy value would be good enough. (Even if request_irq would be a noop for a dummy irq value.) The motivation why platform_get_irq_optional() was introduced was just that platform_get_irq() started to emit an error message (in commit 7723f4c5ecdb8d832f049f8483beb0d1081cedf6) and the (proportional) few drivers where the error message was bad needed a variant that doesn't emit the error message. Look at 31a8d8fa84c51d3ab00bf059158d5de6178cf890, the motivation to use platform_get_irq_optional() wasn't that it simplifies handling in the driver, but that it doesn't emit an error message. Or 8f5783ad9eb83747471f61f94dbe209fb9fb8a7d, or 2fd276c3ee4bd42eb034f8954964a5ae74187c6b, or 55cc33fab5ac9f7e2a97aa7c564e8b35355886d5. Just look at the output of git log -Splatform_get_irq_optional to find some more. That convinces me, that platform_get_irq_optional() is a bad name. The only difference to platform_get_irq is that it's silent. And returning a dummy irq value (which would make it aligned with the other _optional functions) isn't possible. > To me it sounds much more logical for the driver to check if an > optional irq is non-zero (available) or zero (not available), than to > sprinkle around checks for -ENXIO. In addition, you have to remember > that this one returns -ENXIO, while other APIs use -ENOENT or -ENOSYS > (or some other error code) to indicate absence. I thought not having > to care about the actual error code was the main reason behind the > introduction of the *_optional() APIs. No, the main benefit of gpiod_get_optional() (and clk_get_optional()) is that you can handle an absent GPIO (or clk) as if it were available. Best regards Uwe --=20 Pengutronix e.K. | Uwe Kleine-K=F6nig | Industrial Linux Solutions | https://www.pengutronix.de/ | --c63mbqxqltqrb5xh Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEfnIqFpAYrP8+dKQLwfwUeK3K7AkFAmHfSKAACgkQwfwUeK3K 7AlLYgf/TzHojP4Z3A1x+jOxObpLjnDBfLk/J2fJ3fPc7MQFDJwGCubx9/Taculx 9FL859wRU0HEqwefh3A61whyx3wDxBePJWJBBvU1jVZE12XHbvfARqHXYlZH3/rs +NUE/+WdKRh5YlDkOacjJ2x0lj7zudpEqvRquEuCaHNFX1bshPpw723FFZSyVLV3 8sZFHYevwi62q3h1gUPq6tUZib+WVnmCnladf6UYgGxgJQLu/YdvCm5+lp6N6H8u orUVG5PWROmD0F2c504T2qCD7O0hwj+667BfsU5JBAODQJm8dB47BGxjdoU52a2F mMckMccinC+jzqqzaJB4DTX2lMD57Q== =ndjr -----END PGP SIGNATURE----- --c63mbqxqltqrb5xh--