On 1/14/22 11:22 PM, Uwe Kleine-König wrote:
On Fri, Jan 14, 2022 at 10:14:10PM +0300, Sergey Shtylyov wrote:
On 1/14/22 12:25 PM, 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.
platform_get_irq_optional() is like gpiod_get_optional().
The introduction of gpiod_get_optional() made it possible to simplify the following code:
reset_gpio = gpiod_get(...) if IS_ERR(reset_gpio): error = PTR_ERR(reset_gpio) if error != -ENDEV:
ENODEV?
Yes, typo.
return errorelse: gpiod_set_direction(reset_gpiod, INACTIVE)
to
reset_gpio = gpiod_get_optional(....) if IS_ERR(reset_gpio): return reset_gpio gpiod_set_direction(reset_gpiod, INACTIVE)
and I never need to actually know if the reset_gpio actually exists. Either the line is put into its inactive state, or it doesn't exist and then gpiod_set_direction is a noop. For a regulator or a clk this works in a similar way.
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 ...
See below. :-)
... setup irq operation ...} else { /* irq == -ENXIO */ ... setup polling ... }
to
irq = platform_get_irq_optional(...); if (irq < 0 && irq != -ENXIO) { return irq; } else if (irq >= 0) { ... setup irq operation ... } else { /* irq == -ENXIO */ ... setup polling ... }
which isn't a win. When changing the return value as you suggest, it can be changed instead to:
irq = platform_get_irq_optional(...); if (irq < 0) { return irq; } else if (irq > 0) { ... setup irq operation ... } else { /* irq == 0 */ ... setup polling ... }
which is a tad nicer. If that is your goal however I ask you to also change the semantic of platform_get_irq() to return 0 on "not found".
Well, I'm not totally opposed to that... but would there be a considerable win?
Well, compared to your suggestion of making platform_get_irq_optional() return 0 on "not-found" the considerable win would be that platform_get_irq_optional() and platform_get_irq() are not different
They would really be the same function if we do that. But...
just because platform_get_irq() is to hard to change.
It's not just that, of course. If you make platform_get_irq() return 0 ISO -ENXIO, you'd have to add the handling of that 0 to all the callers, and that won't be as simple as:
if (irq < 0) return irq;
since we can't just propagate 0 upstream, we'd have to return something like -ENXIO (or whatever error we see fit). Does that really scale well?
Anyway, we should 1st stop returning 0 for "valid" IRQs -- this is done by my patch the discussed patch series are atop of.
Note the win is considerably less compared to gpiod_get_optional vs
If there's any at all... We'd basically have to touch /all/ platform_get_irq() calls (and get an even larger CC list ;-)).
You got me wrong here. I meant that even if you change both platform_get_irq() and platform_get_irq_optional() to return 0 on "not-found", the win is small compared to the benefit of having both
There's no win at all, it seems.
clk_get() and clk_get_optional().
gpiod_get however. And then it still lacks the semantic of a dummy irq which IMHO forfeits the right to call it ..._optional().
Not quite grasping it... Why e.g. clk_get() doesn't return 0 for a not found clock?
Because NULL is not an error value for clk and when calling clk_get() you want a failure when the clk you asked for isn't available.
Sure you could do the following in a case where you want to insist the clk to be actually available:
clk = clk_get_optional(...) if (IS_ERR_OR_NULL(clk)) { err = PTR_ERR(clk) || -ENODEV; return dev_err_probe(dev, err, ....); }
but this is more ugly than
clk = clk_get(...) if (IS_ERR(clk)) { err = PTR_ERR(clk); return dev_err_probe(dev, err, ....); }
Additionally the first usage would hard-code in the drivers that NULL is the dummy value which you might want to consider a layer violation.
Unfortunately, we don't have a single layer in case of IRQs... There's no platform_request_irq() (yet? :-)).
You have to understand that for clk (and regulator and gpiod) NULL is a valid descriptor that can actually be used, it just has no effect. So this is a convenience value for the case "If the clk/regulator/gpiod in question isn't available, there is nothing to do". This is what makes clk_get_optional() and the others really useful and justifies their existence. This doesn't apply to platform_get_irq_optional().
I do understand that. However, IRQs are a different beast with their own justifications...
So clk_get() is sane and sensible for cases where you need the clk to be there. It doesn't emit an error message, because the caller knows better if it's worth an error message and in some cases the caller can also emit a better error message than clk_get() itself.
I haven't been thinking about the IRQ error messages at all (yet?)... And when I start thinking about it, it doesn't seem that bad, perhaps even saves a lot of the .rodata section... :-)
clk_get_optional() is sane and sensible for cases where the clk might be absent and it helps you because you don't have to differentiate between "not found" and "there is an actual resource".
The reason for platform_get_irq_optional()'s existence is just that platform_get_irq() emits an error message which is wrong or suboptimal
I think you are very wrong here. The real reason is to simplify the callers.
in some cases (and IMHO is platform_get_irq() root fault). It doesn't simplify handling the "not found" case.
Oh, it does... you don't have to special-case 0 when handling its result. In my book, it's a major simplification.
So let's not pretend by the choice of function names that there is a similarity between clk_get() + clk_get_optional() and platform_get_irq() + platform_get_irq_optional().
OK, no similarity. But that's well justified.
And as you cannot change platform_get_irq_optional() to return a working dummy value, IMHO the only sane way out is renaming it.
Your rename really focused on the wrong aspect of the function, I think...
Best regards Uwe
MBR, Sergey