Hi Uwe,
On Tue, Jan 18, 2022 at 1:08 PM Uwe Kleine-König u.kleine-koenig@pengutronix.de wrote:
On Tue, Jan 18, 2022 at 10:37:25AM +0100, Geert Uytterhoeven wrote:
On Tue, Jan 18, 2022 at 10:09 AM Uwe Kleine-König u.kleine-koenig@pengutronix.de wrote:
For the (clk|gpiod|regulator)_get_optional() you don't have to check against the magic not-found value (so no implementation detail magic leaks into the caller code) and just pass it to the next API function. (And my expectation would be that if you chose to represent not-found by (void *)66 instead of NULL, you won't have to adapt any user, just the framework internal checks. This is a good thing!)
Ah, there is the wrong assumption: drivers sometimes do need to know if the resource was found, and thus do need to know about (void *)66, -ENODEV, or -ENXIO. I already gave examples for IRQ and clk before. I can imagine these exist for gpiod and regulator, too, as soon as you go beyond the trivial "enable" and "disable" use-cases.
My premise is that every user who has to check for "not found" explicitly should not use (clk|gpiod)_get_optional() but (clk|gpiod)_get() and do proper (and explicit) error handling for -ENODEV. (clk|gpiod)_get_optional() is only for these trivial use-cases.
And 0/NULL vs. > 0 is the natural check here: missing, but not an error.
For me it it 100% irrelevant if "not found" is an error for the query function or not. I just have to be able to check for "not found" and react accordingly.
And adding a function
def platform_get_irq_opional(): ret = platform_get_irq() if ret == -ENXIO: return 0 return retit's not a useful addition to the API if I cannot use 0 as a dummy because it doesn't simplify the caller enough to justify the additional function.
The only thing I need to be able is to distinguish the cases "there is an irq", "there is no irq" and anything else is "there is a problem I cannot handle and so forward it to my caller". The semantic of platform_get_irq() is able to satisfy this requirement[1], so why introduce platform_get_irq_opional() for the small advantage that I can check for not-found using
if (!irq)instead of
if (irq != -ENXIO)? The semantic of platform_get_irq() is easier ("Either a usable non-negative irq number or a negative error number") compared to platform_get_irq_optional() ("Either a usable positive irq number or a negative error number or 0 meaning not found"). Usage of platform_get_irq() isn't harder or more expensive (neither for a human reader nor for a maching running the resulting compiled code). For a human reader
if (irq != -ENXIO)is even easier to understand because for
if (!irq)they have to check where the value comes from, see it's platform_get_irq_optional() and understand that 0 means not-found.
"vIRQ zero does not exist."
This function just adds overhead because as a irq framework user I have to understand another function. For me the added benefit is too small to justify the additional function. And you break out-of-tree drivers. These are all no major counter arguments, but as the advantage isn't major either, they still matter.
Best regards Uwe
[1] the only annoying thing is the error message.
So there's still a need for two functions.
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