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 6D288C433EF for ; Mon, 17 Jan 2022 11:50:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239248AbiAQLug (ORCPT ); Mon, 17 Jan 2022 06:50:36 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58250 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S239242AbiAQLue (ORCPT ); Mon, 17 Jan 2022 06:50:34 -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 93640C061574 for ; Mon, 17 Jan 2022 03:50:33 -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 1n9QVx-0000fx-Mn; Mon, 17 Jan 2022 12:49:33 +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 1n9QVo-00AoYK-L6; Mon, 17 Jan 2022 12:49:23 +0100 Received: from ukl by ptz.office.stw.pengutronix.de with local (Exim 4.92) (envelope-from ) id 1n9QVn-0001x3-Fg; Mon, 17 Jan 2022 12:49:23 +0100 Date: Mon, 17 Jan 2022 12:49:23 +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 , 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, Khuong Dinh , Florian Fainelli , Matthias Schiffer , Joakim Zhang , Kamal Dasu , Greg Kroah-Hartman , Lee Jones , Bartosz Golaszewski , Daniel Lezcano , Tony Luck , Kishon Vijay Abraham I , bcm-kernel-feedback-list , "open list:SERIAL DRIVERS" , Jakub Kicinski , Zhang Rui , Linux PWM List , Robert Richter , Saravanan Sekar , Corey Minyard , Linux PM list , Linux Kernel Mailing 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 , Eric Auger , Matthias Brugger , Takashi Iwai , platform-driver-x86@vger.kernel.org, Benson Leung , Linux ARM , linux-edac@vger.kernel.org, Sergey Shtylyov , Mun Yew Tham , Hans de Goede , netdev , Yoshihiro Shimoda , Cornelia Huck , Linux MMC List , Liam Girdwood , 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 , "David S. Miller" Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional Message-ID: <20220117114923.d5vajgitxneec7j7@pengutronix.de> References: <20220112213121.5ruae5mxwj6t3qiy@pengutronix.de> <29f0c65d-77f2-e5b2-f6cc-422add8a707d@omp.ru> <20220114092557.jrkfx7ihg26ekzci@pengutronix.de> <61b80939-357d-14f5-df99-b8d102a4e1a1@omp.ru> <20220114202226.ugzklxv4wzr6egwj@pengutronix.de> <20220117092444.opoedfcf5k5u6otq@pengutronix.de> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="p6yh245p57zhiyck" 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-gpio@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-gpio@vger.kernel.org --p6yh245p57zhiyck Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jan 17, 2022 at 11:35:52AM +0100, Geert Uytterhoeven wrote: > Hi Uwe, >=20 > On Mon, Jan 17, 2022 at 10:24 AM Uwe Kleine-K=F6nig > wrote: > > On Mon, Jan 17, 2022 at 09:41:42AM +0100, Geert Uytterhoeven wrote: > > > On Sat, Jan 15, 2022 at 9:22 PM Sergey Shtylyov w= rote: > > > > On 1/14/22 11:22 PM, Uwe Kleine-K=F6nig wrote: > > > > > You have to understand that for clk (and regulator and gpiod) NUL= L is a > > > > > valid descriptor that can actually be used, it just has no effect= =2E So > > > > > this is a convenience value for the case "If the clk/regulator/gp= iod in > > > > > question isn't available, there is nothing to do". This is what m= akes > > > > > clk_get_optional() and the others really useful and justifies the= ir > > > > > existence. This doesn't apply to platform_get_irq_optional(). > > > > > > > > I do understand that. However, IRQs are a different beast with t= heir > > > > own justifications... > > > > > > > > clk_get_optional() is sane and sensible for cases where the clk m= ight be > > > > > absent and it helps you because you don't have to differentiate b= etween > > > > > "not found" and "there is an actual resource". > > > > > > > > > > The reason for platform_get_irq_optional()'s existence is just th= at > > > > > platform_get_irq() emits an error message which is wrong or subop= timal > > > > > > > > I think you are very wrong here. The real reason is to simplify = the > > > > callers. > > > > > > Indeed. > > > > The commit that introduced platform_get_irq_optional() said: > > > > Introduce a new platform_get_irq_optional() that works much like > > platform_get_irq() but does not output an error on failure to > > find the interrupt. > > > > So the author of 8973ea47901c81a1912bd05f1577bed9b5b52506 failed to > > mention the real reason? Or look at > > 31a8d8fa84c51d3ab00bf059158d5de6178cf890: > > > > [...] use platform_get_irq_optional() to get second/third IRQ > > which are optional to avoid below error message during probe: > > [...] > > > > Look through the output of > > > > git log -Splatform_get_irq_optional > > > > to find several more of these. >=20 > Commit 8973ea47901c81a1 ("driver core: platform: Introduce > platform_get_irq_optional()") and the various fixups fixed the ugly > printing of error messages that were not applicable. > In hindsight, probably commit 7723f4c5ecdb8d83 ("driver core: > platform: Add an error message to platform_get_irq*()") should have > been reverted instead, until a platform_get_irq_optional() with proper > semantics was introduced. ack. > But as we were all in a hurry to kill the non-applicable error > message, we went for the quick and dirty fix. >=20 > > Also I fail to see how a caller of (today's) platform_get_irq_optional() > > is simpler than a caller of platform_get_irq() given that there is no > > semantic difference between the two. Please show me a single > > conversion from platform_get_irq to platform_get_irq_optional that > > yielded a simplification. >=20 > That's exactly why we want to change the latter to return 0 ;-) OK. So you agree to my statement "The reason for platform_get_irq_optional()'s existence is just that platform_get_irq() emits an error message [...]". Actually you don't want to oppose but say: It's unfortunate that the silent variant of platform_get_irq() took the obvious name of a function that could have an improved return code semantic. So my suggestion to rename todays platform_get_irq_optional() to platform_get_irq_silently() and then introducing platform_get_irq_optional() with your suggested semantic seems intriguing and straigt forward to me. Another thought: platform_get_irq emits an error message for all problems. Wouldn't it be consistent to let platform_get_irq_optional() emit an error message for all problems but "not found"? Alternatively remove the error printk from platform_get_irq(). > > So you need some more effort to convince me of your POV. > > > > > Even for clocks, you cannot assume that you can always blindly use > > > the returned dummy (actually a NULL pointer) to call into the clk > > > API. While this works fine for simple use cases, where you just > > > want to enable/disable an optional clock (clk_prepare_enable() and > > > clk_disable_unprepare()), it does not work for more complex use cases. > > > > Agreed. But for clks and gpiods and regulators the simple case is quite > > usual. For irqs it isn't. >=20 > It is for devices that can have either separate interrupts, or a single > multiplexed interrupt. >=20 > The logic in e.g. drivers/tty/serial/sh-sci.c and > drivers/spi/spi-rspi.c could be simplified and improved (currently > it doesn't handle deferred probe) if platform_get_irq_optional() > would return 0 instead of -ENXIO. Looking at sh-sci.c the irq handling logic could be improved even without a changed platform_get_irq_optional(): diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index 968967d722d4..c7dc9fb84844 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -2873,11 +2873,13 @@ static int sci_init_single(struct platform_device *= dev, * interrupt ID numbers, or muxed together with another interrupt. */ if (sci_port->irqs[0] < 0) - return -ENXIO; + return sci_port->irqs[0]; =20 - if (sci_port->irqs[1] < 0) + if (sci_port->irqs[1] =3D=3D -ENXIO) for (i =3D 1; i < ARRAY_SIZE(sci_port->irqs); i++) sci_port->irqs[i] =3D sci_port->irqs[0]; + else if (sci_port->irqs[1] < 0) + return sci_port->irqs[1]; =20 sci_port->params =3D sci_probe_regmap(p); if (unlikely(sci_port->params =3D=3D NULL)) And then the code flow is actively irritating. sci_init_single() copies irqs[0] to all other irqs[i] and then sci_request_irq() loops over the already requested irqs and checks for duplicates. A single place that identifies the exact set of required irqs would already help a lot. Also for spi-rspi.c I don't see how platform_get_irq_byname_optional() returning 0 instead of -ENXIO would help. Please talk in patches. Preferably first simplify in-driver logic to make the conversion to the new platform_get_irq_optional() actually reviewable. > > And if you cannot blindly use the dummy, then you're not the targetted > > caller of *_get_optional() and should better use *_get() and handle > > -ENODEV explicitly. >=20 > No, because the janitors tend to consolidate error message handling, > by moving the printing up, inside the *_get() methods. That's exactly > what happened here. This is in my eyes the root cause of the issues at hand. Moving the error message handling into a get function is only right for most of the callers. So the more conservative approach would be to introduce a noisy variant of the get function and convert all users that benefit separately while the unreviewed callers and those that don't want an error message can happily continue to use the silent variant. > So there are three reasons: because the absence of an optional IRQ > is not an error, and thus that should not cause (a) an error code > to be returned, and (b) an error message to be printed, and (c) > because it can simplify the logic in device drivers. I don't agree to (a). If the value signaling not-found is -ENXIO or 0 (or -ENODEV) doesn't matter much. I wouldn't deviate from the return code semantics of platform_get_irq() just for having to check against 0 instead of -ENXIO. Zero is then just another magic value. (c) still has to be proven, see above. > Commit 8973ea47901c81a1 ("driver core: platform: Introduce > platform_get_irq_optional()") fixed (b), but didn't address (a) and > (c). Yes, it fixed (b) and picked a bad name for that. Best regards Uwe --=20 Pengutronix e.K. | Uwe Kleine-K=F6nig | Industrial Linux Solutions | https://www.pengutronix.de/ | --p6yh245p57zhiyck Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEfnIqFpAYrP8+dKQLwfwUeK3K7AkFAmHlV8AACgkQwfwUeK3K 7Am0GQf8CoKYtZsyB2Veq4tA4dVxwehDrqSNzD0/oee9gQ2W8Ug3o/BHJYBwahzq EvMyo3JUywFfBFS6fqP6q+5CXaw3qhcVdLIQIYR1NbdbDku9fPpYgUlMeO8FLj0S AjA1gReJzZffpqQa+j6sWHbwoCmV4ZWTYuhi2tnY6gxes4QcBTcXhrlPtPvEcvRj xiaHDNvm4yBJjau7t98dhCCfb9ioYwkuGybaTVJenP6u4ZB5QxTAKBsVZsaYscE9 K/bTKX+pt+MFJrjy6AN6Qq4JYNuQK8v7MawD5u/q9qZHAELmMQaNyWTpBBDKqjGv Z8p6bAtXmJy2dTalO786GdRxwAWrMQ== =gT+3 -----END PGP SIGNATURE----- --p6yh245p57zhiyck-- 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 4546AC433EF for ; Mon, 17 Jan 2022 11:51:33 +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=F8zTE9I6/jj4yH8xwl8HUcjor9nNnixSNT1nhbqjtDI=; b=g3D2fL7MtHktc3q9uBpLNkEEFl Ls2POoQVcbVDEEe1Je3Cu0RDoEap1LKq46cvxS4fczMdhAXNJFInkjRB4S26axtSbAAYKO7R12z4j 9xoDRes6u15wVEq+/fK0LZ7Aaqd2FNS5jS/4BovCH9DBjAVdxEgdSurV0wy3n7IvSxmjwksQTV7MY JrRfBCSCk6hZrZ06abIz87r7ju/HmcxIHyUdwbyRB61x58wrQNJIHBb/T7TwRbnOefbRJpBJeoVwx PTG6EbfolSi1uCR1nQQD0qc8/TN53iQKtbHP9WnSOVy6a5XoUzXmGNaHsKkXIUzLs5hSW2/OZWzo3 4hRvc/Pw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1n9QXl-00EiEC-9W; Mon, 17 Jan 2022 11:51:25 +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 1n9QXU-00Ei9V-Se for linux-mediatek@lists.infradead.org; Mon, 17 Jan 2022 11:51:11 +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 1n9QVx-0000fx-Mn; Mon, 17 Jan 2022 12:49:33 +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 1n9QVo-00AoYK-L6; Mon, 17 Jan 2022 12:49:23 +0100 Received: from ukl by ptz.office.stw.pengutronix.de with local (Exim 4.92) (envelope-from ) id 1n9QVn-0001x3-Fg; Mon, 17 Jan 2022 12:49:23 +0100 Date: Mon, 17 Jan 2022 12:49:23 +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 , 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, Khuong Dinh , Florian Fainelli , Matthias Schiffer , Joakim Zhang , Kamal Dasu , Greg Kroah-Hartman , Lee Jones , Bartosz Golaszewski , Daniel Lezcano , Tony Luck , Kishon Vijay Abraham I , bcm-kernel-feedback-list , "open list:SERIAL DRIVERS" , Jakub Kicinski , Zhang Rui , Linux PWM List , Robert Richter , Saravanan Sekar , Corey Minyard , Linux PM list , Linux Kernel Mailing 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 , Eric Auger , Matthias Brugger , Takashi Iwai , platform-driver-x86@vger.kernel.org, Benson Leung , Linux ARM , linux-edac@vger.kernel.org, Sergey Shtylyov , Mun Yew Tham , Hans de Goede , netdev , Yoshihiro Shimoda , Cornelia Huck , Linux MMC List , Liam Girdwood , 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 , "David S. Miller" Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional Message-ID: <20220117114923.d5vajgitxneec7j7@pengutronix.de> References: <20220112213121.5ruae5mxwj6t3qiy@pengutronix.de> <29f0c65d-77f2-e5b2-f6cc-422add8a707d@omp.ru> <20220114092557.jrkfx7ihg26ekzci@pengutronix.de> <61b80939-357d-14f5-df99-b8d102a4e1a1@omp.ru> <20220114202226.ugzklxv4wzr6egwj@pengutronix.de> <20220117092444.opoedfcf5k5u6otq@pengutronix.de> MIME-Version: 1.0 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-mediatek@lists.infradead.org X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220117_035109_140642_7CD2FE83 X-CRM114-Status: GOOD ( 63.91 ) 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="===============8089239644367236678==" Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org --===============8089239644367236678== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="p6yh245p57zhiyck" Content-Disposition: inline --p6yh245p57zhiyck Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jan 17, 2022 at 11:35:52AM +0100, Geert Uytterhoeven wrote: > Hi Uwe, >=20 > On Mon, Jan 17, 2022 at 10:24 AM Uwe Kleine-K=F6nig > wrote: > > On Mon, Jan 17, 2022 at 09:41:42AM +0100, Geert Uytterhoeven wrote: > > > On Sat, Jan 15, 2022 at 9:22 PM Sergey Shtylyov w= rote: > > > > On 1/14/22 11:22 PM, Uwe Kleine-K=F6nig wrote: > > > > > You have to understand that for clk (and regulator and gpiod) NUL= L is a > > > > > valid descriptor that can actually be used, it just has no effect= =2E So > > > > > this is a convenience value for the case "If the clk/regulator/gp= iod in > > > > > question isn't available, there is nothing to do". This is what m= akes > > > > > clk_get_optional() and the others really useful and justifies the= ir > > > > > existence. This doesn't apply to platform_get_irq_optional(). > > > > > > > > I do understand that. However, IRQs are a different beast with t= heir > > > > own justifications... > > > > > > > > clk_get_optional() is sane and sensible for cases where the clk m= ight be > > > > > absent and it helps you because you don't have to differentiate b= etween > > > > > "not found" and "there is an actual resource". > > > > > > > > > > The reason for platform_get_irq_optional()'s existence is just th= at > > > > > platform_get_irq() emits an error message which is wrong or subop= timal > > > > > > > > I think you are very wrong here. The real reason is to simplify = the > > > > callers. > > > > > > Indeed. > > > > The commit that introduced platform_get_irq_optional() said: > > > > Introduce a new platform_get_irq_optional() that works much like > > platform_get_irq() but does not output an error on failure to > > find the interrupt. > > > > So the author of 8973ea47901c81a1912bd05f1577bed9b5b52506 failed to > > mention the real reason? Or look at > > 31a8d8fa84c51d3ab00bf059158d5de6178cf890: > > > > [...] use platform_get_irq_optional() to get second/third IRQ > > which are optional to avoid below error message during probe: > > [...] > > > > Look through the output of > > > > git log -Splatform_get_irq_optional > > > > to find several more of these. >=20 > Commit 8973ea47901c81a1 ("driver core: platform: Introduce > platform_get_irq_optional()") and the various fixups fixed the ugly > printing of error messages that were not applicable. > In hindsight, probably commit 7723f4c5ecdb8d83 ("driver core: > platform: Add an error message to platform_get_irq*()") should have > been reverted instead, until a platform_get_irq_optional() with proper > semantics was introduced. ack. > But as we were all in a hurry to kill the non-applicable error > message, we went for the quick and dirty fix. >=20 > > Also I fail to see how a caller of (today's) platform_get_irq_optional() > > is simpler than a caller of platform_get_irq() given that there is no > > semantic difference between the two. Please show me a single > > conversion from platform_get_irq to platform_get_irq_optional that > > yielded a simplification. >=20 > That's exactly why we want to change the latter to return 0 ;-) OK. So you agree to my statement "The reason for platform_get_irq_optional()'s existence is just that platform_get_irq() emits an error message [...]". Actually you don't want to oppose but say: It's unfortunate that the silent variant of platform_get_irq() took the obvious name of a function that could have an improved return code semantic. So my suggestion to rename todays platform_get_irq_optional() to platform_get_irq_silently() and then introducing platform_get_irq_optional() with your suggested semantic seems intriguing and straigt forward to me. Another thought: platform_get_irq emits an error message for all problems. Wouldn't it be consistent to let platform_get_irq_optional() emit an error message for all problems but "not found"? Alternatively remove the error printk from platform_get_irq(). > > So you need some more effort to convince me of your POV. > > > > > Even for clocks, you cannot assume that you can always blindly use > > > the returned dummy (actually a NULL pointer) to call into the clk > > > API. While this works fine for simple use cases, where you just > > > want to enable/disable an optional clock (clk_prepare_enable() and > > > clk_disable_unprepare()), it does not work for more complex use cases. > > > > Agreed. But for clks and gpiods and regulators the simple case is quite > > usual. For irqs it isn't. >=20 > It is for devices that can have either separate interrupts, or a single > multiplexed interrupt. >=20 > The logic in e.g. drivers/tty/serial/sh-sci.c and > drivers/spi/spi-rspi.c could be simplified and improved (currently > it doesn't handle deferred probe) if platform_get_irq_optional() > would return 0 instead of -ENXIO. Looking at sh-sci.c the irq handling logic could be improved even without a changed platform_get_irq_optional(): diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index 968967d722d4..c7dc9fb84844 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -2873,11 +2873,13 @@ static int sci_init_single(struct platform_device *= dev, * interrupt ID numbers, or muxed together with another interrupt. */ if (sci_port->irqs[0] < 0) - return -ENXIO; + return sci_port->irqs[0]; =20 - if (sci_port->irqs[1] < 0) + if (sci_port->irqs[1] =3D=3D -ENXIO) for (i =3D 1; i < ARRAY_SIZE(sci_port->irqs); i++) sci_port->irqs[i] =3D sci_port->irqs[0]; + else if (sci_port->irqs[1] < 0) + return sci_port->irqs[1]; =20 sci_port->params =3D sci_probe_regmap(p); if (unlikely(sci_port->params =3D=3D NULL)) And then the code flow is actively irritating. sci_init_single() copies irqs[0] to all other irqs[i] and then sci_request_irq() loops over the already requested irqs and checks for duplicates. A single place that identifies the exact set of required irqs would already help a lot. Also for spi-rspi.c I don't see how platform_get_irq_byname_optional() returning 0 instead of -ENXIO would help. Please talk in patches. Preferably first simplify in-driver logic to make the conversion to the new platform_get_irq_optional() actually reviewable. > > And if you cannot blindly use the dummy, then you're not the targetted > > caller of *_get_optional() and should better use *_get() and handle > > -ENODEV explicitly. >=20 > No, because the janitors tend to consolidate error message handling, > by moving the printing up, inside the *_get() methods. That's exactly > what happened here. This is in my eyes the root cause of the issues at hand. Moving the error message handling into a get function is only right for most of the callers. So the more conservative approach would be to introduce a noisy variant of the get function and convert all users that benefit separately while the unreviewed callers and those that don't want an error message can happily continue to use the silent variant. > So there are three reasons: because the absence of an optional IRQ > is not an error, and thus that should not cause (a) an error code > to be returned, and (b) an error message to be printed, and (c) > because it can simplify the logic in device drivers. I don't agree to (a). If the value signaling not-found is -ENXIO or 0 (or -ENODEV) doesn't matter much. I wouldn't deviate from the return code semantics of platform_get_irq() just for having to check against 0 instead of -ENXIO. Zero is then just another magic value. (c) still has to be proven, see above. > Commit 8973ea47901c81a1 ("driver core: platform: Introduce > platform_get_irq_optional()") fixed (b), but didn't address (a) and > (c). Yes, it fixed (b) and picked a bad name for that. Best regards Uwe --=20 Pengutronix e.K. | Uwe Kleine-K=F6nig | Industrial Linux Solutions | https://www.pengutronix.de/ | --p6yh245p57zhiyck Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEfnIqFpAYrP8+dKQLwfwUeK3K7AkFAmHlV8AACgkQwfwUeK3K 7Am0GQf8CoKYtZsyB2Veq4tA4dVxwehDrqSNzD0/oee9gQ2W8Ug3o/BHJYBwahzq EvMyo3JUywFfBFS6fqP6q+5CXaw3qhcVdLIQIYR1NbdbDku9fPpYgUlMeO8FLj0S AjA1gReJzZffpqQa+j6sWHbwoCmV4ZWTYuhi2tnY6gxes4QcBTcXhrlPtPvEcvRj xiaHDNvm4yBJjau7t98dhCCfb9ioYwkuGybaTVJenP6u4ZB5QxTAKBsVZsaYscE9 K/bTKX+pt+MFJrjy6AN6Qq4JYNuQK8v7MawD5u/q9qZHAELmMQaNyWTpBBDKqjGv Z8p6bAtXmJy2dTalO786GdRxwAWrMQ== =gT+3 -----END PGP SIGNATURE----- --p6yh245p57zhiyck-- --===============8089239644367236678== 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 --===============8089239644367236678==-- 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 683A7C433EF for ; Mon, 17 Jan 2022 11:51:24 +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=PBGRmjBj1n1NrUrg4tXMqO5WGfqwPrXAf7rxA7MNWxQ=; b=MDmC81Q3vEeygivB6/UB6pQ4I1 AA4x3bp4DUL2EzypSi+tZzDfPRIs5Hq58FH+Hs3WOK975y0V1g5oOEMcBN2EAeO+8GT71HM2xX9v6 peirQeWoTo3w9U/KfKaPyANUYcNJLmC/XbE1bZhEPApolltNgPWAjHi4FHjhUcxVrqyiFYPo1dEqw 24/nJg1SLyObe08TBakquvq8VWfUSVA8DuxgS8TH8uGfW8N4abCcJcDdHeHvac6eMO9iw0TWUtvPY yVeRfDZ/emgzCJ8Y4crQXOPmzO3vUGtBG0cUI15KBRh5dhvc7BPPrEf012xHF7H5pIisY36wO1Fyz qng6QHGQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1n9QXj-00EiDR-Rt; Mon, 17 Jan 2022 11:51:23 +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 1n9QXR-00Ei8R-EM for linux-phy@lists.infradead.org; Mon, 17 Jan 2022 11:51:07 +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 1n9QVx-0000fx-Mn; Mon, 17 Jan 2022 12:49:33 +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 1n9QVo-00AoYK-L6; Mon, 17 Jan 2022 12:49:23 +0100 Received: from ukl by ptz.office.stw.pengutronix.de with local (Exim 4.92) (envelope-from ) id 1n9QVn-0001x3-Fg; Mon, 17 Jan 2022 12:49:23 +0100 Date: Mon, 17 Jan 2022 12:49:23 +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 , 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, Khuong Dinh , Florian Fainelli , Matthias Schiffer , Joakim Zhang , Kamal Dasu , Greg Kroah-Hartman , Lee Jones , Bartosz Golaszewski , Daniel Lezcano , Tony Luck , Kishon Vijay Abraham I , bcm-kernel-feedback-list , "open list:SERIAL DRIVERS" , Jakub Kicinski , Zhang Rui , Linux PWM List , Robert Richter , Saravanan Sekar , Corey Minyard , Linux PM list , Linux Kernel Mailing 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 , Eric Auger , Matthias Brugger , Takashi Iwai , platform-driver-x86@vger.kernel.org, Benson Leung , Linux ARM , linux-edac@vger.kernel.org, Sergey Shtylyov , Mun Yew Tham , Hans de Goede , netdev , Yoshihiro Shimoda , Cornelia Huck , Linux MMC List , Liam Girdwood , 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 , "David S. Miller" Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional Message-ID: <20220117114923.d5vajgitxneec7j7@pengutronix.de> References: <20220112213121.5ruae5mxwj6t3qiy@pengutronix.de> <29f0c65d-77f2-e5b2-f6cc-422add8a707d@omp.ru> <20220114092557.jrkfx7ihg26ekzci@pengutronix.de> <61b80939-357d-14f5-df99-b8d102a4e1a1@omp.ru> <20220114202226.ugzklxv4wzr6egwj@pengutronix.de> <20220117092444.opoedfcf5k5u6otq@pengutronix.de> MIME-Version: 1.0 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-phy@lists.infradead.org X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220117_035105_660794_87C24016 X-CRM114-Status: GOOD ( 63.81 ) 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="===============7518680165685253467==" Sender: "linux-phy" Errors-To: linux-phy-bounces+linux-phy=archiver.kernel.org@lists.infradead.org --===============7518680165685253467== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="p6yh245p57zhiyck" Content-Disposition: inline --p6yh245p57zhiyck Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jan 17, 2022 at 11:35:52AM +0100, Geert Uytterhoeven wrote: > Hi Uwe, >=20 > On Mon, Jan 17, 2022 at 10:24 AM Uwe Kleine-K=F6nig > wrote: > > On Mon, Jan 17, 2022 at 09:41:42AM +0100, Geert Uytterhoeven wrote: > > > On Sat, Jan 15, 2022 at 9:22 PM Sergey Shtylyov w= rote: > > > > On 1/14/22 11:22 PM, Uwe Kleine-K=F6nig wrote: > > > > > You have to understand that for clk (and regulator and gpiod) NUL= L is a > > > > > valid descriptor that can actually be used, it just has no effect= =2E So > > > > > this is a convenience value for the case "If the clk/regulator/gp= iod in > > > > > question isn't available, there is nothing to do". This is what m= akes > > > > > clk_get_optional() and the others really useful and justifies the= ir > > > > > existence. This doesn't apply to platform_get_irq_optional(). > > > > > > > > I do understand that. However, IRQs are a different beast with t= heir > > > > own justifications... > > > > > > > > clk_get_optional() is sane and sensible for cases where the clk m= ight be > > > > > absent and it helps you because you don't have to differentiate b= etween > > > > > "not found" and "there is an actual resource". > > > > > > > > > > The reason for platform_get_irq_optional()'s existence is just th= at > > > > > platform_get_irq() emits an error message which is wrong or subop= timal > > > > > > > > I think you are very wrong here. The real reason is to simplify = the > > > > callers. > > > > > > Indeed. > > > > The commit that introduced platform_get_irq_optional() said: > > > > Introduce a new platform_get_irq_optional() that works much like > > platform_get_irq() but does not output an error on failure to > > find the interrupt. > > > > So the author of 8973ea47901c81a1912bd05f1577bed9b5b52506 failed to > > mention the real reason? Or look at > > 31a8d8fa84c51d3ab00bf059158d5de6178cf890: > > > > [...] use platform_get_irq_optional() to get second/third IRQ > > which are optional to avoid below error message during probe: > > [...] > > > > Look through the output of > > > > git log -Splatform_get_irq_optional > > > > to find several more of these. >=20 > Commit 8973ea47901c81a1 ("driver core: platform: Introduce > platform_get_irq_optional()") and the various fixups fixed the ugly > printing of error messages that were not applicable. > In hindsight, probably commit 7723f4c5ecdb8d83 ("driver core: > platform: Add an error message to platform_get_irq*()") should have > been reverted instead, until a platform_get_irq_optional() with proper > semantics was introduced. ack. > But as we were all in a hurry to kill the non-applicable error > message, we went for the quick and dirty fix. >=20 > > Also I fail to see how a caller of (today's) platform_get_irq_optional() > > is simpler than a caller of platform_get_irq() given that there is no > > semantic difference between the two. Please show me a single > > conversion from platform_get_irq to platform_get_irq_optional that > > yielded a simplification. >=20 > That's exactly why we want to change the latter to return 0 ;-) OK. So you agree to my statement "The reason for platform_get_irq_optional()'s existence is just that platform_get_irq() emits an error message [...]". Actually you don't want to oppose but say: It's unfortunate that the silent variant of platform_get_irq() took the obvious name of a function that could have an improved return code semantic. So my suggestion to rename todays platform_get_irq_optional() to platform_get_irq_silently() and then introducing platform_get_irq_optional() with your suggested semantic seems intriguing and straigt forward to me. Another thought: platform_get_irq emits an error message for all problems. Wouldn't it be consistent to let platform_get_irq_optional() emit an error message for all problems but "not found"? Alternatively remove the error printk from platform_get_irq(). > > So you need some more effort to convince me of your POV. > > > > > Even for clocks, you cannot assume that you can always blindly use > > > the returned dummy (actually a NULL pointer) to call into the clk > > > API. While this works fine for simple use cases, where you just > > > want to enable/disable an optional clock (clk_prepare_enable() and > > > clk_disable_unprepare()), it does not work for more complex use cases. > > > > Agreed. But for clks and gpiods and regulators the simple case is quite > > usual. For irqs it isn't. >=20 > It is for devices that can have either separate interrupts, or a single > multiplexed interrupt. >=20 > The logic in e.g. drivers/tty/serial/sh-sci.c and > drivers/spi/spi-rspi.c could be simplified and improved (currently > it doesn't handle deferred probe) if platform_get_irq_optional() > would return 0 instead of -ENXIO. Looking at sh-sci.c the irq handling logic could be improved even without a changed platform_get_irq_optional(): diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index 968967d722d4..c7dc9fb84844 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -2873,11 +2873,13 @@ static int sci_init_single(struct platform_device *= dev, * interrupt ID numbers, or muxed together with another interrupt. */ if (sci_port->irqs[0] < 0) - return -ENXIO; + return sci_port->irqs[0]; =20 - if (sci_port->irqs[1] < 0) + if (sci_port->irqs[1] =3D=3D -ENXIO) for (i =3D 1; i < ARRAY_SIZE(sci_port->irqs); i++) sci_port->irqs[i] =3D sci_port->irqs[0]; + else if (sci_port->irqs[1] < 0) + return sci_port->irqs[1]; =20 sci_port->params =3D sci_probe_regmap(p); if (unlikely(sci_port->params =3D=3D NULL)) And then the code flow is actively irritating. sci_init_single() copies irqs[0] to all other irqs[i] and then sci_request_irq() loops over the already requested irqs and checks for duplicates. A single place that identifies the exact set of required irqs would already help a lot. Also for spi-rspi.c I don't see how platform_get_irq_byname_optional() returning 0 instead of -ENXIO would help. Please talk in patches. Preferably first simplify in-driver logic to make the conversion to the new platform_get_irq_optional() actually reviewable. > > And if you cannot blindly use the dummy, then you're not the targetted > > caller of *_get_optional() and should better use *_get() and handle > > -ENODEV explicitly. >=20 > No, because the janitors tend to consolidate error message handling, > by moving the printing up, inside the *_get() methods. That's exactly > what happened here. This is in my eyes the root cause of the issues at hand. Moving the error message handling into a get function is only right for most of the callers. So the more conservative approach would be to introduce a noisy variant of the get function and convert all users that benefit separately while the unreviewed callers and those that don't want an error message can happily continue to use the silent variant. > So there are three reasons: because the absence of an optional IRQ > is not an error, and thus that should not cause (a) an error code > to be returned, and (b) an error message to be printed, and (c) > because it can simplify the logic in device drivers. I don't agree to (a). If the value signaling not-found is -ENXIO or 0 (or -ENODEV) doesn't matter much. I wouldn't deviate from the return code semantics of platform_get_irq() just for having to check against 0 instead of -ENXIO. Zero is then just another magic value. (c) still has to be proven, see above. > Commit 8973ea47901c81a1 ("driver core: platform: Introduce > platform_get_irq_optional()") fixed (b), but didn't address (a) and > (c). Yes, it fixed (b) and picked a bad name for that. Best regards Uwe --=20 Pengutronix e.K. | Uwe Kleine-K=F6nig | Industrial Linux Solutions | https://www.pengutronix.de/ | --p6yh245p57zhiyck Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEfnIqFpAYrP8+dKQLwfwUeK3K7AkFAmHlV8AACgkQwfwUeK3K 7Am0GQf8CoKYtZsyB2Veq4tA4dVxwehDrqSNzD0/oee9gQ2W8Ug3o/BHJYBwahzq EvMyo3JUywFfBFS6fqP6q+5CXaw3qhcVdLIQIYR1NbdbDku9fPpYgUlMeO8FLj0S AjA1gReJzZffpqQa+j6sWHbwoCmV4ZWTYuhi2tnY6gxes4QcBTcXhrlPtPvEcvRj xiaHDNvm4yBJjau7t98dhCCfb9ioYwkuGybaTVJenP6u4ZB5QxTAKBsVZsaYscE9 K/bTKX+pt+MFJrjy6AN6Qq4JYNuQK8v7MawD5u/q9qZHAELmMQaNyWTpBBDKqjGv Z8p6bAtXmJy2dTalO786GdRxwAWrMQ== =gT+3 -----END PGP SIGNATURE----- --p6yh245p57zhiyck-- --===============7518680165685253467== 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 --===============7518680165685253467==-- 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 9C589C433F5 for ; Mon, 17 Jan 2022 11:51:42 +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=Shx0y9Te5drkzxuckjf5a9HCynBvmm1DmFx1OTd2zJg=; b=C6A7H85n2lgW+FJzybfYP7Bj7G TPR27fHo8qhvt/QWKhMe9Ymkxv76G7qN+4/GyO7NUI1pcGHUZXr5a63BMtoZXYCd8w8npjkP+Wy7R 9NLEi4bgnFOOPy88T7CQLpzJGB/hMCQio2G6vI5f3rDtNdIOM5C13UREKe7xvl194F27tBAraUztM blvFRYTHTvRIq/rWszMU/ciakS/HujQdfmqk3rw69+Tjy4htHbLlzIUJpzapwMA2h6ykdLgbVY4pV g8RsR4eSY+GHT1C8km98vergVA9+K2CeDSk2zHCTNuYGdK0zZo3q9HT94mvjx2JTiJ+iN5dQxTpMu uk6PIuYg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1n9QXV-00Ei9p-6Y; Mon, 17 Jan 2022 11:51:09 +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 1n9QXR-00Ei8M-1h for linux-mtd@lists.infradead.org; Mon, 17 Jan 2022 11:51:07 +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 1n9QVx-0000fx-Mn; Mon, 17 Jan 2022 12:49:33 +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 1n9QVo-00AoYK-L6; Mon, 17 Jan 2022 12:49:23 +0100 Received: from ukl by ptz.office.stw.pengutronix.de with local (Exim 4.92) (envelope-from ) id 1n9QVn-0001x3-Fg; Mon, 17 Jan 2022 12:49:23 +0100 Date: Mon, 17 Jan 2022 12:49:23 +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 , 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, Khuong Dinh , Florian Fainelli , Matthias Schiffer , Joakim Zhang , Kamal Dasu , Greg Kroah-Hartman , Lee Jones , Bartosz Golaszewski , Daniel Lezcano , Tony Luck , Kishon Vijay Abraham I , bcm-kernel-feedback-list , "open list:SERIAL DRIVERS" , Jakub Kicinski , Zhang Rui , Linux PWM List , Robert Richter , Saravanan Sekar , Corey Minyard , Linux PM list , Linux Kernel Mailing 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 , Eric Auger , Matthias Brugger , Takashi Iwai , platform-driver-x86@vger.kernel.org, Benson Leung , Linux ARM , linux-edac@vger.kernel.org, Sergey Shtylyov , Mun Yew Tham , Hans de Goede , netdev , Yoshihiro Shimoda , Cornelia Huck , Linux MMC List , Liam Girdwood , 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 , "David S. Miller" Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional Message-ID: <20220117114923.d5vajgitxneec7j7@pengutronix.de> References: <20220112213121.5ruae5mxwj6t3qiy@pengutronix.de> <29f0c65d-77f2-e5b2-f6cc-422add8a707d@omp.ru> <20220114092557.jrkfx7ihg26ekzci@pengutronix.de> <61b80939-357d-14f5-df99-b8d102a4e1a1@omp.ru> <20220114202226.ugzklxv4wzr6egwj@pengutronix.de> <20220117092444.opoedfcf5k5u6otq@pengutronix.de> MIME-Version: 1.0 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-mtd@lists.infradead.org X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220117_035105_268807_D3C40CD9 X-CRM114-Status: GOOD ( 64.10 ) 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="===============7695964106822029001==" Sender: "linux-mtd" Errors-To: linux-mtd-bounces+linux-mtd=archiver.kernel.org@lists.infradead.org --===============7695964106822029001== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="p6yh245p57zhiyck" Content-Disposition: inline --p6yh245p57zhiyck Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jan 17, 2022 at 11:35:52AM +0100, Geert Uytterhoeven wrote: > Hi Uwe, >=20 > On Mon, Jan 17, 2022 at 10:24 AM Uwe Kleine-K=F6nig > wrote: > > On Mon, Jan 17, 2022 at 09:41:42AM +0100, Geert Uytterhoeven wrote: > > > On Sat, Jan 15, 2022 at 9:22 PM Sergey Shtylyov w= rote: > > > > On 1/14/22 11:22 PM, Uwe Kleine-K=F6nig wrote: > > > > > You have to understand that for clk (and regulator and gpiod) NUL= L is a > > > > > valid descriptor that can actually be used, it just has no effect= =2E So > > > > > this is a convenience value for the case "If the clk/regulator/gp= iod in > > > > > question isn't available, there is nothing to do". This is what m= akes > > > > > clk_get_optional() and the others really useful and justifies the= ir > > > > > existence. This doesn't apply to platform_get_irq_optional(). > > > > > > > > I do understand that. However, IRQs are a different beast with t= heir > > > > own justifications... > > > > > > > > clk_get_optional() is sane and sensible for cases where the clk m= ight be > > > > > absent and it helps you because you don't have to differentiate b= etween > > > > > "not found" and "there is an actual resource". > > > > > > > > > > The reason for platform_get_irq_optional()'s existence is just th= at > > > > > platform_get_irq() emits an error message which is wrong or subop= timal > > > > > > > > I think you are very wrong here. The real reason is to simplify = the > > > > callers. > > > > > > Indeed. > > > > The commit that introduced platform_get_irq_optional() said: > > > > Introduce a new platform_get_irq_optional() that works much like > > platform_get_irq() but does not output an error on failure to > > find the interrupt. > > > > So the author of 8973ea47901c81a1912bd05f1577bed9b5b52506 failed to > > mention the real reason? Or look at > > 31a8d8fa84c51d3ab00bf059158d5de6178cf890: > > > > [...] use platform_get_irq_optional() to get second/third IRQ > > which are optional to avoid below error message during probe: > > [...] > > > > Look through the output of > > > > git log -Splatform_get_irq_optional > > > > to find several more of these. >=20 > Commit 8973ea47901c81a1 ("driver core: platform: Introduce > platform_get_irq_optional()") and the various fixups fixed the ugly > printing of error messages that were not applicable. > In hindsight, probably commit 7723f4c5ecdb8d83 ("driver core: > platform: Add an error message to platform_get_irq*()") should have > been reverted instead, until a platform_get_irq_optional() with proper > semantics was introduced. ack. > But as we were all in a hurry to kill the non-applicable error > message, we went for the quick and dirty fix. >=20 > > Also I fail to see how a caller of (today's) platform_get_irq_optional() > > is simpler than a caller of platform_get_irq() given that there is no > > semantic difference between the two. Please show me a single > > conversion from platform_get_irq to platform_get_irq_optional that > > yielded a simplification. >=20 > That's exactly why we want to change the latter to return 0 ;-) OK. So you agree to my statement "The reason for platform_get_irq_optional()'s existence is just that platform_get_irq() emits an error message [...]". Actually you don't want to oppose but say: It's unfortunate that the silent variant of platform_get_irq() took the obvious name of a function that could have an improved return code semantic. So my suggestion to rename todays platform_get_irq_optional() to platform_get_irq_silently() and then introducing platform_get_irq_optional() with your suggested semantic seems intriguing and straigt forward to me. Another thought: platform_get_irq emits an error message for all problems. Wouldn't it be consistent to let platform_get_irq_optional() emit an error message for all problems but "not found"? Alternatively remove the error printk from platform_get_irq(). > > So you need some more effort to convince me of your POV. > > > > > Even for clocks, you cannot assume that you can always blindly use > > > the returned dummy (actually a NULL pointer) to call into the clk > > > API. While this works fine for simple use cases, where you just > > > want to enable/disable an optional clock (clk_prepare_enable() and > > > clk_disable_unprepare()), it does not work for more complex use cases. > > > > Agreed. But for clks and gpiods and regulators the simple case is quite > > usual. For irqs it isn't. >=20 > It is for devices that can have either separate interrupts, or a single > multiplexed interrupt. >=20 > The logic in e.g. drivers/tty/serial/sh-sci.c and > drivers/spi/spi-rspi.c could be simplified and improved (currently > it doesn't handle deferred probe) if platform_get_irq_optional() > would return 0 instead of -ENXIO. Looking at sh-sci.c the irq handling logic could be improved even without a changed platform_get_irq_optional(): diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index 968967d722d4..c7dc9fb84844 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -2873,11 +2873,13 @@ static int sci_init_single(struct platform_device *= dev, * interrupt ID numbers, or muxed together with another interrupt. */ if (sci_port->irqs[0] < 0) - return -ENXIO; + return sci_port->irqs[0]; =20 - if (sci_port->irqs[1] < 0) + if (sci_port->irqs[1] =3D=3D -ENXIO) for (i =3D 1; i < ARRAY_SIZE(sci_port->irqs); i++) sci_port->irqs[i] =3D sci_port->irqs[0]; + else if (sci_port->irqs[1] < 0) + return sci_port->irqs[1]; =20 sci_port->params =3D sci_probe_regmap(p); if (unlikely(sci_port->params =3D=3D NULL)) And then the code flow is actively irritating. sci_init_single() copies irqs[0] to all other irqs[i] and then sci_request_irq() loops over the already requested irqs and checks for duplicates. A single place that identifies the exact set of required irqs would already help a lot. Also for spi-rspi.c I don't see how platform_get_irq_byname_optional() returning 0 instead of -ENXIO would help. Please talk in patches. Preferably first simplify in-driver logic to make the conversion to the new platform_get_irq_optional() actually reviewable. > > And if you cannot blindly use the dummy, then you're not the targetted > > caller of *_get_optional() and should better use *_get() and handle > > -ENODEV explicitly. >=20 > No, because the janitors tend to consolidate error message handling, > by moving the printing up, inside the *_get() methods. That's exactly > what happened here. This is in my eyes the root cause of the issues at hand. Moving the error message handling into a get function is only right for most of the callers. So the more conservative approach would be to introduce a noisy variant of the get function and convert all users that benefit separately while the unreviewed callers and those that don't want an error message can happily continue to use the silent variant. > So there are three reasons: because the absence of an optional IRQ > is not an error, and thus that should not cause (a) an error code > to be returned, and (b) an error message to be printed, and (c) > because it can simplify the logic in device drivers. I don't agree to (a). If the value signaling not-found is -ENXIO or 0 (or -ENODEV) doesn't matter much. I wouldn't deviate from the return code semantics of platform_get_irq() just for having to check against 0 instead of -ENXIO. Zero is then just another magic value. (c) still has to be proven, see above. > Commit 8973ea47901c81a1 ("driver core: platform: Introduce > platform_get_irq_optional()") fixed (b), but didn't address (a) and > (c). Yes, it fixed (b) and picked a bad name for that. Best regards Uwe --=20 Pengutronix e.K. | Uwe Kleine-K=F6nig | Industrial Linux Solutions | https://www.pengutronix.de/ | --p6yh245p57zhiyck Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEfnIqFpAYrP8+dKQLwfwUeK3K7AkFAmHlV8AACgkQwfwUeK3K 7Am0GQf8CoKYtZsyB2Veq4tA4dVxwehDrqSNzD0/oee9gQ2W8Ug3o/BHJYBwahzq EvMyo3JUywFfBFS6fqP6q+5CXaw3qhcVdLIQIYR1NbdbDku9fPpYgUlMeO8FLj0S AjA1gReJzZffpqQa+j6sWHbwoCmV4ZWTYuhi2tnY6gxes4QcBTcXhrlPtPvEcvRj xiaHDNvm4yBJjau7t98dhCCfb9ioYwkuGybaTVJenP6u4ZB5QxTAKBsVZsaYscE9 K/bTKX+pt+MFJrjy6AN6Qq4JYNuQK8v7MawD5u/q9qZHAELmMQaNyWTpBBDKqjGv Z8p6bAtXmJy2dTalO786GdRxwAWrMQ== =gT+3 -----END PGP SIGNATURE----- --p6yh245p57zhiyck-- --===============7695964106822029001== 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/ --===============7695964106822029001==-- 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 78FDDC433F5 for ; Thu, 20 Jan 2022 07:07:16 +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 B4D302F94; Thu, 20 Jan 2022 08:06:24 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz B4D302F94 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1642662434; bh=vujQr2/zrQOvezve/LsE4kh3o7fetXKGS/+sT5eT9kI=; h=Date:From:To:Subject:References:In-Reply-To:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=ZUHj/vSgdMkXyaM3aqEb/ex55RLCjiChYPy+0gfnHA8im+mNDqjR50Khy0XY0mFoD 5nxGKi6lmDKFowbFQmOIQHEBc9HyqCoNk4iVlJ1lAnrKXIePmIzBKa26HISPn7vqUA dkzjm43zmr1jV9qD7zkcfvrf/P1XzejMfHedYN4g= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id C9BD6F80535; Thu, 20 Jan 2022 08:03:43 +0100 (CET) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 875C6F80425; Mon, 17 Jan 2022 12:49:44 +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 AC39BF80249 for ; Mon, 17 Jan 2022 12:49:36 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz AC39BF80249 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 1n9QVx-0000fx-Mn; Mon, 17 Jan 2022 12:49:33 +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 1n9QVo-00AoYK-L6; Mon, 17 Jan 2022 12:49:23 +0100 Received: from ukl by ptz.office.stw.pengutronix.de with local (Exim 4.92) (envelope-from ) id 1n9QVn-0001x3-Fg; Mon, 17 Jan 2022 12:49:23 +0100 Date: Mon, 17 Jan 2022 12:49:23 +0100 From: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= To: Geert Uytterhoeven Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional Message-ID: <20220117114923.d5vajgitxneec7j7@pengutronix.de> References: <20220112213121.5ruae5mxwj6t3qiy@pengutronix.de> <29f0c65d-77f2-e5b2-f6cc-422add8a707d@omp.ru> <20220114092557.jrkfx7ihg26ekzci@pengutronix.de> <61b80939-357d-14f5-df99-b8d102a4e1a1@omp.ru> <20220114202226.ugzklxv4wzr6egwj@pengutronix.de> <20220117092444.opoedfcf5k5u6otq@pengutronix.de> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="p6yh245p57zhiyck" 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: alsa-devel@alsa-project.org X-Mailman-Approved-At: Thu, 20 Jan 2022 08:03:35 +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 , Miquel Raynal , linux-phy@lists.infradead.org, linux-spi , Jiri Slaby , "David S. Miller" , 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 , platform-driver-x86@vger.kernel.org, Linux PWM List , Hans de Goede , Robert Richter , Saravanan Sekar , Corey Minyard , Linux PM list , Liam Girdwood , Mauro Carvalho Chehab , John Garry , Peter Korsgaard , William Breathitt Gray , Mark Gross , "open list:GPIO SUBSYSTEM" , Alex Williamson , Mark Brown , Borislav Petkov , Takashi Iwai , 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 , Sergey Shtylyov , Vinod Koul , James Morse , Zha Qipeng , Sebastian Reichel , Niklas =?utf-8?Q?S=C3=B6derlund?= , linux-mediatek@lists.infradead.org, Brian Norris , netdev 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" --p6yh245p57zhiyck Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jan 17, 2022 at 11:35:52AM +0100, Geert Uytterhoeven wrote: > Hi Uwe, >=20 > On Mon, Jan 17, 2022 at 10:24 AM Uwe Kleine-K=F6nig > wrote: > > On Mon, Jan 17, 2022 at 09:41:42AM +0100, Geert Uytterhoeven wrote: > > > On Sat, Jan 15, 2022 at 9:22 PM Sergey Shtylyov w= rote: > > > > On 1/14/22 11:22 PM, Uwe Kleine-K=F6nig wrote: > > > > > You have to understand that for clk (and regulator and gpiod) NUL= L is a > > > > > valid descriptor that can actually be used, it just has no effect= =2E So > > > > > this is a convenience value for the case "If the clk/regulator/gp= iod in > > > > > question isn't available, there is nothing to do". This is what m= akes > > > > > clk_get_optional() and the others really useful and justifies the= ir > > > > > existence. This doesn't apply to platform_get_irq_optional(). > > > > > > > > I do understand that. However, IRQs are a different beast with t= heir > > > > own justifications... > > > > > > > > clk_get_optional() is sane and sensible for cases where the clk m= ight be > > > > > absent and it helps you because you don't have to differentiate b= etween > > > > > "not found" and "there is an actual resource". > > > > > > > > > > The reason for platform_get_irq_optional()'s existence is just th= at > > > > > platform_get_irq() emits an error message which is wrong or subop= timal > > > > > > > > I think you are very wrong here. The real reason is to simplify = the > > > > callers. > > > > > > Indeed. > > > > The commit that introduced platform_get_irq_optional() said: > > > > Introduce a new platform_get_irq_optional() that works much like > > platform_get_irq() but does not output an error on failure to > > find the interrupt. > > > > So the author of 8973ea47901c81a1912bd05f1577bed9b5b52506 failed to > > mention the real reason? Or look at > > 31a8d8fa84c51d3ab00bf059158d5de6178cf890: > > > > [...] use platform_get_irq_optional() to get second/third IRQ > > which are optional to avoid below error message during probe: > > [...] > > > > Look through the output of > > > > git log -Splatform_get_irq_optional > > > > to find several more of these. >=20 > Commit 8973ea47901c81a1 ("driver core: platform: Introduce > platform_get_irq_optional()") and the various fixups fixed the ugly > printing of error messages that were not applicable. > In hindsight, probably commit 7723f4c5ecdb8d83 ("driver core: > platform: Add an error message to platform_get_irq*()") should have > been reverted instead, until a platform_get_irq_optional() with proper > semantics was introduced. ack. > But as we were all in a hurry to kill the non-applicable error > message, we went for the quick and dirty fix. >=20 > > Also I fail to see how a caller of (today's) platform_get_irq_optional() > > is simpler than a caller of platform_get_irq() given that there is no > > semantic difference between the two. Please show me a single > > conversion from platform_get_irq to platform_get_irq_optional that > > yielded a simplification. >=20 > That's exactly why we want to change the latter to return 0 ;-) OK. So you agree to my statement "The reason for platform_get_irq_optional()'s existence is just that platform_get_irq() emits an error message [...]". Actually you don't want to oppose but say: It's unfortunate that the silent variant of platform_get_irq() took the obvious name of a function that could have an improved return code semantic. So my suggestion to rename todays platform_get_irq_optional() to platform_get_irq_silently() and then introducing platform_get_irq_optional() with your suggested semantic seems intriguing and straigt forward to me. Another thought: platform_get_irq emits an error message for all problems. Wouldn't it be consistent to let platform_get_irq_optional() emit an error message for all problems but "not found"? Alternatively remove the error printk from platform_get_irq(). > > So you need some more effort to convince me of your POV. > > > > > Even for clocks, you cannot assume that you can always blindly use > > > the returned dummy (actually a NULL pointer) to call into the clk > > > API. While this works fine for simple use cases, where you just > > > want to enable/disable an optional clock (clk_prepare_enable() and > > > clk_disable_unprepare()), it does not work for more complex use cases. > > > > Agreed. But for clks and gpiods and regulators the simple case is quite > > usual. For irqs it isn't. >=20 > It is for devices that can have either separate interrupts, or a single > multiplexed interrupt. >=20 > The logic in e.g. drivers/tty/serial/sh-sci.c and > drivers/spi/spi-rspi.c could be simplified and improved (currently > it doesn't handle deferred probe) if platform_get_irq_optional() > would return 0 instead of -ENXIO. Looking at sh-sci.c the irq handling logic could be improved even without a changed platform_get_irq_optional(): diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index 968967d722d4..c7dc9fb84844 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -2873,11 +2873,13 @@ static int sci_init_single(struct platform_device *= dev, * interrupt ID numbers, or muxed together with another interrupt. */ if (sci_port->irqs[0] < 0) - return -ENXIO; + return sci_port->irqs[0]; =20 - if (sci_port->irqs[1] < 0) + if (sci_port->irqs[1] =3D=3D -ENXIO) for (i =3D 1; i < ARRAY_SIZE(sci_port->irqs); i++) sci_port->irqs[i] =3D sci_port->irqs[0]; + else if (sci_port->irqs[1] < 0) + return sci_port->irqs[1]; =20 sci_port->params =3D sci_probe_regmap(p); if (unlikely(sci_port->params =3D=3D NULL)) And then the code flow is actively irritating. sci_init_single() copies irqs[0] to all other irqs[i] and then sci_request_irq() loops over the already requested irqs and checks for duplicates. A single place that identifies the exact set of required irqs would already help a lot. Also for spi-rspi.c I don't see how platform_get_irq_byname_optional() returning 0 instead of -ENXIO would help. Please talk in patches. Preferably first simplify in-driver logic to make the conversion to the new platform_get_irq_optional() actually reviewable. > > And if you cannot blindly use the dummy, then you're not the targetted > > caller of *_get_optional() and should better use *_get() and handle > > -ENODEV explicitly. >=20 > No, because the janitors tend to consolidate error message handling, > by moving the printing up, inside the *_get() methods. That's exactly > what happened here. This is in my eyes the root cause of the issues at hand. Moving the error message handling into a get function is only right for most of the callers. So the more conservative approach would be to introduce a noisy variant of the get function and convert all users that benefit separately while the unreviewed callers and those that don't want an error message can happily continue to use the silent variant. > So there are three reasons: because the absence of an optional IRQ > is not an error, and thus that should not cause (a) an error code > to be returned, and (b) an error message to be printed, and (c) > because it can simplify the logic in device drivers. I don't agree to (a). If the value signaling not-found is -ENXIO or 0 (or -ENODEV) doesn't matter much. I wouldn't deviate from the return code semantics of platform_get_irq() just for having to check against 0 instead of -ENXIO. Zero is then just another magic value. (c) still has to be proven, see above. > Commit 8973ea47901c81a1 ("driver core: platform: Introduce > platform_get_irq_optional()") fixed (b), but didn't address (a) and > (c). Yes, it fixed (b) and picked a bad name for that. Best regards Uwe --=20 Pengutronix e.K. | Uwe Kleine-K=F6nig | Industrial Linux Solutions | https://www.pengutronix.de/ | --p6yh245p57zhiyck Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEfnIqFpAYrP8+dKQLwfwUeK3K7AkFAmHlV8AACgkQwfwUeK3K 7Am0GQf8CoKYtZsyB2Veq4tA4dVxwehDrqSNzD0/oee9gQ2W8Ug3o/BHJYBwahzq EvMyo3JUywFfBFS6fqP6q+5CXaw3qhcVdLIQIYR1NbdbDku9fPpYgUlMeO8FLj0S AjA1gReJzZffpqQa+j6sWHbwoCmV4ZWTYuhi2tnY6gxes4QcBTcXhrlPtPvEcvRj xiaHDNvm4yBJjau7t98dhCCfb9ioYwkuGybaTVJenP6u4ZB5QxTAKBsVZsaYscE9 K/bTKX+pt+MFJrjy6AN6Qq4JYNuQK8v7MawD5u/q9qZHAELmMQaNyWTpBBDKqjGv Z8p6bAtXmJy2dTalO786GdRxwAWrMQ== =gT+3 -----END PGP SIGNATURE----- --p6yh245p57zhiyck--