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 8C38EC4332F for ; Tue, 18 Jan 2022 20:22:01 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1349152AbiARUV7 (ORCPT ); Tue, 18 Jan 2022 15:21:59 -0500 Received: from mxout03.lancloud.ru ([45.84.86.113]:41690 "EHLO mxout03.lancloud.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233125AbiARUV4 (ORCPT ); Tue, 18 Jan 2022 15:21:56 -0500 Received: from LanCloud DKIM-Filter: OpenDKIM Filter v2.11.0 mxout03.lancloud.ru 291BD20A4270 Received: from LanCloud Received: from LanCloud Received: from LanCloud Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional To: =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= CC: Andrew Lunn , Ulf Hansson , Vignesh Raghavendra , KVM list , "Rafael J. Wysocki" , , "Linus Walleij" , Amit Kucheria , "ALSA Development Mailing List" , Jaroslav Kysela , Guenter Roeck , Thierry Reding , MTD Maling List , Linux I2C , "open list:GPIO SUBSYSTEM" , Miquel Raynal , , , 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 , , 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 , , "Andy Shevchenko" , Benson Leung , Pengutronix Kernel Team , "Linux ARM" , , 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" , =?UTF-8?Q?Niklas_S=c3=b6derlund?= , , "Brian Norris" , "David S. Miller" References: <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> <20220114202226.ugzklxv4wzr6egwj@pengutronix.de> <57af1851-9341-985e-7b28-d2ba86770ecb@omp.ru> <20220117084732.cdy2sash5hxp4lwo@pengutronix.de> From: Sergey Shtylyov Organization: Open Mobile Platform Message-ID: <68d3bb7a-7572-7495-d295-e1d512ef509e@omp.ru> Date: Tue, 18 Jan 2022 23:21:45 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <20220117084732.cdy2sash5hxp4lwo@pengutronix.de> Content-Type: text/plain; charset="windows-1252" Content-Language: en-US Content-Transfer-Encoding: 8bit X-Originating-IP: [192.168.11.198] X-ClientProxiedBy: LFEXT01.lancloud.ru (fd00:f066::141) To LFEX1907.lancloud.ru (fd00:f066::207) Precedence: bulk List-ID: X-Mailing-List: linux-gpio@vger.kernel.org Hello! On 1/17/22 11:47 AM, Uwe Kleine-König wrote: [...] >>>>>>>>> To me it sounds much more logical for the driver to check if an >>>>>>>>> optional irq is non-zero (available) or zero (not available), than to >>>>>>>>> sprinkle around checks for -ENXIO. In addition, you have to remember >>>>>>>>> that this one returns -ENXIO, while other APIs use -ENOENT or -ENOSYS >>>>>>>>> (or some other error code) to indicate absence. I thought not having >>>>>>>>> to care about the actual error code was the main reason behind the >>>>>>>>> introduction of the *_optional() APIs. >>>>>>> >>>>>>>> No, the main benefit of gpiod_get_optional() (and clk_get_optional()) is >>>>>>>> that you can handle an absent GPIO (or clk) as if it were available. >>>>>> >>>>>> 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 sake >>>>>> of i8253 drivers in arch/... >>>>> >>>>> Let me reformulate your statement to the IMHO equivalent: >>>>> >>>>> If you set aside the differences between >>>>> platform_get_irq_optional() and gpiod_get_optional(), >>>> >>>> Sorry, I should make it clear this is actually the diff between a would-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. >> >> I have noting to say here, rather than optional IRQ could well have a different >> meaning than for clk/gpio/etc. >> >> [...] >>>>> 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. >>>>> >>>>> Leaving error message reporting aside, the introduction of >>>>> platform_get_irq_optional() allows to change >>>>> >>>>> irq = platform_get_irq(...); >>>>> if (irq < 0 && irq != -ENXIO) { >>>>> return irq; >>>>> } else if (irq >= 0) { >>>> >>>> Rather (irq > 0) actually, IRQ0 is considered invalid (but still returned). >>> >>> 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 ... >> >> Note that we should absolutely (and first of all) stop returning 0 from platform_get_irq() >> on a "real" IRQ0. Handling that "still good" zero absolutely doesn't scale e.g. for the subsystems >> (like libata) which take 0 as an indication that the polling mode should be used... We can't afford >> to be sloppy here. ;-) > > Then maybe do that really first? I'm doing it first already: https://lore.kernel.org/all/5e001ec1-d3f1-bcb8-7f30-a6301fd9930c@omp.ru/ This series is atop of the above patch... > I didn't recheck, but is this what the > driver changes in your patch is about? Partly, yes. We can afford to play with the meaning of 0 after the above patch. > After some more thoughts I wonder if your focus isn't to align > platform_get_irq_optional to (clk|gpiod|regulator)_get_optional, but to > simplify return code checking. Because with your change we have: > > - < 0 -> error > - == 0 -> no irq > - > 0 -> irq Mainly, yes. That's why the code examples were given in the description. > For my part I'd say this doesn't justify the change, but at least I > could better life with the reasoning. If you start at: > > irq = platform_get_irq_optional(...) > if (irq < 0 && irq != -ENXIO) > return irq > else if (irq > 0) > setup_irq(irq); > else > setup_polling() > > I'd change that to > > irq = platform_get_irq_optional(...) > if (irq > 0) /* or >= 0 ? */ Not >= 0, no... > setup_irq(irq) > else if (irq == -ENXIO) > setup_polling() > else > return irq > > This still has to mention -ENXIO, but this is ok and checking for 0 just > hardcodes a different return value. I think comparing with 0 is simpler (and shorter) than with -ENXIO, if you consider the RISC CPUs, like e.g. MIPS... > Anyhow, I think if you still want to change platform_get_irq_optional > you should add a few patches converting some drivers which demonstrates > the improvement for the callers. Mhm, I did include all the drivers where the IRQ checks have to be modified, not sure what else you want me to touch... > Best regards > Uwe MBR, Sergey 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 C80F1C433EF for ; Tue, 18 Jan 2022 20:22:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date: Message-ID:From:References:CC:To:Subject:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Owner; bh=OY7Ww+tAD7fjcect53zpzatygYNAkABVDaZveAyGrFk=; b=Ln9tXQrvJJzJlFfSgLn2Ztv6/X ks4VAWbZ5yT/MpE66DkKTVbZuCAtOnzZ0/rt7HKszzSKwR/Hehe/++qBGm2bnCHbnAEXoAXSNhkIX ZfZI08DlM2CWUFW7olkUyMqRTZsu/3IeL/L/nygxR5HsFhLNeXhKrFFjw4NlTnPrombfzeaJ7vVK5 O/8LGZJxUerdcdjjvy9vjr2APFrh1NQCdgn64XNnGJX4S6IhLde2atYnic0EwA38YWGRA5njbyoTm rclSgEKkx98tRiYyizye1Vh7oN/WsPhOLHX7uBw/Dg08aBrByvPvSrgC8p7X9CTGw1ZdAr9uT543v Lm3dDk0A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1n9uzX-002s9Q-4q; Tue, 18 Jan 2022 20:22:07 +0000 Received: from mxout02.lancloud.ru ([45.84.86.82]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1n9uzQ-002s6e-Db; Tue, 18 Jan 2022 20:22:04 +0000 Received: from LanCloud DKIM-Filter: OpenDKIM Filter v2.11.0 mxout02.lancloud.ru 27A6E20B359D Received: from LanCloud Received: from LanCloud Received: from LanCloud Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional To: =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= CC: Andrew Lunn , Ulf Hansson , Vignesh Raghavendra , KVM list , "Rafael J. Wysocki" , , "Linus Walleij" , Amit Kucheria , "ALSA Development Mailing List" , Jaroslav Kysela , Guenter Roeck , Thierry Reding , MTD Maling List , Linux I2C , "open list:GPIO SUBSYSTEM" , Miquel Raynal , , , 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 , , 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 , , "Andy Shevchenko" , Benson Leung , Pengutronix Kernel Team , "Linux ARM" , , 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" , =?UTF-8?Q?Niklas_S=c3=b6derlund?= , , "Brian Norris" , "David S. Miller" References: <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> <20220114202226.ugzklxv4wzr6egwj@pengutronix.de> <57af1851-9341-985e-7b28-d2ba86770ecb@omp.ru> <20220117084732.cdy2sash5hxp4lwo@pengutronix.de> From: Sergey Shtylyov Organization: Open Mobile Platform Message-ID: <68d3bb7a-7572-7495-d295-e1d512ef509e@omp.ru> Date: Tue, 18 Jan 2022 23:21:45 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <20220117084732.cdy2sash5hxp4lwo@pengutronix.de> Content-Language: en-US X-Originating-IP: [192.168.11.198] X-ClientProxiedBy: LFEXT01.lancloud.ru (fd00:f066::141) To LFEX1907.lancloud.ru (fd00:f066::207) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220118_122200_863165_78C5137D X-CRM114-Status: GOOD ( 34.50 ) 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: text/plain; charset="windows-1252" Content-Transfer-Encoding: quoted-printable Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org Hello! On 1/17/22 11:47 AM, Uwe Kleine-K=F6nig wrote: [...] >>>>>>>>> To me it sounds much more logical for the driver to check if an >>>>>>>>> optional irq is non-zero (available) or zero (not available), tha= n to >>>>>>>>> sprinkle around checks for -ENXIO. In addition, you have to remem= ber >>>>>>>>> that this one returns -ENXIO, while other APIs use -ENOENT or -EN= OSYS >>>>>>>>> (or some other error code) to indicate absence. I thought not hav= ing >>>>>>>>> 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 availabl= e. >>>>>> >>>>>> Hm, I've just looked at these and must note that they match 1:1 w= ith >>>>>> platform_get_irq_optional(). Unfortunately, we can't however behave = the >>>>>> same way in request_irq() -- because it has to support IRQ0 for the = sake >>>>>> of i8253 drivers in arch/... >>>>> >>>>> Let me reformulate your statement to the IMHO equivalent: >>>>> >>>>> If you set aside the differences between >>>>> platform_get_irq_optional() and gpiod_get_optional(), >>>> >>>> Sorry, I should make it clear this is actually the diff between a w= ould-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. >> >> I have noting to say here, rather than optional IRQ could well have a= different >> meaning than for clk/gpio/etc. >> >> [...] >>>>> However for an interupt this cannot work. You will always have to che= ck >>>>> if the irq is actually there or not because if it's not you cannot ju= st >>>>> ignore that. So there is no benefit of an optional irq. >>>>> >>>>> Leaving error message reporting aside, the introduction of >>>>> platform_get_irq_optional() allows to change >>>>> >>>>> irq =3D platform_get_irq(...); >>>>> if (irq < 0 && irq !=3D -ENXIO) { >>>>> return irq; >>>>> } else if (irq >=3D 0) { >>>> >>>> Rather (irq > 0) actually, IRQ0 is considered invalid (but still re= turned). >>> >>> 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 ... >> >> Note that we should absolutely (and first of all) stop returning 0 fr= om platform_get_irq() >> on a "real" IRQ0. Handling that "still good" zero absolutely doesn't sca= le e.g. for the subsystems >> (like libata) which take 0 as an indication that the polling mode should= be used... We can't afford >> to be sloppy here. ;-) > = > Then maybe do that really first? I'm doing it first already: https://lore.kernel.org/all/5e001ec1-d3f1-bcb8-7f30-a6301fd9930c@omp.ru/ This series is atop of the above patch... > I didn't recheck, but is this what the > driver changes in your patch is about? Partly, yes. We can afford to play with the meaning of 0 after the above= patch. > After some more thoughts I wonder if your focus isn't to align > platform_get_irq_optional to (clk|gpiod|regulator)_get_optional, but to > simplify return code checking. Because with your change we have: > = > - < 0 -> error > - =3D=3D 0 -> no irq > - > 0 -> irq Mainly, yes. That's why the code examples were given in the description. > For my part I'd say this doesn't justify the change, but at least I > could better life with the reasoning. If you start at: > = > irq =3D platform_get_irq_optional(...) > if (irq < 0 && irq !=3D -ENXIO) > return irq > else if (irq > 0) > setup_irq(irq); > else > setup_polling() > = > I'd change that to > = > irq =3D platform_get_irq_optional(...) > if (irq > 0) /* or >=3D 0 ? */ Not >=3D 0, no... > setup_irq(irq) > else if (irq =3D=3D -ENXIO) > setup_polling() > else > return irq > = > This still has to mention -ENXIO, but this is ok and checking for 0 just > hardcodes a different return value. I think comparing with 0 is simpler (and shorter) than with -ENXIO, if y= ou consider the RISC CPUs, like e.g. MIPS... > Anyhow, I think if you still want to change platform_get_irq_optional > you should add a few patches converting some drivers which demonstrates > the improvement for the callers. Mhm, I did include all the drivers where the IRQ checks have to be modif= ied, not sure what else you want me to touch... > Best regards > Uwe MBR, Sergey _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek 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 EB5E6C433EF for ; Tue, 18 Jan 2022 20:22:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date: Message-ID:From:References:CC:To:Subject:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Owner; bh=RoK13qXBuwvJpjFx/9hVv9CclM3wuyhcU+2p5Uq4Tp0=; b=EJo0PnClblLPKAh7ME0oygqfst tYUiX7cEU9sH+2R3xyQmcfIhakPIH0zcPSqthekzb1RRG2CK93cAXNuEE5b3UAFga2ZJVwsvB3PX5 YDUSHAPYvMabQhKWw8Z2ROWvNBeFgClFfulvvgVKut3SE/DL9nverP512xXHfTAZOU9y44jhRDtFD cCwhGi6neLwzHXbbj/+BE9HH8F4ui8YMd+AbGPpqPbLQznXyM/Jgin6LFLgqLt7XwRl/FD5GVfRP4 UvRn3pjsTuORZYAkc7qxr0j1yisw/VaZEh7txg5JcEgVEZEZ5at39sfuGI48o4sa20Um31SqVSS8Z zwR+PbyQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1n9uzk-002sBl-D2; Tue, 18 Jan 2022 20:22:20 +0000 Received: from mxout02.lancloud.ru ([45.84.86.82]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1n9uzQ-002s6e-Db; Tue, 18 Jan 2022 20:22:04 +0000 Received: from LanCloud DKIM-Filter: OpenDKIM Filter v2.11.0 mxout02.lancloud.ru 27A6E20B359D Received: from LanCloud Received: from LanCloud Received: from LanCloud Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional To: =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= CC: Andrew Lunn , Ulf Hansson , Vignesh Raghavendra , KVM list , "Rafael J. Wysocki" , , "Linus Walleij" , Amit Kucheria , "ALSA Development Mailing List" , Jaroslav Kysela , Guenter Roeck , Thierry Reding , MTD Maling List , Linux I2C , "open list:GPIO SUBSYSTEM" , Miquel Raynal , , , 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 , , 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 , , "Andy Shevchenko" , Benson Leung , Pengutronix Kernel Team , "Linux ARM" , , 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" , =?UTF-8?Q?Niklas_S=c3=b6derlund?= , , "Brian Norris" , "David S. Miller" References: <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> <20220114202226.ugzklxv4wzr6egwj@pengutronix.de> <57af1851-9341-985e-7b28-d2ba86770ecb@omp.ru> <20220117084732.cdy2sash5hxp4lwo@pengutronix.de> From: Sergey Shtylyov Organization: Open Mobile Platform Message-ID: <68d3bb7a-7572-7495-d295-e1d512ef509e@omp.ru> Date: Tue, 18 Jan 2022 23:21:45 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <20220117084732.cdy2sash5hxp4lwo@pengutronix.de> Content-Language: en-US X-Originating-IP: [192.168.11.198] X-ClientProxiedBy: LFEXT01.lancloud.ru (fd00:f066::141) To LFEX1907.lancloud.ru (fd00:f066::207) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220118_122200_863165_78C5137D X-CRM114-Status: GOOD ( 34.50 ) 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: text/plain; charset="windows-1252" Content-Transfer-Encoding: quoted-printable Sender: "linux-phy" Errors-To: linux-phy-bounces+linux-phy=archiver.kernel.org@lists.infradead.org Hello! On 1/17/22 11:47 AM, Uwe Kleine-K=F6nig wrote: [...] >>>>>>>>> To me it sounds much more logical for the driver to check if an >>>>>>>>> optional irq is non-zero (available) or zero (not available), tha= n to >>>>>>>>> sprinkle around checks for -ENXIO. In addition, you have to remem= ber >>>>>>>>> that this one returns -ENXIO, while other APIs use -ENOENT or -EN= OSYS >>>>>>>>> (or some other error code) to indicate absence. I thought not hav= ing >>>>>>>>> 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 availabl= e. >>>>>> >>>>>> Hm, I've just looked at these and must note that they match 1:1 w= ith >>>>>> platform_get_irq_optional(). Unfortunately, we can't however behave = the >>>>>> same way in request_irq() -- because it has to support IRQ0 for the = sake >>>>>> of i8253 drivers in arch/... >>>>> >>>>> Let me reformulate your statement to the IMHO equivalent: >>>>> >>>>> If you set aside the differences between >>>>> platform_get_irq_optional() and gpiod_get_optional(), >>>> >>>> Sorry, I should make it clear this is actually the diff between a w= ould-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. >> >> I have noting to say here, rather than optional IRQ could well have a= different >> meaning than for clk/gpio/etc. >> >> [...] >>>>> However for an interupt this cannot work. You will always have to che= ck >>>>> if the irq is actually there or not because if it's not you cannot ju= st >>>>> ignore that. So there is no benefit of an optional irq. >>>>> >>>>> Leaving error message reporting aside, the introduction of >>>>> platform_get_irq_optional() allows to change >>>>> >>>>> irq =3D platform_get_irq(...); >>>>> if (irq < 0 && irq !=3D -ENXIO) { >>>>> return irq; >>>>> } else if (irq >=3D 0) { >>>> >>>> Rather (irq > 0) actually, IRQ0 is considered invalid (but still re= turned). >>> >>> 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 ... >> >> Note that we should absolutely (and first of all) stop returning 0 fr= om platform_get_irq() >> on a "real" IRQ0. Handling that "still good" zero absolutely doesn't sca= le e.g. for the subsystems >> (like libata) which take 0 as an indication that the polling mode should= be used... We can't afford >> to be sloppy here. ;-) > = > Then maybe do that really first? I'm doing it first already: https://lore.kernel.org/all/5e001ec1-d3f1-bcb8-7f30-a6301fd9930c@omp.ru/ This series is atop of the above patch... > I didn't recheck, but is this what the > driver changes in your patch is about? Partly, yes. We can afford to play with the meaning of 0 after the above= patch. > After some more thoughts I wonder if your focus isn't to align > platform_get_irq_optional to (clk|gpiod|regulator)_get_optional, but to > simplify return code checking. Because with your change we have: > = > - < 0 -> error > - =3D=3D 0 -> no irq > - > 0 -> irq Mainly, yes. That's why the code examples were given in the description. > For my part I'd say this doesn't justify the change, but at least I > could better life with the reasoning. If you start at: > = > irq =3D platform_get_irq_optional(...) > if (irq < 0 && irq !=3D -ENXIO) > return irq > else if (irq > 0) > setup_irq(irq); > else > setup_polling() > = > I'd change that to > = > irq =3D platform_get_irq_optional(...) > if (irq > 0) /* or >=3D 0 ? */ Not >=3D 0, no... > setup_irq(irq) > else if (irq =3D=3D -ENXIO) > setup_polling() > else > return irq > = > This still has to mention -ENXIO, but this is ok and checking for 0 just > hardcodes a different return value. I think comparing with 0 is simpler (and shorter) than with -ENXIO, if y= ou consider the RISC CPUs, like e.g. MIPS... > Anyhow, I think if you still want to change platform_get_irq_optional > you should add a few patches converting some drivers which demonstrates > the improvement for the callers. Mhm, I did include all the drivers where the IRQ checks have to be modif= ied, not sure what else you want me to touch... > Best regards > Uwe MBR, Sergey -- = linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy 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 C246CC433F5 for ; Tue, 18 Jan 2022 20:22:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date: Message-ID:From:References:CC:To:Subject:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Owner; bh=Jh4ck83TflUsruovPh074uI/bSRwM5nic/MyQkDcEuY=; b=TgbKoXlr6nrUJfMfV52e8yO4Wv MjHpI6LchexgMxvup/Djxk5/HvPJN9YQsU+lFtyAxEPlv1f1rUU0R9Bp+vng6QqE1Fo6sTtsFltyG PdVR5yNQYfyiPua8GT4Dre46OQZL/eDkLNkecG5a3+yvPZPC9+e2fHF0YFcBveoJYrFLRR6rch2wf 5nUTb50ciEEnuv36UKgDcPwpP3gs6HoaG1ttfF8jT+dVFjVqt2kp+uNaAlGvgAuY6zcB7wKTux0RO jxXmM/jKmvwMthGmM+Byyyptfdpo33XzPHLK0C+Hxo0cUQ33GqSttMXsO0cTxyw43A0AEEbqD9DLM MkYm5pzA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1n9uzY-002s9n-6y; Tue, 18 Jan 2022 20:22:08 +0000 Received: from mxout02.lancloud.ru ([45.84.86.82]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1n9uzQ-002s6e-Db; Tue, 18 Jan 2022 20:22:04 +0000 Received: from LanCloud DKIM-Filter: OpenDKIM Filter v2.11.0 mxout02.lancloud.ru 27A6E20B359D Received: from LanCloud Received: from LanCloud Received: from LanCloud Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional To: =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= CC: Andrew Lunn , Ulf Hansson , Vignesh Raghavendra , KVM list , "Rafael J. Wysocki" , , "Linus Walleij" , Amit Kucheria , "ALSA Development Mailing List" , Jaroslav Kysela , Guenter Roeck , Thierry Reding , MTD Maling List , Linux I2C , "open list:GPIO SUBSYSTEM" , Miquel Raynal , , , 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 , , 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 , , "Andy Shevchenko" , Benson Leung , Pengutronix Kernel Team , "Linux ARM" , , 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" , =?UTF-8?Q?Niklas_S=c3=b6derlund?= , , "Brian Norris" , "David S. Miller" References: <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> <20220114202226.ugzklxv4wzr6egwj@pengutronix.de> <57af1851-9341-985e-7b28-d2ba86770ecb@omp.ru> <20220117084732.cdy2sash5hxp4lwo@pengutronix.de> From: Sergey Shtylyov Organization: Open Mobile Platform Message-ID: <68d3bb7a-7572-7495-d295-e1d512ef509e@omp.ru> Date: Tue, 18 Jan 2022 23:21:45 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <20220117084732.cdy2sash5hxp4lwo@pengutronix.de> Content-Language: en-US X-Originating-IP: [192.168.11.198] X-ClientProxiedBy: LFEXT01.lancloud.ru (fd00:f066::141) To LFEX1907.lancloud.ru (fd00:f066::207) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220118_122200_863165_78C5137D X-CRM114-Status: GOOD ( 34.50 ) 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: text/plain; charset="windows-1252" Content-Transfer-Encoding: quoted-printable Sender: "linux-mtd" Errors-To: linux-mtd-bounces+linux-mtd=archiver.kernel.org@lists.infradead.org Hello! On 1/17/22 11:47 AM, Uwe Kleine-K=F6nig wrote: [...] >>>>>>>>> To me it sounds much more logical for the driver to check if an >>>>>>>>> optional irq is non-zero (available) or zero (not available), tha= n to >>>>>>>>> sprinkle around checks for -ENXIO. In addition, you have to remem= ber >>>>>>>>> that this one returns -ENXIO, while other APIs use -ENOENT or -EN= OSYS >>>>>>>>> (or some other error code) to indicate absence. I thought not hav= ing >>>>>>>>> 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 availabl= e. >>>>>> >>>>>> Hm, I've just looked at these and must note that they match 1:1 w= ith >>>>>> platform_get_irq_optional(). Unfortunately, we can't however behave = the >>>>>> same way in request_irq() -- because it has to support IRQ0 for the = sake >>>>>> of i8253 drivers in arch/... >>>>> >>>>> Let me reformulate your statement to the IMHO equivalent: >>>>> >>>>> If you set aside the differences between >>>>> platform_get_irq_optional() and gpiod_get_optional(), >>>> >>>> Sorry, I should make it clear this is actually the diff between a w= ould-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. >> >> I have noting to say here, rather than optional IRQ could well have a= different >> meaning than for clk/gpio/etc. >> >> [...] >>>>> However for an interupt this cannot work. You will always have to che= ck >>>>> if the irq is actually there or not because if it's not you cannot ju= st >>>>> ignore that. So there is no benefit of an optional irq. >>>>> >>>>> Leaving error message reporting aside, the introduction of >>>>> platform_get_irq_optional() allows to change >>>>> >>>>> irq =3D platform_get_irq(...); >>>>> if (irq < 0 && irq !=3D -ENXIO) { >>>>> return irq; >>>>> } else if (irq >=3D 0) { >>>> >>>> Rather (irq > 0) actually, IRQ0 is considered invalid (but still re= turned). >>> >>> 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 ... >> >> Note that we should absolutely (and first of all) stop returning 0 fr= om platform_get_irq() >> on a "real" IRQ0. Handling that "still good" zero absolutely doesn't sca= le e.g. for the subsystems >> (like libata) which take 0 as an indication that the polling mode should= be used... We can't afford >> to be sloppy here. ;-) > = > Then maybe do that really first? I'm doing it first already: https://lore.kernel.org/all/5e001ec1-d3f1-bcb8-7f30-a6301fd9930c@omp.ru/ This series is atop of the above patch... > I didn't recheck, but is this what the > driver changes in your patch is about? Partly, yes. We can afford to play with the meaning of 0 after the above= patch. > After some more thoughts I wonder if your focus isn't to align > platform_get_irq_optional to (clk|gpiod|regulator)_get_optional, but to > simplify return code checking. Because with your change we have: > = > - < 0 -> error > - =3D=3D 0 -> no irq > - > 0 -> irq Mainly, yes. That's why the code examples were given in the description. > For my part I'd say this doesn't justify the change, but at least I > could better life with the reasoning. If you start at: > = > irq =3D platform_get_irq_optional(...) > if (irq < 0 && irq !=3D -ENXIO) > return irq > else if (irq > 0) > setup_irq(irq); > else > setup_polling() > = > I'd change that to > = > irq =3D platform_get_irq_optional(...) > if (irq > 0) /* or >=3D 0 ? */ Not >=3D 0, no... > setup_irq(irq) > else if (irq =3D=3D -ENXIO) > setup_polling() > else > return irq > = > This still has to mention -ENXIO, but this is ok and checking for 0 just > hardcodes a different return value. I think comparing with 0 is simpler (and shorter) than with -ENXIO, if y= ou consider the RISC CPUs, like e.g. MIPS... > Anyhow, I think if you still want to change platform_get_irq_optional > you should add a few patches converting some drivers which demonstrates > the improvement for the callers. Mhm, I did include all the drivers where the IRQ checks have to be modif= ied, not sure what else you want me to touch... > Best regards > Uwe MBR, Sergey ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ 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 DCE40C433EF for ; Thu, 20 Jan 2022 07:10:37 +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 3C6C22FD4; Thu, 20 Jan 2022 08:09:46 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 3C6C22FD4 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1642662636; bh=dMhRyKSPUCNdg3u+bV3hM8ga01CpEbjLfEyrVM7LTY0=; h=Subject:To:References:From:Date:In-Reply-To:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=DuJdeuasTSgUTho3IBjuCCB0Silq3dMt4SZ3SGRK54wVpz5Y4vWA4g4e6V9tyyaN4 QcyBfwYZs6vWzIixJ2EZo7aAYpS46AVLlDsM1wjZSN/Qwb8IC0J+z+VnOo9er4Y5W0 0IcVNKN3dusXrEwcBMZKoAvyeObc/QnV/rMTqPww= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id EB5BCF805AB; Thu, 20 Jan 2022 08:03:49 +0100 (CET) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 0A122F80100; Tue, 18 Jan 2022 21:21:58 +0100 (CET) Received: from mxout02.lancloud.ru (mxout02.lancloud.ru [45.84.86.82]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 02B88F80100 for ; Tue, 18 Jan 2022 21:21:53 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz 02B88F80100 Received: from LanCloud DKIM-Filter: OpenDKIM Filter v2.11.0 mxout02.lancloud.ru 27A6E20B359D Received: from LanCloud Received: from LanCloud Received: from LanCloud Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional To: =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= References: <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> <20220114202226.ugzklxv4wzr6egwj@pengutronix.de> <57af1851-9341-985e-7b28-d2ba86770ecb@omp.ru> <20220117084732.cdy2sash5hxp4lwo@pengutronix.de> From: Sergey Shtylyov Organization: Open Mobile Platform Message-ID: <68d3bb7a-7572-7495-d295-e1d512ef509e@omp.ru> Date: Tue, 18 Jan 2022 23:21:45 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <20220117084732.cdy2sash5hxp4lwo@pengutronix.de> Content-Type: text/plain; charset="windows-1252" Content-Language: en-US Content-Transfer-Encoding: 8bit X-Originating-IP: [192.168.11.198] X-ClientProxiedBy: LFEXT01.lancloud.ru (fd00:f066::141) To LFEX1907.lancloud.ru (fd00:f066::207) 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 , Andy Shevchenko , 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 , Kishon Vijay Abraham I , Geert Uytterhoeven , "open list:SERIAL DRIVERS" , bcm-kernel-feedback-list , 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 , 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 , Hans de Goede , netdev@vger.kernel.org, Yoshihiro Shimoda , Cornelia Huck , Linux MMC List , Liam Girdwood , linux-spi , Linux-Renesas , Vinod Koul , James Morse , Zha Qipeng , Pengutronix Kernel Team , Richard Weinberger , =?UTF-8?Q?Niklas_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" Hello! On 1/17/22 11:47 AM, Uwe Kleine-König wrote: [...] >>>>>>>>> To me it sounds much more logical for the driver to check if an >>>>>>>>> optional irq is non-zero (available) or zero (not available), than to >>>>>>>>> sprinkle around checks for -ENXIO. In addition, you have to remember >>>>>>>>> that this one returns -ENXIO, while other APIs use -ENOENT or -ENOSYS >>>>>>>>> (or some other error code) to indicate absence. I thought not having >>>>>>>>> to care about the actual error code was the main reason behind the >>>>>>>>> introduction of the *_optional() APIs. >>>>>>> >>>>>>>> No, the main benefit of gpiod_get_optional() (and clk_get_optional()) is >>>>>>>> that you can handle an absent GPIO (or clk) as if it were available. >>>>>> >>>>>> 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 sake >>>>>> of i8253 drivers in arch/... >>>>> >>>>> Let me reformulate your statement to the IMHO equivalent: >>>>> >>>>> If you set aside the differences between >>>>> platform_get_irq_optional() and gpiod_get_optional(), >>>> >>>> Sorry, I should make it clear this is actually the diff between a would-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. >> >> I have noting to say here, rather than optional IRQ could well have a different >> meaning than for clk/gpio/etc. >> >> [...] >>>>> 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. >>>>> >>>>> Leaving error message reporting aside, the introduction of >>>>> platform_get_irq_optional() allows to change >>>>> >>>>> irq = platform_get_irq(...); >>>>> if (irq < 0 && irq != -ENXIO) { >>>>> return irq; >>>>> } else if (irq >= 0) { >>>> >>>> Rather (irq > 0) actually, IRQ0 is considered invalid (but still returned). >>> >>> 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 ... >> >> Note that we should absolutely (and first of all) stop returning 0 from platform_get_irq() >> on a "real" IRQ0. Handling that "still good" zero absolutely doesn't scale e.g. for the subsystems >> (like libata) which take 0 as an indication that the polling mode should be used... We can't afford >> to be sloppy here. ;-) > > Then maybe do that really first? I'm doing it first already: https://lore.kernel.org/all/5e001ec1-d3f1-bcb8-7f30-a6301fd9930c@omp.ru/ This series is atop of the above patch... > I didn't recheck, but is this what the > driver changes in your patch is about? Partly, yes. We can afford to play with the meaning of 0 after the above patch. > After some more thoughts I wonder if your focus isn't to align > platform_get_irq_optional to (clk|gpiod|regulator)_get_optional, but to > simplify return code checking. Because with your change we have: > > - < 0 -> error > - == 0 -> no irq > - > 0 -> irq Mainly, yes. That's why the code examples were given in the description. > For my part I'd say this doesn't justify the change, but at least I > could better life with the reasoning. If you start at: > > irq = platform_get_irq_optional(...) > if (irq < 0 && irq != -ENXIO) > return irq > else if (irq > 0) > setup_irq(irq); > else > setup_polling() > > I'd change that to > > irq = platform_get_irq_optional(...) > if (irq > 0) /* or >= 0 ? */ Not >= 0, no... > setup_irq(irq) > else if (irq == -ENXIO) > setup_polling() > else > return irq > > This still has to mention -ENXIO, but this is ok and checking for 0 just > hardcodes a different return value. I think comparing with 0 is simpler (and shorter) than with -ENXIO, if you consider the RISC CPUs, like e.g. MIPS... > Anyhow, I think if you still want to change platform_get_irq_optional > you should add a few patches converting some drivers which demonstrates > the improvement for the callers. Mhm, I did include all the drivers where the IRQ checks have to be modified, not sure what else you want me to touch... > Best regards > Uwe MBR, Sergey