Message-ID: <20220117092444.opoedfcf5k5u6otq@pengutronix.de>
Date:   Mon, 17 Jan 2022 10:24:44 +0100
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Sergey Shtylyov <s.shtylyov@....ru>, Andrew Lunn <andrew@...n.ch>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Vignesh Raghavendra <vigneshr@...com>,
        KVM list <kvm@...r.kernel.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>, linux-iio@...r.kernel.org,
        Linus Walleij <linus.walleij@...aro.org>,
        Amit Kucheria <amitk@...nel.org>,
        ALSA Development Mailing List <alsa-devel@...a-project.org>,
        Jaroslav Kysela <perex@...ex.cz>,
        Guenter Roeck <groeck@...omium.org>,
        Thierry Reding <thierry.reding@...il.com>,
        MTD Maling List <linux-mtd@...ts.infradead.org>,
        Linux I2C <linux-i2c@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        linux-phy@...ts.infradead.org, netdev@...r.kernel.org,
        linux-spi <linux-spi@...r.kernel.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        Khuong Dinh <khuong@...amperecomputing.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Matthias Schiffer <matthias.schiffer@...tq-group.com>,
        Kamal Dasu <kdasu.kdev@...il.com>,
        Lee Jones <lee.jones@...aro.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Kishon Vijay Abraham I <kishon@...com>,
        bcm-kernel-feedback-list <bcm-kernel-feedback-list@...adcom.com>,
        "open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Zhang Rui <rui.zhang@...el.com>,
        platform-driver-x86@...r.kernel.org,
        Linux PWM List <linux-pwm@...r.kernel.org>,
        Robert Richter <rric@...nel.org>,
        Saravanan Sekar <sravanhome@...il.com>,
        Corey Minyard <minyard@....org>,
        Linux PM list <linux-pm@...r.kernel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        John Garry <john.garry@...wei.com>,
        Peter Korsgaard <peter@...sgaard.com>,
        William Breathitt Gray <vilhelm.gray@...il.com>,
        Mark Gross <markgross@...nel.org>,
        Hans de Goede <hdegoede@...hat.com>,
        Alex Williamson <alex.williamson@...hat.com>,
        Mark Brown <broonie@...nel.org>,
        Borislav Petkov <bp@...en8.de>, Takashi Iwai <tiwai@...e.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        openipmi-developer@...ts.sourceforge.net,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Benson Leung <bleung@...omium.org>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        linux-edac@...r.kernel.org, Tony Luck <tony.luck@...el.com>,
        Richard Weinberger <richard@....at>,
        Mun Yew Tham <mun.yew.tham@...el.com>,
        Eric Auger <eric.auger@...hat.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
        Cornelia Huck <cohuck@...hat.com>,
        Linux MMC List <linux-mmc@...r.kernel.org>,
        Joakim Zhang <qiangqing.zhang@....com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
        Vinod Koul <vkoul@...nel.org>,
        James Morse <james.morse@....com>,
        Zha Qipeng <qipeng.zha@...el.com>,
        Sebastian Reichel <sre@...nel.org>,
        Niklas Söderlund <niklas.soderlund@...natech.se>,
        linux-mediatek@...ts.infradead.org,
        Brian Norris <computersforpeace@...il.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional

Hello Geert,

On Mon, Jan 17, 2022 at 09:41:42AM +0100, Geert Uytterhoeven wrote:
> On Sat, Jan 15, 2022 at 9:22 PM Sergey Shtylyov <s.shtylyov@....ru> wrote:
> > On 1/14/22 11:22 PM, Uwe Kleine-König wrote:
> > > You have to understand that for clk (and regulator and gpiod) NULL is a
> > > valid descriptor that can actually be used, it just has no effect. So
> > > this is a convenience value for the case "If the clk/regulator/gpiod in
> > > question isn't available, there is nothing to do". This is what makes
> > > clk_get_optional() and the others really useful and justifies their
> > > existence. This doesn't apply to platform_get_irq_optional().
> >
> >    I do understand that. However, IRQs are a different beast with their
> > own justifications...
> 
> > > clk_get_optional() is sane and sensible for cases where the clk might be
> > > absent and it helps you because you don't have to differentiate between
> > > "not found" and "there is an actual resource".
> > >
> > > The reason for platform_get_irq_optional()'s existence is just that
> > > platform_get_irq() emits an error message which is wrong or suboptimal
> >
> >    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.

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.

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.

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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists