Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional

[Geert Uytterhoeven]

Hi Uwe,

On Mon, Jan 17, 2022 at 10:24 AM Uwe Kleine-König
<u.kleine-koenig@xxxxxxxxxxxxxx> 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 <s.shtylyov@xxxxxx> 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.

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.  But as we were all in a hurry to kill
the non-applicable error message, we went for the quick and dirty fix.

> 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.

That's exactly why we want to change the latter to return 0 ;-)

> 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.

It is for devices that can have either separate interrupts, or a single
multiplexed interrupt.

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.

> 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.

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.
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.

Commit 8973ea47901c81a1 ("driver core: platform: Introduce
platform_get_irq_optional()") fixed (b), but didn't address (a) and
(c).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds