Hi Uwe,

On Mon, Feb 14, 2022 at 8:29 AM Uwe Kleine-König
[off-list ref] wrote:

On Sat, Feb 12, 2022 at 11:16:30PM +0300, Sergey Shtylyov wrote:

quoted

This patch is based on the former Andy Shevchenko's patch:

https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/ (local)

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 <redacted>
Signed-off-by: Sergey Shtylyov <redacted>

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@linux-m68k.org

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