Hello!
On 1/17/22 11:47 AM, Uwe Kleine-König wrote:
[...]
>>> 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.
Hm, I've just looked at these and must note that they match 1:1 with platform_get_irq_optional(). Unfortunately, we can't however behave the same way in request_irq() -- because it has to support IRQ0 for the sake of i8253 drivers in arch/...
Let me reformulate your statement to the IMHO equivalent:
If you set aside the differences between platform_get_irq_optional() and gpiod_get_optional(),
Sorry, I should make it clear this is actually the diff between a would-be platform_get_irq_optional() after my patch, not the current code...
The similarity is that with your patch both gpiod_get_optional() and platform_get_irq_optional() return NULL and 0 on not-found. The relevant difference however is that for a gpiod NULL is a dummy value, while for irqs it's not. So the similarity is only syntactically, but not semantically.
I have noting to say here, rather than optional IRQ could well have a different meaning than for clk/gpio/etc.
[...]
However for an interupt this cannot work. You will always have to check if the irq is actually there or not because if it's not you cannot just ignore that. So there is no benefit of an optional irq.
Leaving error message reporting aside, the introduction of platform_get_irq_optional() allows to change
irq = platform_get_irq(...); if (irq < 0 && irq != -ENXIO) { return irq; } else if (irq >= 0) {
Rather (irq > 0) actually, IRQ0 is considered invalid (but still returned).
This is a topic I don't feel strong for, so I'm sloppy here. If changing this is all that is needed to convince you of my point ...
Note that we should absolutely (and first of all) stop returning 0 from platform_get_irq() on a "real" IRQ0. Handling that "still good" zero absolutely doesn't scale e.g. for the subsystems (like libata) which take 0 as an indication that the polling mode should be used... We can't afford to be sloppy here. ;-)
Then maybe do that really first?
I'm doing it first already:
https://lore.kernel.org/all/5e001ec1-d3f1-bcb8-7f30-a6301fd9930c@omp.ru/
This series is atop of the above patch...
I didn't recheck, but is this what the driver changes in your patch is about?
Partly, yes. We can afford to play with the meaning of 0 after the above patch.
After some more thoughts I wonder if your focus isn't to align platform_get_irq_optional to (clk|gpiod|regulator)_get_optional, but to simplify return code checking. Because with your change we have:
- < 0 -> error
- == 0 -> no irq
Mainly, yes. That's why the code examples were given in the description.
For my part I'd say this doesn't justify the change, but at least I could better life with the reasoning. If you start at:
irq = platform_get_irq_optional(...) if (irq < 0 && irq != -ENXIO) return irq else if (irq > 0) setup_irq(irq); else setup_polling()
I'd change that to
irq = platform_get_irq_optional(...) if (irq > 0) /* or >= 0 ? */
Not >= 0, no...
setup_irq(irq)else if (irq == -ENXIO) setup_polling() else return irq
This still has to mention -ENXIO, but this is ok and checking for 0 just hardcodes a different return value.
I think comparing with 0 is simpler (and shorter) than with -ENXIO, if you consider the RISC CPUs, like e.g. MIPS...
Anyhow, I think if you still want to change platform_get_irq_optional you should add a few patches converting some drivers which demonstrates the improvement for the callers.
Mhm, I did include all the drivers where the IRQ checks have to be modified, not sure what else you want me to touch...
Best regards Uwe
MBR, Sergey