On Wed, Jan 12, 2022 at 11:27:02AM +0100, Geert Uytterhoeven wrote:

Hi Uwe,

On Wed, Jan 12, 2022 at 9:51 AM Uwe Kleine-König
[off-list ref] wrote:

quoted

On Wed, Jan 12, 2022 at 09:33:48AM +0100, Geert Uytterhoeven wrote:

quoted

On Mon, Jan 10, 2022 at 10:20 PM Andrew Lunn [off-list ref] wrote:

quoted

On Mon, Jan 10, 2022 at 09:10:14PM +0100, Uwe Kleine-König wrote:

quoted

On Mon, Jan 10, 2022 at 10:54:48PM +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...

The difference to gpiod_get_optional (and most other *_optional) is that
you can use the NULL value as if it were a valid GPIO.

As this isn't given with for irqs, I don't think changing the return
value has much sense.

We actually want platform_get_irq_optional() to look different to all
the other _optional() methods because it is not equivalent. If it
looks the same, developers will assume it is the same, and get
themselves into trouble.

Developers already assume it is the same, and thus forget they have
to check against -ENXIO instead of zero.

I agree that -ENXIO is unfortunate and -ENOENT would be more in line
with other functions. I assume it's insane to want to change that.

quoted

Is this an ack for renaming platform_get_irq_optional() to
platform_get_irq_silent()?

No it isn't ;-)

If an optional IRQ is not present, drivers either just ignore it (e.g.
for devices that can have multiple interrupts or a single muxed IRQ),
or they have to resort to polling. For the latter, fall-back handling
is needed elsewhere in the driver.

I think irq are not suitable for such a dummy handling. For clocks or
GPIOs there are cases where just doing nothing in the absence of a
certain optional clock or GPIO is fine.

I checked a few users of platform_get_irq_optional() and I didn't find a
single one that doesn't need to differentiate the irq and the no-irq
case later. Do you know one? If you do, isn't that so exceptional that
it doesn't justify the idea of a dummy irq value? So until proven
otherwise I think platform_get_irq_optional() just isn't in the spirit
of clk_get_optional() and gpiod_get_optional() because there are no use
cases where a dummy value would be good enough. (Even if request_irq
would be a noop for a dummy irq value.)

The motivation why platform_get_irq_optional() was introduced was just
that platform_get_irq() started to emit an error message (in commit
7723f4c5ecdb8d832f049f8483beb0d1081cedf6) and the (proportional) few
drivers where the error message was bad needed a variant that doesn't
emit the error message. Look at
31a8d8fa84c51d3ab00bf059158d5de6178cf890, the motivation to use
platform_get_irq_optional() wasn't that it simplifies handling in the
driver, but that it doesn't emit an error message. Or
8f5783ad9eb83747471f61f94dbe209fb9fb8a7d, or
2fd276c3ee4bd42eb034f8954964a5ae74187c6b, or
55cc33fab5ac9f7e2a97aa7c564e8b35355886d5. Just look at the output of git
log -Splatform_get_irq_optional to find some more.

That convinces me, that platform_get_irq_optional() is a bad name. The
only difference to platform_get_irq is that it's silent. And returning
a dummy irq value (which would make it aligned with the other _optional
functions) isn't possible.

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.

Best regards
Uwe

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