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

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Uwe,

On Mon, Feb 14, 2022 at 8:29 AM Uwe Kleine-König
<u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> On Sat, Feb 12, 2022 at 11:16:30PM +0300, Sergey Shtylyov wrote:
> > This patch is based on the former Andy Shevchenko's patch:
> >
> > https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@xxxxxxxxxxxxxxx/
> >
> > Currently platform_get_irq_optional() returns an error code even if IRQ
> > resource simply has not been found.  It prevents the callers from being
> > error code agnostic in their error handling:
> >
> >       ret = platform_get_irq_optional(...);
> >       if (ret < 0 && ret != -ENXIO)
> >               return ret; // respect deferred probe
> >       if (ret > 0)
> >               ...we get an IRQ...
> >
> > All other *_optional() APIs seem to return 0 or NULL in case an optional
> > resource is not available.  Let's follow this good example, so that the
> > callers would look like:
> >
> >       ret = platform_get_irq_optional(...);
> >       if (ret < 0)
> >               return ret;
> >       if (ret > 0)
> >               ...we get an IRQ...
> >
> > Reported-by: Matthias Schiffer <matthias.schiffer@xxxxxxxxxxxxxxx>
> > Signed-off-by: Sergey Shtylyov <s.shtylyov@xxxxxx>
>
> While this patch is better than v1, I still don't like it for the
> reasons discussed for v1. (i.e. 0 isn't usable as a dummy value which I
> consider the real advantage for the other _get_optional() functions.)

IMHO the real advantage is the simplified error handling, which is the
area where most of the current bugs are. So I applaud the core change.

Also IMHO, the dummy value handling is a red herring.  Contrary to
optional clocks and resets, a missing optional interrupt does not
always mean there is nothing to do: in case of polling, something
else must definitely be done.  So even if request_irq() would accept
a dummy interrupt zero and just do nothing, it would give the false
impression that that is all there is to do, while an actual check
for zero with polling code handling may still need to be present,
thus leading to more not less bugs.

> Apart from that, I think the subject is badly chosen. With "Make
> somefunc() optional" I would expect that you introduce a Kconfig symbol
> that results in the function not being available when disabled.

Agreed.

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