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 6FDDFC433EF for ; Fri, 14 Jan 2022 20:25:40 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S244302AbiANUZk (ORCPT ); Fri, 14 Jan 2022 15:25:40 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47540 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S244298AbiANUZj (ORCPT ); Fri, 14 Jan 2022 15:25:39 -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 F1D70C061574 for ; Fri, 14 Jan 2022 12:25:38 -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 1n8T5w-0000w6-8j; Fri, 14 Jan 2022 21:22:44 +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 1n8T5j-00AJbK-DJ; Fri, 14 Jan 2022 21:22:30 +0100 Received: from ukl by ptz.office.stw.pengutronix.de with local (Exim 4.92) (envelope-from ) id 1n8T5h-0002Jl-M3; Fri, 14 Jan 2022 21:22:29 +0100 Date: Fri, 14 Jan 2022 21:22:26 +0100 From: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= To: Sergey Shtylyov 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 , Jaroslav Kysela , Guenter Roeck , Thierry Reding , MTD Maling List , Linux I2C , Miquel Raynal , linux-phy@lists.infradead.org, Jiri Slaby , openipmi-developer@lists.sourceforge.net, "David S. Miller" , Khuong Dinh , Florian Fainelli , Matthias Schiffer , Joakim Zhang , Kamal Dasu , Lee Jones , Bartosz Golaszewski , Daniel Lezcano , Kishon Vijay Abraham I , Geert Uytterhoeven , "open list:SERIAL DRIVERS" , Yoshihiro Shimoda , bcm-kernel-feedback-list , Zhang Rui , Linux PWM List , Robert Richter , Saravanan Sekar , Corey Minyard , Linux PM list , Liam Girdwood , Mauro Carvalho Chehab , John Garry , Peter Korsgaard , William Breathitt Gray , Mark Gross , Hans de Goede , Alex Williamson , Mark Brown , Borislav Petkov , Sebastian Reichel , Jakub Kicinski , 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 , "open list:GPIO SUBSYSTEM" , Cornelia Huck , Linux MMC List , Linux Kernel Mailing List , linux-spi , Linux-Renesas , 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 , netdev@vger.kernel.org Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional Message-ID: <20220114202226.ugzklxv4wzr6egwj@pengutronix.de> References: <20220110201014.mtajyrfcfznfhyqm@pengutronix.de> <20220112085009.dbasceh3obfok5dc@pengutronix.de> <20220112213121.5ruae5mxwj6t3qiy@pengutronix.de> <29f0c65d-77f2-e5b2-f6cc-422add8a707d@omp.ru> <20220114092557.jrkfx7ihg26ekzci@pengutronix.de> <61b80939-357d-14f5-df99-b8d102a4e1a1@omp.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="uk3tmcd6neo3zuxi" Content-Disposition: inline In-Reply-To: <61b80939-357d-14f5-df99-b8d102a4e1a1@omp.ru> 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-gpio@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-gpio@vger.kernel.org --uk3tmcd6neo3zuxi Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jan 14, 2022 at 10:14:10PM +0300, Sergey Shtylyov wrote: > On 1/14/22 12:25 PM, Uwe Kleine-K=F6nig wrote: >=20 > >>>>> 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 -ENOS= YS > >>>>> (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. > >> > >> Hm, I've just looked at these and must note that they match 1:1 with > >> platform_get_irq_optional(). Unfortunately, we can't however behave the > >> same way in request_irq() -- because it has to support IRQ0 for the sa= ke > >> of i8253 drivers in arch/... > >=20 > > Let me reformulate your statement to the IMHO equivalent: > >=20 > > If you set aside the differences between > > platform_get_irq_optional() and gpiod_get_optional(), >=20 > Sorry, I should make it clear this is actually the diff between a woul= d-be > platform_get_irq_optional() after my patch, not the current code... The similarity is that with your patch both gpiod_get_optional() and platform_get_irq_optional() return NULL and 0 on not-found. The relevant difference however is that for a gpiod NULL is a dummy value, while for irqs it's not. So the similarity is only syntactically, but not semantically. > > platform_get_irq_optional() is like gpiod_get_optional(). > >=20 > > The introduction of gpiod_get_optional() made it possible to simplify > > the following code: > >=20 > > reset_gpio =3D gpiod_get(...) > > if IS_ERR(reset_gpio): > > error =3D PTR_ERR(reset_gpio) > > if error !=3D -ENDEV: >=20 > ENODEV? Yes, typo. > > return error > > else: > > gpiod_set_direction(reset_gpiod, INACTIVE) > >=20 > > to > >=20 > > reset_gpio =3D gpiod_get_optional(....) > > if IS_ERR(reset_gpio): > > return reset_gpio > > gpiod_set_direction(reset_gpiod, INACTIVE) > >=20 > > and I never need to actually know if the reset_gpio actually exists. > > Either the line is put into its inactive state, or it doesn't exist and > > then gpiod_set_direction is a noop. For a regulator or a clk this works > > in a similar way. > >=20 > > However for an interupt this cannot work. You will always have to check > > if the irq is actually there or not because if it's not you cannot just > > ignore that. So there is no benefit of an optional irq. > >=20 > > Leaving error message reporting aside, the introduction of > > platform_get_irq_optional() allows to change > >=20 > > irq =3D platform_get_irq(...); > > if (irq < 0 && irq !=3D -ENXIO) { > > return irq; > > } else if (irq >=3D 0) { >=20 > Rather (irq > 0) actually, IRQ0 is considered invalid (but still retur= ned). This is a topic I don't feel strong for, so I'm sloppy here. If changing this is all that is needed to convince you of my point ... > > ... setup irq operation ... > > } else { /* irq =3D=3D -ENXIO */ > > ... setup polling ... > > } > >=20 > > to > > =09 > > irq =3D platform_get_irq_optional(...); > > if (irq < 0 && irq !=3D -ENXIO) { > > return irq; > > } else if (irq >=3D 0) { > > ... setup irq operation ... > > } else { /* irq =3D=3D -ENXIO */ > > ... setup polling ... > > } > >=20 > > which isn't a win. When changing the return value as you suggest, it can > > be changed instead to: > >=20 > > irq =3D platform_get_irq_optional(...); > > if (irq < 0) { > > return irq; > > } else if (irq > 0) { > > ... setup irq operation ... > > } else { /* irq =3D=3D 0 */ > > ... setup polling ... > > } > >=20 > > which is a tad nicer. If that is your goal however I ask you to also > > change the semantic of platform_get_irq() to return 0 on "not found". >=20 > Well, I'm not totally opposed to that... but would there be a conside= rable win? Well, compared to your suggestion of making platform_get_irq_optional() return 0 on "not-found" the considerable win would be that platform_get_irq_optional() and platform_get_irq() are not different just because platform_get_irq() is to hard to change. > Anyway, we should 1st stop returning 0 for "valid" IRQs -- this is done b= y my patch > the discussed patch series are atop of. >=20 > > Note the win is considerably less compared to gpiod_get_optional vs >=20 > If there's any at all... We'd basically have to touch /all/ platform_g= et_irq() > calls (and get an even larger CC list ;-)). You got me wrong here. I meant that even if you change both platform_get_irq() and platform_get_irq_optional() to return 0 on "not-found", the win is small compared to the benefit of having both clk_get() and clk_get_optional(). > > gpiod_get however. And then it still lacks the semantic of a dummy irq > > which IMHO forfeits the right to call it ..._optional(). >=20 > Not quite grasping it... Why e.g. clk_get() doesn't return 0 for a not= found clock? Because NULL is not an error value for clk and when calling clk_get() you want a failure when the clk you asked for isn't available. Sure you could do the following in a case where you want to insist the clk to be actually available: clk =3D clk_get_optional(...) if (IS_ERR_OR_NULL(clk)) { err =3D PTR_ERR(clk) || -ENODEV; return dev_err_probe(dev, err, ....); } but this is more ugly than clk =3D clk_get(...) if (IS_ERR(clk)) { err =3D PTR_ERR(clk); return dev_err_probe(dev, err, ....); } Additionally the first usage would hard-code in the drivers that NULL is the dummy value which you might want to consider a layer violation. You have to understand that for clk (and regulator and gpiod) NULL is a valid descriptor that can actually be used, it just has no effect. So this is a convenience value for the case "If the clk/regulator/gpiod in question isn't available, there is nothing to do". This is what makes clk_get_optional() and the others really useful and justifies their existence. This doesn't apply to platform_get_irq_optional(). So clk_get() is sane and sensible for cases where you need the clk to be there. It doesn't emit an error message, because the caller knows better if it's worth an error message and in some cases the caller can also emit a better error message than clk_get() itself. clk_get_optional() is sane and sensible for cases where the clk might be absent and it helps you because you don't have to differentiate between "not found" and "there is an actual resource". The reason for platform_get_irq_optional()'s existence is just that platform_get_irq() emits an error message which is wrong or suboptimal in some cases (and IMHO is platform_get_irq() root fault). It doesn't simplify handling the "not found" case. So let's not pretend by the choice of function names that there is a similarity between clk_get() + clk_get_optional() and platform_get_irq() + platform_get_irq_optional(). And as you cannot change platform_get_irq_optional() to return a working dummy value, IMHO the only sane way out is renaming it. Best regards Uwe --=20 Pengutronix e.K. | Uwe Kleine-K=F6nig | Industrial Linux Solutions | https://www.pengutronix.de/ | --uk3tmcd6neo3zuxi Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEfnIqFpAYrP8+dKQLwfwUeK3K7AkFAmHh230ACgkQwfwUeK3K 7AlWNwf/bRFJlZD6u0VGoy9sl0BdPd/mUmPL35/vP2hJR8x4u+01YZ3J5DPX9kIK pE1ufbhwUnqJgelbz3N4R/r7kog28Jsz6cHn+cw2adtMIJQfl/xpk1C2Rsk6K3bO KHClfmbHJxl7kNt7wdN0jocSaun5fy8o5qNdvZ3+tvShyZNbWFPRHeiAhVTQxKCn qLNOfy/yFDXAUKKLTeXBnSCb8PbfjnDYhbGZpNiXgwHirXAc3aUM2PL9LSJdJeyb MlXBBJWUwdu7JXIo6+5KJI3txCmuKSByoERP3yu9BirMSfvEu+0QBc8IvwL1/j4z FjtcKcDXPQ/+lDBRPl94fRT8j8cK5Q== =uAhV -----END PGP SIGNATURE----- --uk3tmcd6neo3zuxi-- 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 8C724C433F5 for ; Fri, 14 Jan 2022 20:26:12 +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-Type: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: In-Reply-To:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date: Reply-To:Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date :Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=OL9tXP0kmRJI3PKcvovtxjaeQPHCenMIy1JSdH90rGA=; b=4WkKr1h/XH/mC9cMTcdnXlHjLp VDzVANf3eW7kEjpa2nc6VtraGpvWIt6YQyKGqrFfZrH9KMgJSb46XwqJGzEOw1Q+koM316tkbvytV aS9T0mj6giDQpneB6ZTgA8ulVe8scDW+HMs6DmNmPdjUV/hGea5I7L5wpdA11o8+27WNi/fogKevX o0CmmUfG2OnhQLRw23x7i182Bd7+/CHmuK20gii9HvUWCD9bFBcNqHcTUpIKWySScqdXKL8WHsw/R 7FDZFFqp3uFSlyvIpBQISPMhEXgeaHDbOSzu8tbz5emaaFem93GbIcu9P2kjL92SnD9FYKN6RKkBf 6xHDk1vg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1n8T8W-00ABZM-4w; Fri, 14 Jan 2022 20:25:24 +0000 Received: from metis.ext.pengutronix.de ([2001:67c:670:201:290:27ff:fe1d:cc33]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1n8T8S-00ABYa-EH for linux-mtd@lists.infradead.org; Fri, 14 Jan 2022 20:25:22 +0000 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 1n8T5w-0000w6-8j; Fri, 14 Jan 2022 21:22:44 +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 1n8T5j-00AJbK-DJ; Fri, 14 Jan 2022 21:22:30 +0100 Received: from ukl by ptz.office.stw.pengutronix.de with local (Exim 4.92) (envelope-from ) id 1n8T5h-0002Jl-M3; Fri, 14 Jan 2022 21:22:29 +0100 Date: Fri, 14 Jan 2022 21:22:26 +0100 From: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= To: Sergey Shtylyov 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 , Jaroslav Kysela , Guenter Roeck , Thierry Reding , MTD Maling List , Linux I2C , Miquel Raynal , linux-phy@lists.infradead.org, Jiri Slaby , openipmi-developer@lists.sourceforge.net, "David S. Miller" , Khuong Dinh , Florian Fainelli , Matthias Schiffer , Joakim Zhang , Kamal Dasu , Lee Jones , Bartosz Golaszewski , Daniel Lezcano , Kishon Vijay Abraham I , Geert Uytterhoeven , "open list:SERIAL DRIVERS" , Yoshihiro Shimoda , bcm-kernel-feedback-list , Zhang Rui , Linux PWM List , Robert Richter , Saravanan Sekar , Corey Minyard , Linux PM list , Liam Girdwood , Mauro Carvalho Chehab , John Garry , Peter Korsgaard , William Breathitt Gray , Mark Gross , Hans de Goede , Alex Williamson , Mark Brown , Borislav Petkov , Sebastian Reichel , Jakub Kicinski , 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 , "open list:GPIO SUBSYSTEM" , Cornelia Huck , Linux MMC List , Linux Kernel Mailing List , linux-spi , Linux-Renesas , 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 , netdev@vger.kernel.org Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional Message-ID: <20220114202226.ugzklxv4wzr6egwj@pengutronix.de> References: <20220110201014.mtajyrfcfznfhyqm@pengutronix.de> <20220112085009.dbasceh3obfok5dc@pengutronix.de> <20220112213121.5ruae5mxwj6t3qiy@pengutronix.de> <29f0c65d-77f2-e5b2-f6cc-422add8a707d@omp.ru> <20220114092557.jrkfx7ihg26ekzci@pengutronix.de> <61b80939-357d-14f5-df99-b8d102a4e1a1@omp.ru> MIME-Version: 1.0 In-Reply-To: <61b80939-357d-14f5-df99-b8d102a4e1a1@omp.ru> 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-mtd@lists.infradead.org X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220114_122520_678796_E9083351 X-CRM114-Status: GOOD ( 60.25 ) X-BeenThere: linux-mtd@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: multipart/mixed; boundary="===============5250595641059575529==" Sender: "linux-mtd" Errors-To: linux-mtd-bounces+linux-mtd=archiver.kernel.org@lists.infradead.org --===============5250595641059575529== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="uk3tmcd6neo3zuxi" Content-Disposition: inline --uk3tmcd6neo3zuxi Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jan 14, 2022 at 10:14:10PM +0300, Sergey Shtylyov wrote: > On 1/14/22 12:25 PM, Uwe Kleine-K=F6nig wrote: >=20 > >>>>> 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 -ENOS= YS > >>>>> (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. > >> > >> Hm, I've just looked at these and must note that they match 1:1 with > >> platform_get_irq_optional(). Unfortunately, we can't however behave the > >> same way in request_irq() -- because it has to support IRQ0 for the sa= ke > >> of i8253 drivers in arch/... > >=20 > > Let me reformulate your statement to the IMHO equivalent: > >=20 > > If you set aside the differences between > > platform_get_irq_optional() and gpiod_get_optional(), >=20 > Sorry, I should make it clear this is actually the diff between a woul= d-be > platform_get_irq_optional() after my patch, not the current code... The similarity is that with your patch both gpiod_get_optional() and platform_get_irq_optional() return NULL and 0 on not-found. The relevant difference however is that for a gpiod NULL is a dummy value, while for irqs it's not. So the similarity is only syntactically, but not semantically. > > platform_get_irq_optional() is like gpiod_get_optional(). > >=20 > > The introduction of gpiod_get_optional() made it possible to simplify > > the following code: > >=20 > > reset_gpio =3D gpiod_get(...) > > if IS_ERR(reset_gpio): > > error =3D PTR_ERR(reset_gpio) > > if error !=3D -ENDEV: >=20 > ENODEV? Yes, typo. > > return error > > else: > > gpiod_set_direction(reset_gpiod, INACTIVE) > >=20 > > to > >=20 > > reset_gpio =3D gpiod_get_optional(....) > > if IS_ERR(reset_gpio): > > return reset_gpio > > gpiod_set_direction(reset_gpiod, INACTIVE) > >=20 > > and I never need to actually know if the reset_gpio actually exists. > > Either the line is put into its inactive state, or it doesn't exist and > > then gpiod_set_direction is a noop. For a regulator or a clk this works > > in a similar way. > >=20 > > However for an interupt this cannot work. You will always have to check > > if the irq is actually there or not because if it's not you cannot just > > ignore that. So there is no benefit of an optional irq. > >=20 > > Leaving error message reporting aside, the introduction of > > platform_get_irq_optional() allows to change > >=20 > > irq =3D platform_get_irq(...); > > if (irq < 0 && irq !=3D -ENXIO) { > > return irq; > > } else if (irq >=3D 0) { >=20 > Rather (irq > 0) actually, IRQ0 is considered invalid (but still retur= ned). This is a topic I don't feel strong for, so I'm sloppy here. If changing this is all that is needed to convince you of my point ... > > ... setup irq operation ... > > } else { /* irq =3D=3D -ENXIO */ > > ... setup polling ... > > } > >=20 > > to > > =09 > > irq =3D platform_get_irq_optional(...); > > if (irq < 0 && irq !=3D -ENXIO) { > > return irq; > > } else if (irq >=3D 0) { > > ... setup irq operation ... > > } else { /* irq =3D=3D -ENXIO */ > > ... setup polling ... > > } > >=20 > > which isn't a win. When changing the return value as you suggest, it can > > be changed instead to: > >=20 > > irq =3D platform_get_irq_optional(...); > > if (irq < 0) { > > return irq; > > } else if (irq > 0) { > > ... setup irq operation ... > > } else { /* irq =3D=3D 0 */ > > ... setup polling ... > > } > >=20 > > which is a tad nicer. If that is your goal however I ask you to also > > change the semantic of platform_get_irq() to return 0 on "not found". >=20 > Well, I'm not totally opposed to that... but would there be a conside= rable win? Well, compared to your suggestion of making platform_get_irq_optional() return 0 on "not-found" the considerable win would be that platform_get_irq_optional() and platform_get_irq() are not different just because platform_get_irq() is to hard to change. > Anyway, we should 1st stop returning 0 for "valid" IRQs -- this is done b= y my patch > the discussed patch series are atop of. >=20 > > Note the win is considerably less compared to gpiod_get_optional vs >=20 > If there's any at all... We'd basically have to touch /all/ platform_g= et_irq() > calls (and get an even larger CC list ;-)). You got me wrong here. I meant that even if you change both platform_get_irq() and platform_get_irq_optional() to return 0 on "not-found", the win is small compared to the benefit of having both clk_get() and clk_get_optional(). > > gpiod_get however. And then it still lacks the semantic of a dummy irq > > which IMHO forfeits the right to call it ..._optional(). >=20 > Not quite grasping it... Why e.g. clk_get() doesn't return 0 for a not= found clock? Because NULL is not an error value for clk and when calling clk_get() you want a failure when the clk you asked for isn't available. Sure you could do the following in a case where you want to insist the clk to be actually available: clk =3D clk_get_optional(...) if (IS_ERR_OR_NULL(clk)) { err =3D PTR_ERR(clk) || -ENODEV; return dev_err_probe(dev, err, ....); } but this is more ugly than clk =3D clk_get(...) if (IS_ERR(clk)) { err =3D PTR_ERR(clk); return dev_err_probe(dev, err, ....); } Additionally the first usage would hard-code in the drivers that NULL is the dummy value which you might want to consider a layer violation. You have to understand that for clk (and regulator and gpiod) NULL is a valid descriptor that can actually be used, it just has no effect. So this is a convenience value for the case "If the clk/regulator/gpiod in question isn't available, there is nothing to do". This is what makes clk_get_optional() and the others really useful and justifies their existence. This doesn't apply to platform_get_irq_optional(). So clk_get() is sane and sensible for cases where you need the clk to be there. It doesn't emit an error message, because the caller knows better if it's worth an error message and in some cases the caller can also emit a better error message than clk_get() itself. clk_get_optional() is sane and sensible for cases where the clk might be absent and it helps you because you don't have to differentiate between "not found" and "there is an actual resource". The reason for platform_get_irq_optional()'s existence is just that platform_get_irq() emits an error message which is wrong or suboptimal in some cases (and IMHO is platform_get_irq() root fault). It doesn't simplify handling the "not found" case. So let's not pretend by the choice of function names that there is a similarity between clk_get() + clk_get_optional() and platform_get_irq() + platform_get_irq_optional(). And as you cannot change platform_get_irq_optional() to return a working dummy value, IMHO the only sane way out is renaming it. Best regards Uwe --=20 Pengutronix e.K. | Uwe Kleine-K=F6nig | Industrial Linux Solutions | https://www.pengutronix.de/ | --uk3tmcd6neo3zuxi Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEfnIqFpAYrP8+dKQLwfwUeK3K7AkFAmHh230ACgkQwfwUeK3K 7AlWNwf/bRFJlZD6u0VGoy9sl0BdPd/mUmPL35/vP2hJR8x4u+01YZ3J5DPX9kIK pE1ufbhwUnqJgelbz3N4R/r7kog28Jsz6cHn+cw2adtMIJQfl/xpk1C2Rsk6K3bO KHClfmbHJxl7kNt7wdN0jocSaun5fy8o5qNdvZ3+tvShyZNbWFPRHeiAhVTQxKCn qLNOfy/yFDXAUKKLTeXBnSCb8PbfjnDYhbGZpNiXgwHirXAc3aUM2PL9LSJdJeyb MlXBBJWUwdu7JXIo6+5KJI3txCmuKSByoERP3yu9BirMSfvEu+0QBc8IvwL1/j4z FjtcKcDXPQ/+lDBRPl94fRT8j8cK5Q== =uAhV -----END PGP SIGNATURE----- --uk3tmcd6neo3zuxi-- --===============5250595641059575529== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ --===============5250595641059575529==-- 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 7F2CCC433F5 for ; Fri, 14 Jan 2022 20:25:46 +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-Type: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: In-Reply-To:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date: Reply-To:Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date :Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=otYeTyMpCD1l0kRdqlutm3S5NFnOoR7sn0JI/RJt48Y=; b=Uv1c+uW0T5cNOf1PpVdm3BvuL6 FtIHi+fU6CPe9hWgK0NSwf1FvsQ+ScsIv5d0jM/FunGkXV+YMXDR4HDjZ+dJcxk0TvXBbO/4J5sZP WuH6AgKwBLLPPUs8Rny5ETpdkE3z+Ov6+ZbUAX/snUiQADnoq4q+6HrFVtz/A8OLpMWhIgONoBThM 37jGRv+g4uDAdoENJ1iv17MnzUrWl3g8POeCucNHk/UD4R1hDXOYJXlXZiVQOPoJCsvrHEDs/U/jO tl6FwL6rkUv0/AWz6KnITHTodrcVnCN7R1nShccsgoJp3DkW9kcVr3tj/8qPa3tlWipxIh//vJgCp v/k7ZcBQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1n8T8i-00ABap-Hp; Fri, 14 Jan 2022 20:25:36 +0000 Received: from metis.ext.pengutronix.de ([2001:67c:670:201:290:27ff:fe1d:cc33]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1n8T8Y-00ABZk-Qt for linux-mediatek@lists.infradead.org; Fri, 14 Jan 2022 20:25:29 +0000 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 1n8T5w-0000w6-8j; Fri, 14 Jan 2022 21:22:44 +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 1n8T5j-00AJbK-DJ; Fri, 14 Jan 2022 21:22:30 +0100 Received: from ukl by ptz.office.stw.pengutronix.de with local (Exim 4.92) (envelope-from ) id 1n8T5h-0002Jl-M3; Fri, 14 Jan 2022 21:22:29 +0100 Date: Fri, 14 Jan 2022 21:22:26 +0100 From: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= To: Sergey Shtylyov 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 , Jaroslav Kysela , Guenter Roeck , Thierry Reding , MTD Maling List , Linux I2C , Miquel Raynal , linux-phy@lists.infradead.org, Jiri Slaby , openipmi-developer@lists.sourceforge.net, "David S. Miller" , Khuong Dinh , Florian Fainelli , Matthias Schiffer , Joakim Zhang , Kamal Dasu , Lee Jones , Bartosz Golaszewski , Daniel Lezcano , Kishon Vijay Abraham I , Geert Uytterhoeven , "open list:SERIAL DRIVERS" , Yoshihiro Shimoda , bcm-kernel-feedback-list , Zhang Rui , Linux PWM List , Robert Richter , Saravanan Sekar , Corey Minyard , Linux PM list , Liam Girdwood , Mauro Carvalho Chehab , John Garry , Peter Korsgaard , William Breathitt Gray , Mark Gross , Hans de Goede , Alex Williamson , Mark Brown , Borislav Petkov , Sebastian Reichel , Jakub Kicinski , 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 , "open list:GPIO SUBSYSTEM" , Cornelia Huck , Linux MMC List , Linux Kernel Mailing List , linux-spi , Linux-Renesas , 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 , netdev@vger.kernel.org Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional Message-ID: <20220114202226.ugzklxv4wzr6egwj@pengutronix.de> References: <20220110201014.mtajyrfcfznfhyqm@pengutronix.de> <20220112085009.dbasceh3obfok5dc@pengutronix.de> <20220112213121.5ruae5mxwj6t3qiy@pengutronix.de> <29f0c65d-77f2-e5b2-f6cc-422add8a707d@omp.ru> <20220114092557.jrkfx7ihg26ekzci@pengutronix.de> <61b80939-357d-14f5-df99-b8d102a4e1a1@omp.ru> MIME-Version: 1.0 In-Reply-To: <61b80939-357d-14f5-df99-b8d102a4e1a1@omp.ru> 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-mediatek@lists.infradead.org X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220114_122527_049810_C0353AFE X-CRM114-Status: GOOD ( 60.32 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: multipart/mixed; boundary="===============6257537732717672435==" Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org --===============6257537732717672435== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="uk3tmcd6neo3zuxi" Content-Disposition: inline --uk3tmcd6neo3zuxi Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jan 14, 2022 at 10:14:10PM +0300, Sergey Shtylyov wrote: > On 1/14/22 12:25 PM, Uwe Kleine-K=F6nig wrote: >=20 > >>>>> 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 -ENOS= YS > >>>>> (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. > >> > >> Hm, I've just looked at these and must note that they match 1:1 with > >> platform_get_irq_optional(). Unfortunately, we can't however behave the > >> same way in request_irq() -- because it has to support IRQ0 for the sa= ke > >> of i8253 drivers in arch/... > >=20 > > Let me reformulate your statement to the IMHO equivalent: > >=20 > > If you set aside the differences between > > platform_get_irq_optional() and gpiod_get_optional(), >=20 > Sorry, I should make it clear this is actually the diff between a woul= d-be > platform_get_irq_optional() after my patch, not the current code... The similarity is that with your patch both gpiod_get_optional() and platform_get_irq_optional() return NULL and 0 on not-found. The relevant difference however is that for a gpiod NULL is a dummy value, while for irqs it's not. So the similarity is only syntactically, but not semantically. > > platform_get_irq_optional() is like gpiod_get_optional(). > >=20 > > The introduction of gpiod_get_optional() made it possible to simplify > > the following code: > >=20 > > reset_gpio =3D gpiod_get(...) > > if IS_ERR(reset_gpio): > > error =3D PTR_ERR(reset_gpio) > > if error !=3D -ENDEV: >=20 > ENODEV? Yes, typo. > > return error > > else: > > gpiod_set_direction(reset_gpiod, INACTIVE) > >=20 > > to > >=20 > > reset_gpio =3D gpiod_get_optional(....) > > if IS_ERR(reset_gpio): > > return reset_gpio > > gpiod_set_direction(reset_gpiod, INACTIVE) > >=20 > > and I never need to actually know if the reset_gpio actually exists. > > Either the line is put into its inactive state, or it doesn't exist and > > then gpiod_set_direction is a noop. For a regulator or a clk this works > > in a similar way. > >=20 > > However for an interupt this cannot work. You will always have to check > > if the irq is actually there or not because if it's not you cannot just > > ignore that. So there is no benefit of an optional irq. > >=20 > > Leaving error message reporting aside, the introduction of > > platform_get_irq_optional() allows to change > >=20 > > irq =3D platform_get_irq(...); > > if (irq < 0 && irq !=3D -ENXIO) { > > return irq; > > } else if (irq >=3D 0) { >=20 > Rather (irq > 0) actually, IRQ0 is considered invalid (but still retur= ned). This is a topic I don't feel strong for, so I'm sloppy here. If changing this is all that is needed to convince you of my point ... > > ... setup irq operation ... > > } else { /* irq =3D=3D -ENXIO */ > > ... setup polling ... > > } > >=20 > > to > > =09 > > irq =3D platform_get_irq_optional(...); > > if (irq < 0 && irq !=3D -ENXIO) { > > return irq; > > } else if (irq >=3D 0) { > > ... setup irq operation ... > > } else { /* irq =3D=3D -ENXIO */ > > ... setup polling ... > > } > >=20 > > which isn't a win. When changing the return value as you suggest, it can > > be changed instead to: > >=20 > > irq =3D platform_get_irq_optional(...); > > if (irq < 0) { > > return irq; > > } else if (irq > 0) { > > ... setup irq operation ... > > } else { /* irq =3D=3D 0 */ > > ... setup polling ... > > } > >=20 > > which is a tad nicer. If that is your goal however I ask you to also > > change the semantic of platform_get_irq() to return 0 on "not found". >=20 > Well, I'm not totally opposed to that... but would there be a conside= rable win? Well, compared to your suggestion of making platform_get_irq_optional() return 0 on "not-found" the considerable win would be that platform_get_irq_optional() and platform_get_irq() are not different just because platform_get_irq() is to hard to change. > Anyway, we should 1st stop returning 0 for "valid" IRQs -- this is done b= y my patch > the discussed patch series are atop of. >=20 > > Note the win is considerably less compared to gpiod_get_optional vs >=20 > If there's any at all... We'd basically have to touch /all/ platform_g= et_irq() > calls (and get an even larger CC list ;-)). You got me wrong here. I meant that even if you change both platform_get_irq() and platform_get_irq_optional() to return 0 on "not-found", the win is small compared to the benefit of having both clk_get() and clk_get_optional(). > > gpiod_get however. And then it still lacks the semantic of a dummy irq > > which IMHO forfeits the right to call it ..._optional(). >=20 > Not quite grasping it... Why e.g. clk_get() doesn't return 0 for a not= found clock? Because NULL is not an error value for clk and when calling clk_get() you want a failure when the clk you asked for isn't available. Sure you could do the following in a case where you want to insist the clk to be actually available: clk =3D clk_get_optional(...) if (IS_ERR_OR_NULL(clk)) { err =3D PTR_ERR(clk) || -ENODEV; return dev_err_probe(dev, err, ....); } but this is more ugly than clk =3D clk_get(...) if (IS_ERR(clk)) { err =3D PTR_ERR(clk); return dev_err_probe(dev, err, ....); } Additionally the first usage would hard-code in the drivers that NULL is the dummy value which you might want to consider a layer violation. You have to understand that for clk (and regulator and gpiod) NULL is a valid descriptor that can actually be used, it just has no effect. So this is a convenience value for the case "If the clk/regulator/gpiod in question isn't available, there is nothing to do". This is what makes clk_get_optional() and the others really useful and justifies their existence. This doesn't apply to platform_get_irq_optional(). So clk_get() is sane and sensible for cases where you need the clk to be there. It doesn't emit an error message, because the caller knows better if it's worth an error message and in some cases the caller can also emit a better error message than clk_get() itself. clk_get_optional() is sane and sensible for cases where the clk might be absent and it helps you because you don't have to differentiate between "not found" and "there is an actual resource". The reason for platform_get_irq_optional()'s existence is just that platform_get_irq() emits an error message which is wrong or suboptimal in some cases (and IMHO is platform_get_irq() root fault). It doesn't simplify handling the "not found" case. So let's not pretend by the choice of function names that there is a similarity between clk_get() + clk_get_optional() and platform_get_irq() + platform_get_irq_optional(). And as you cannot change platform_get_irq_optional() to return a working dummy value, IMHO the only sane way out is renaming it. Best regards Uwe --=20 Pengutronix e.K. | Uwe Kleine-K=F6nig | Industrial Linux Solutions | https://www.pengutronix.de/ | --uk3tmcd6neo3zuxi Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEfnIqFpAYrP8+dKQLwfwUeK3K7AkFAmHh230ACgkQwfwUeK3K 7AlWNwf/bRFJlZD6u0VGoy9sl0BdPd/mUmPL35/vP2hJR8x4u+01YZ3J5DPX9kIK pE1ufbhwUnqJgelbz3N4R/r7kog28Jsz6cHn+cw2adtMIJQfl/xpk1C2Rsk6K3bO KHClfmbHJxl7kNt7wdN0jocSaun5fy8o5qNdvZ3+tvShyZNbWFPRHeiAhVTQxKCn qLNOfy/yFDXAUKKLTeXBnSCb8PbfjnDYhbGZpNiXgwHirXAc3aUM2PL9LSJdJeyb MlXBBJWUwdu7JXIo6+5KJI3txCmuKSByoERP3yu9BirMSfvEu+0QBc8IvwL1/j4z FjtcKcDXPQ/+lDBRPl94fRT8j8cK5Q== =uAhV -----END PGP SIGNATURE----- --uk3tmcd6neo3zuxi-- --===============6257537732717672435== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek --===============6257537732717672435==-- 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 CE9CFC433EF for ; Fri, 14 Jan 2022 20:25:36 +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-Type: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: In-Reply-To:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date: Reply-To:Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date :Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=zBz0HD8WzyxMjtOlWFj4svajhqY41+tm1eD9dQOw4+E=; b=EcmQaD9s1BF30St3ylzamCplUB fuGYij2skAm5twCXpK/69BJt9aAweIbmEsBArxDNfxkY5J0nTzi++3d+jnm9Q0WGKuyqpL5FxVTNJ rQlFh7BYL+DUgUxhOd0mbKJHsoCBzosQjhHAA7yeDCE3VbGsu2D7gHYwcmIt+Too7EQ5gFjfJ8PqF rDyNar89F6TxDeRwis2VgEHuWzQ2Ck1zisqNkH6FspStxN3GcmKE5PUf+Jh/xub1J5PeHABH8ALhS GIW2ZxiL0LWdPXXezOOPmRbrwcbYzk4pEqLu79ASvKraJXA6dT9xnnv2mFMm9N1k/b2nsU934W4Fu aDcjKjQg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1n8T8i-00ABad-BD; Fri, 14 Jan 2022 20:25:36 +0000 Received: from metis.ext.pengutronix.de ([2001:67c:670:201:290:27ff:fe1d:cc33]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1n8T8V-00ABZ2-Es for linux-phy@lists.infradead.org; Fri, 14 Jan 2022 20:25:25 +0000 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 1n8T5w-0000w6-8j; Fri, 14 Jan 2022 21:22:44 +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 1n8T5j-00AJbK-DJ; Fri, 14 Jan 2022 21:22:30 +0100 Received: from ukl by ptz.office.stw.pengutronix.de with local (Exim 4.92) (envelope-from ) id 1n8T5h-0002Jl-M3; Fri, 14 Jan 2022 21:22:29 +0100 Date: Fri, 14 Jan 2022 21:22:26 +0100 From: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= To: Sergey Shtylyov 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 , Jaroslav Kysela , Guenter Roeck , Thierry Reding , MTD Maling List , Linux I2C , Miquel Raynal , linux-phy@lists.infradead.org, Jiri Slaby , openipmi-developer@lists.sourceforge.net, "David S. Miller" , Khuong Dinh , Florian Fainelli , Matthias Schiffer , Joakim Zhang , Kamal Dasu , Lee Jones , Bartosz Golaszewski , Daniel Lezcano , Kishon Vijay Abraham I , Geert Uytterhoeven , "open list:SERIAL DRIVERS" , Yoshihiro Shimoda , bcm-kernel-feedback-list , Zhang Rui , Linux PWM List , Robert Richter , Saravanan Sekar , Corey Minyard , Linux PM list , Liam Girdwood , Mauro Carvalho Chehab , John Garry , Peter Korsgaard , William Breathitt Gray , Mark Gross , Hans de Goede , Alex Williamson , Mark Brown , Borislav Petkov , Sebastian Reichel , Jakub Kicinski , 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 , "open list:GPIO SUBSYSTEM" , Cornelia Huck , Linux MMC List , Linux Kernel Mailing List , linux-spi , Linux-Renesas , 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 , netdev@vger.kernel.org Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional Message-ID: <20220114202226.ugzklxv4wzr6egwj@pengutronix.de> References: <20220110201014.mtajyrfcfznfhyqm@pengutronix.de> <20220112085009.dbasceh3obfok5dc@pengutronix.de> <20220112213121.5ruae5mxwj6t3qiy@pengutronix.de> <29f0c65d-77f2-e5b2-f6cc-422add8a707d@omp.ru> <20220114092557.jrkfx7ihg26ekzci@pengutronix.de> <61b80939-357d-14f5-df99-b8d102a4e1a1@omp.ru> MIME-Version: 1.0 In-Reply-To: <61b80939-357d-14f5-df99-b8d102a4e1a1@omp.ru> 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-phy@lists.infradead.org X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220114_122523_678282_EE621EC9 X-CRM114-Status: GOOD ( 59.83 ) X-BeenThere: linux-phy@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Linux Phy Mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: multipart/mixed; boundary="===============4826413683778702353==" Sender: "linux-phy" Errors-To: linux-phy-bounces+linux-phy=archiver.kernel.org@lists.infradead.org --===============4826413683778702353== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="uk3tmcd6neo3zuxi" Content-Disposition: inline --uk3tmcd6neo3zuxi Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jan 14, 2022 at 10:14:10PM +0300, Sergey Shtylyov wrote: > On 1/14/22 12:25 PM, Uwe Kleine-K=F6nig wrote: >=20 > >>>>> 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 -ENOS= YS > >>>>> (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. > >> > >> Hm, I've just looked at these and must note that they match 1:1 with > >> platform_get_irq_optional(). Unfortunately, we can't however behave the > >> same way in request_irq() -- because it has to support IRQ0 for the sa= ke > >> of i8253 drivers in arch/... > >=20 > > Let me reformulate your statement to the IMHO equivalent: > >=20 > > If you set aside the differences between > > platform_get_irq_optional() and gpiod_get_optional(), >=20 > Sorry, I should make it clear this is actually the diff between a woul= d-be > platform_get_irq_optional() after my patch, not the current code... The similarity is that with your patch both gpiod_get_optional() and platform_get_irq_optional() return NULL and 0 on not-found. The relevant difference however is that for a gpiod NULL is a dummy value, while for irqs it's not. So the similarity is only syntactically, but not semantically. > > platform_get_irq_optional() is like gpiod_get_optional(). > >=20 > > The introduction of gpiod_get_optional() made it possible to simplify > > the following code: > >=20 > > reset_gpio =3D gpiod_get(...) > > if IS_ERR(reset_gpio): > > error =3D PTR_ERR(reset_gpio) > > if error !=3D -ENDEV: >=20 > ENODEV? Yes, typo. > > return error > > else: > > gpiod_set_direction(reset_gpiod, INACTIVE) > >=20 > > to > >=20 > > reset_gpio =3D gpiod_get_optional(....) > > if IS_ERR(reset_gpio): > > return reset_gpio > > gpiod_set_direction(reset_gpiod, INACTIVE) > >=20 > > and I never need to actually know if the reset_gpio actually exists. > > Either the line is put into its inactive state, or it doesn't exist and > > then gpiod_set_direction is a noop. For a regulator or a clk this works > > in a similar way. > >=20 > > However for an interupt this cannot work. You will always have to check > > if the irq is actually there or not because if it's not you cannot just > > ignore that. So there is no benefit of an optional irq. > >=20 > > Leaving error message reporting aside, the introduction of > > platform_get_irq_optional() allows to change > >=20 > > irq =3D platform_get_irq(...); > > if (irq < 0 && irq !=3D -ENXIO) { > > return irq; > > } else if (irq >=3D 0) { >=20 > Rather (irq > 0) actually, IRQ0 is considered invalid (but still retur= ned). This is a topic I don't feel strong for, so I'm sloppy here. If changing this is all that is needed to convince you of my point ... > > ... setup irq operation ... > > } else { /* irq =3D=3D -ENXIO */ > > ... setup polling ... > > } > >=20 > > to > > =09 > > irq =3D platform_get_irq_optional(...); > > if (irq < 0 && irq !=3D -ENXIO) { > > return irq; > > } else if (irq >=3D 0) { > > ... setup irq operation ... > > } else { /* irq =3D=3D -ENXIO */ > > ... setup polling ... > > } > >=20 > > which isn't a win. When changing the return value as you suggest, it can > > be changed instead to: > >=20 > > irq =3D platform_get_irq_optional(...); > > if (irq < 0) { > > return irq; > > } else if (irq > 0) { > > ... setup irq operation ... > > } else { /* irq =3D=3D 0 */ > > ... setup polling ... > > } > >=20 > > which is a tad nicer. If that is your goal however I ask you to also > > change the semantic of platform_get_irq() to return 0 on "not found". >=20 > Well, I'm not totally opposed to that... but would there be a conside= rable win? Well, compared to your suggestion of making platform_get_irq_optional() return 0 on "not-found" the considerable win would be that platform_get_irq_optional() and platform_get_irq() are not different just because platform_get_irq() is to hard to change. > Anyway, we should 1st stop returning 0 for "valid" IRQs -- this is done b= y my patch > the discussed patch series are atop of. >=20 > > Note the win is considerably less compared to gpiod_get_optional vs >=20 > If there's any at all... We'd basically have to touch /all/ platform_g= et_irq() > calls (and get an even larger CC list ;-)). You got me wrong here. I meant that even if you change both platform_get_irq() and platform_get_irq_optional() to return 0 on "not-found", the win is small compared to the benefit of having both clk_get() and clk_get_optional(). > > gpiod_get however. And then it still lacks the semantic of a dummy irq > > which IMHO forfeits the right to call it ..._optional(). >=20 > Not quite grasping it... Why e.g. clk_get() doesn't return 0 for a not= found clock? Because NULL is not an error value for clk and when calling clk_get() you want a failure when the clk you asked for isn't available. Sure you could do the following in a case where you want to insist the clk to be actually available: clk =3D clk_get_optional(...) if (IS_ERR_OR_NULL(clk)) { err =3D PTR_ERR(clk) || -ENODEV; return dev_err_probe(dev, err, ....); } but this is more ugly than clk =3D clk_get(...) if (IS_ERR(clk)) { err =3D PTR_ERR(clk); return dev_err_probe(dev, err, ....); } Additionally the first usage would hard-code in the drivers that NULL is the dummy value which you might want to consider a layer violation. You have to understand that for clk (and regulator and gpiod) NULL is a valid descriptor that can actually be used, it just has no effect. So this is a convenience value for the case "If the clk/regulator/gpiod in question isn't available, there is nothing to do". This is what makes clk_get_optional() and the others really useful and justifies their existence. This doesn't apply to platform_get_irq_optional(). So clk_get() is sane and sensible for cases where you need the clk to be there. It doesn't emit an error message, because the caller knows better if it's worth an error message and in some cases the caller can also emit a better error message than clk_get() itself. clk_get_optional() is sane and sensible for cases where the clk might be absent and it helps you because you don't have to differentiate between "not found" and "there is an actual resource". The reason for platform_get_irq_optional()'s existence is just that platform_get_irq() emits an error message which is wrong or suboptimal in some cases (and IMHO is platform_get_irq() root fault). It doesn't simplify handling the "not found" case. So let's not pretend by the choice of function names that there is a similarity between clk_get() + clk_get_optional() and platform_get_irq() + platform_get_irq_optional(). And as you cannot change platform_get_irq_optional() to return a working dummy value, IMHO the only sane way out is renaming it. Best regards Uwe --=20 Pengutronix e.K. | Uwe Kleine-K=F6nig | Industrial Linux Solutions | https://www.pengutronix.de/ | --uk3tmcd6neo3zuxi Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEfnIqFpAYrP8+dKQLwfwUeK3K7AkFAmHh230ACgkQwfwUeK3K 7AlWNwf/bRFJlZD6u0VGoy9sl0BdPd/mUmPL35/vP2hJR8x4u+01YZ3J5DPX9kIK pE1ufbhwUnqJgelbz3N4R/r7kog28Jsz6cHn+cw2adtMIJQfl/xpk1C2Rsk6K3bO KHClfmbHJxl7kNt7wdN0jocSaun5fy8o5qNdvZ3+tvShyZNbWFPRHeiAhVTQxKCn qLNOfy/yFDXAUKKLTeXBnSCb8PbfjnDYhbGZpNiXgwHirXAc3aUM2PL9LSJdJeyb MlXBBJWUwdu7JXIo6+5KJI3txCmuKSByoERP3yu9BirMSfvEu+0QBc8IvwL1/j4z FjtcKcDXPQ/+lDBRPl94fRT8j8cK5Q== =uAhV -----END PGP SIGNATURE----- --uk3tmcd6neo3zuxi-- --===============4826413683778702353== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy --===============4826413683778702353==-- 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 alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (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 DFCE5C433EF for ; Sat, 15 Jan 2022 08:08:59 +0000 (UTC) Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id 329E320D6; Sat, 15 Jan 2022 09:08:08 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 329E320D6 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1642234138; bh=3Hoe+XfLFu4/O6fbH34J55CxHiXNnp4Q2cElQqk2LDE=; h=Date:From:To:Subject:References:In-Reply-To:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=Tlppgmveh4YfwrteRmF9waMXOr4L1x88vxLENaEu0BFrva28sqhPU4Mlp5/WGlNE2 P8i0Jfm5ezdJ/ytza2Dh8LmbSPAQ2p58A+tBaKwDanWQPw5Qq0ek2aekganv2F7Zqc I66/nC4vBx0lSKk2W9S5ysdl3Xjtfy9Mj8AcIZpM= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 03661F80571; Sat, 15 Jan 2022 09:03:02 +0100 (CET) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 5FF86F8030F; Fri, 14 Jan 2022 21:22:55 +0100 (CET) Received: from metis.ext.pengutronix.de (metis.ext.pengutronix.de [IPv6:2001:67c:670:201:290:27ff:fe1d:cc33]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 43795F800CE for ; Fri, 14 Jan 2022 21:22:50 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz 43795F800CE 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 1n8T5w-0000w6-8j; Fri, 14 Jan 2022 21:22:44 +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 1n8T5j-00AJbK-DJ; Fri, 14 Jan 2022 21:22:30 +0100 Received: from ukl by ptz.office.stw.pengutronix.de with local (Exim 4.92) (envelope-from ) id 1n8T5h-0002Jl-M3; Fri, 14 Jan 2022 21:22:29 +0100 Date: Fri, 14 Jan 2022 21:22:26 +0100 From: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= To: Sergey Shtylyov Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional Message-ID: <20220114202226.ugzklxv4wzr6egwj@pengutronix.de> References: <20220110201014.mtajyrfcfznfhyqm@pengutronix.de> <20220112085009.dbasceh3obfok5dc@pengutronix.de> <20220112213121.5ruae5mxwj6t3qiy@pengutronix.de> <29f0c65d-77f2-e5b2-f6cc-422add8a707d@omp.ru> <20220114092557.jrkfx7ihg26ekzci@pengutronix.de> <61b80939-357d-14f5-df99-b8d102a4e1a1@omp.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="uk3tmcd6neo3zuxi" Content-Disposition: inline In-Reply-To: <61b80939-357d-14f5-df99-b8d102a4e1a1@omp.ru> 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: alsa-devel@alsa-project.org X-Mailman-Approved-At: Sat, 15 Jan 2022 09:02:47 +0100 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 , Guenter Roeck , Thierry Reding , MTD Maling List , Linux I2C , "open list:GPIO SUBSYSTEM" , Miquel Raynal , linux-phy@lists.infradead.org, netdev@vger.kernel.org, linux-spi , Jiri Slaby , Khuong Dinh , Florian Fainelli , Matthias Schiffer , Kamal Dasu , Lee Jones , Bartosz Golaszewski , Daniel Lezcano , Kishon Vijay Abraham I , Geert Uytterhoeven , "open list:SERIAL DRIVERS" , bcm-kernel-feedback-list , Zhang Rui , platform-driver-x86@vger.kernel.org, Linux PWM List , Robert Richter , Saravanan Sekar , Corey Minyard , Linux PM list , Liam Girdwood , Mauro Carvalho Chehab , John Garry , Takashi Iwai , Peter Korsgaard , William Breathitt Gray , Mark Gross , Hans de Goede , Alex Williamson , Mark Brown , Borislav Petkov , Jakub Kicinski , Matthias Brugger , openipmi-developer@lists.sourceforge.net, Andy Shevchenko , Benson Leung , Pengutronix Kernel Team , Linux ARM , linux-edac@vger.kernel.org, Tony Luck , Richard Weinberger , Mun Yew Tham , Eric Auger , Greg Kroah-Hartman , Yoshihiro Shimoda , Cornelia Huck , Linux MMC List , Joakim Zhang , Linux Kernel Mailing List , Linux-Renesas , Vinod Koul , James Morse , Zha Qipeng , Sebastian Reichel , Niklas =?utf-8?Q?S=C3=B6derlund?= , linux-mediatek@lists.infradead.org, Brian Norris , "David S. Miller" X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" --uk3tmcd6neo3zuxi Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jan 14, 2022 at 10:14:10PM +0300, Sergey Shtylyov wrote: > On 1/14/22 12:25 PM, Uwe Kleine-K=F6nig wrote: >=20 > >>>>> 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 -ENOS= YS > >>>>> (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. > >> > >> Hm, I've just looked at these and must note that they match 1:1 with > >> platform_get_irq_optional(). Unfortunately, we can't however behave the > >> same way in request_irq() -- because it has to support IRQ0 for the sa= ke > >> of i8253 drivers in arch/... > >=20 > > Let me reformulate your statement to the IMHO equivalent: > >=20 > > If you set aside the differences between > > platform_get_irq_optional() and gpiod_get_optional(), >=20 > Sorry, I should make it clear this is actually the diff between a woul= d-be > platform_get_irq_optional() after my patch, not the current code... The similarity is that with your patch both gpiod_get_optional() and platform_get_irq_optional() return NULL and 0 on not-found. The relevant difference however is that for a gpiod NULL is a dummy value, while for irqs it's not. So the similarity is only syntactically, but not semantically. > > platform_get_irq_optional() is like gpiod_get_optional(). > >=20 > > The introduction of gpiod_get_optional() made it possible to simplify > > the following code: > >=20 > > reset_gpio =3D gpiod_get(...) > > if IS_ERR(reset_gpio): > > error =3D PTR_ERR(reset_gpio) > > if error !=3D -ENDEV: >=20 > ENODEV? Yes, typo. > > return error > > else: > > gpiod_set_direction(reset_gpiod, INACTIVE) > >=20 > > to > >=20 > > reset_gpio =3D gpiod_get_optional(....) > > if IS_ERR(reset_gpio): > > return reset_gpio > > gpiod_set_direction(reset_gpiod, INACTIVE) > >=20 > > and I never need to actually know if the reset_gpio actually exists. > > Either the line is put into its inactive state, or it doesn't exist and > > then gpiod_set_direction is a noop. For a regulator or a clk this works > > in a similar way. > >=20 > > However for an interupt this cannot work. You will always have to check > > if the irq is actually there or not because if it's not you cannot just > > ignore that. So there is no benefit of an optional irq. > >=20 > > Leaving error message reporting aside, the introduction of > > platform_get_irq_optional() allows to change > >=20 > > irq =3D platform_get_irq(...); > > if (irq < 0 && irq !=3D -ENXIO) { > > return irq; > > } else if (irq >=3D 0) { >=20 > Rather (irq > 0) actually, IRQ0 is considered invalid (but still retur= ned). This is a topic I don't feel strong for, so I'm sloppy here. If changing this is all that is needed to convince you of my point ... > > ... setup irq operation ... > > } else { /* irq =3D=3D -ENXIO */ > > ... setup polling ... > > } > >=20 > > to > > =09 > > irq =3D platform_get_irq_optional(...); > > if (irq < 0 && irq !=3D -ENXIO) { > > return irq; > > } else if (irq >=3D 0) { > > ... setup irq operation ... > > } else { /* irq =3D=3D -ENXIO */ > > ... setup polling ... > > } > >=20 > > which isn't a win. When changing the return value as you suggest, it can > > be changed instead to: > >=20 > > irq =3D platform_get_irq_optional(...); > > if (irq < 0) { > > return irq; > > } else if (irq > 0) { > > ... setup irq operation ... > > } else { /* irq =3D=3D 0 */ > > ... setup polling ... > > } > >=20 > > which is a tad nicer. If that is your goal however I ask you to also > > change the semantic of platform_get_irq() to return 0 on "not found". >=20 > Well, I'm not totally opposed to that... but would there be a conside= rable win? Well, compared to your suggestion of making platform_get_irq_optional() return 0 on "not-found" the considerable win would be that platform_get_irq_optional() and platform_get_irq() are not different just because platform_get_irq() is to hard to change. > Anyway, we should 1st stop returning 0 for "valid" IRQs -- this is done b= y my patch > the discussed patch series are atop of. >=20 > > Note the win is considerably less compared to gpiod_get_optional vs >=20 > If there's any at all... We'd basically have to touch /all/ platform_g= et_irq() > calls (and get an even larger CC list ;-)). You got me wrong here. I meant that even if you change both platform_get_irq() and platform_get_irq_optional() to return 0 on "not-found", the win is small compared to the benefit of having both clk_get() and clk_get_optional(). > > gpiod_get however. And then it still lacks the semantic of a dummy irq > > which IMHO forfeits the right to call it ..._optional(). >=20 > Not quite grasping it... Why e.g. clk_get() doesn't return 0 for a not= found clock? Because NULL is not an error value for clk and when calling clk_get() you want a failure when the clk you asked for isn't available. Sure you could do the following in a case where you want to insist the clk to be actually available: clk =3D clk_get_optional(...) if (IS_ERR_OR_NULL(clk)) { err =3D PTR_ERR(clk) || -ENODEV; return dev_err_probe(dev, err, ....); } but this is more ugly than clk =3D clk_get(...) if (IS_ERR(clk)) { err =3D PTR_ERR(clk); return dev_err_probe(dev, err, ....); } Additionally the first usage would hard-code in the drivers that NULL is the dummy value which you might want to consider a layer violation. You have to understand that for clk (and regulator and gpiod) NULL is a valid descriptor that can actually be used, it just has no effect. So this is a convenience value for the case "If the clk/regulator/gpiod in question isn't available, there is nothing to do". This is what makes clk_get_optional() and the others really useful and justifies their existence. This doesn't apply to platform_get_irq_optional(). So clk_get() is sane and sensible for cases where you need the clk to be there. It doesn't emit an error message, because the caller knows better if it's worth an error message and in some cases the caller can also emit a better error message than clk_get() itself. clk_get_optional() is sane and sensible for cases where the clk might be absent and it helps you because you don't have to differentiate between "not found" and "there is an actual resource". The reason for platform_get_irq_optional()'s existence is just that platform_get_irq() emits an error message which is wrong or suboptimal in some cases (and IMHO is platform_get_irq() root fault). It doesn't simplify handling the "not found" case. So let's not pretend by the choice of function names that there is a similarity between clk_get() + clk_get_optional() and platform_get_irq() + platform_get_irq_optional(). And as you cannot change platform_get_irq_optional() to return a working dummy value, IMHO the only sane way out is renaming it. Best regards Uwe --=20 Pengutronix e.K. | Uwe Kleine-K=F6nig | Industrial Linux Solutions | https://www.pengutronix.de/ | --uk3tmcd6neo3zuxi Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEfnIqFpAYrP8+dKQLwfwUeK3K7AkFAmHh230ACgkQwfwUeK3K 7AlWNwf/bRFJlZD6u0VGoy9sl0BdPd/mUmPL35/vP2hJR8x4u+01YZ3J5DPX9kIK pE1ufbhwUnqJgelbz3N4R/r7kog28Jsz6cHn+cw2adtMIJQfl/xpk1C2Rsk6K3bO KHClfmbHJxl7kNt7wdN0jocSaun5fy8o5qNdvZ3+tvShyZNbWFPRHeiAhVTQxKCn qLNOfy/yFDXAUKKLTeXBnSCb8PbfjnDYhbGZpNiXgwHirXAc3aUM2PL9LSJdJeyb MlXBBJWUwdu7JXIo6+5KJI3txCmuKSByoERP3yu9BirMSfvEu+0QBc8IvwL1/j4z FjtcKcDXPQ/+lDBRPl94fRT8j8cK5Q== =uAhV -----END PGP SIGNATURE----- --uk3tmcd6neo3zuxi--