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 u.kleine-koenig@pengutronix.de wrote:
On Wed, Jan 12, 2022 at 09:33:48AM +0100, Geert Uytterhoeven wrote:
On Mon, Jan 10, 2022 at 10:20 PM Andrew Lunn andrew@lunn.ch wrote:
On Mon, Jan 10, 2022 at 09:10:14PM +0100, Uwe Kleine-König wrote:
On Mon, Jan 10, 2022 at 10:54:48PM +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@linux....
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.
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