On Thu, Jan 13, 2022 at 01:42:57PM -0800, Florian Fainelli wrote:
On 1/13/2022 11:43 AM, Uwe Kleine-König wrote:
The subsystems regulator, clk and gpio have the concept of a dummy resource. For regulator, clk and gpio there is a semantic difference between the regular _get() function and the _get_optional() variant. (One might return the dummy resource, the other won't. Unfortunately which one implements which isn't the same for these three.) The difference between platform_get_irq() and platform_get_irq_optional() is only that the former might emit an error message and the later won't.
To prevent people's expectations that there is a semantic difference between these too, rename platform_get_irq_optional() to platform_get_irq_silent() to make the actual difference more obvious.
The #define for the old name can and should be removed once all patches currently in flux still relying on platform_get_irq_optional() are fixed.
Signed-off-by: Uwe Kleine-König u.kleine-koenig@pengutronix.de
Hello,
On Thu, Jan 13, 2022 at 02:45:30PM +0000, Mark Brown wrote:
On Thu, Jan 13, 2022 at 12:08:31PM +0100, Uwe Kleine-König wrote:
This is all very unfortunate. In my eyes b) is the most sensible sense, but the past showed that we don't agree here. (The most annoying part of regulator_get is the warning that is emitted that regularily makes customers ask what happens here and if this is fixable.)
Fortunately it can be fixed, and it's safer to clearly specify things. The prints are there because when the description is wrong enough to cause things to blow up we can fail to boot or run messily and forgetting to describe some supplies (or typoing so they haven't done that) and people were having a hard time figuring out what might've happened.
Yes, that's right. I sent a patch for such a warning in 2019 and pinged occationally. Still waiting for it to be merged :-\ (https://lore.kernel.org/r/20190625100412.11815-1-u.kleine-koenig@pengutronix...)
I think at least c) is easy to resolve because platform_get_irq_optional() isn't that old yet and mechanically replacing it by platform_get_irq_silent() should be easy and safe. And this is orthogonal to the discussion if -ENOXIO is a sensible return value and if it's as easy as it could be to work with errors on irq lookups.
It'd certainly be good to name anything that doesn't correspond to one of the existing semantics for the API (!) something different rather than adding yet another potentially overloaded meaning.
It seems we're (at least) three who agree about this. Here is a patch fixing the name.
From an API naming perspective this does not make much sense anymore with the name chosen, it is understood that whent he function is called platform_get_irq_optional(), optional applies to the IRQ. An optional IRQ is something people can reason about because it makes sense.
The problem I see is that the semantic is different to the other available *_get_optional() functions. And this isn't fixable for irqs. So better don't use that naming scheme.
What is a a "silent" IRQ however? It does not apply to the object it is trying to fetch to anymore, but to the message that may not be printed in case the resource failed to be obtained, because said resource is optional. Woah, that's quite a stretch.
I'm surprised that the semantic isn't obvious, but ok, I can accept that.Can you maek a constructive suggestion here? What name pair would you choose for the two functions functions under discussion?
(BTW, my favourite would be that platform_get_irq() doesn't emit an error message and the caller is reliable for emitting that. But I think it's too late for this approach.)
Following the discussion and original 2 patches set from Sergey, it is not entirely clear to me anymore what is it that we are trying to fix.
(I think) Sergey's focus is:
platform_get_irq_optional() returning -ENXIO is ugly, other functions return -ENOENT and other *_get_optional() functions return NULL for "This resource doesn't exist". So let's return 0 in this case.
My focus is:
There cannot be an "optional irq" where the driver doesn't have to care if the irq actually exist or not. So the pattern with *_get_optional() isn't sensible for irqs and if changing the returned value meaning "This resource doesn't exist" is sensible for platform_get_irq_optional(), I claim it's sensible for platform_get_irq(), too.
So the semantic difference between platform_get_irq() and platform_get_irq_optional() is only that one emits an error message and the other doesn't. So this function pair should use a different naming than get + get_optional as this naming evokes expectations that must be wrong as there cannot be a dummy irq value.
I nearly forgot, I would paint it blue, sky blue, not navy blue, not light blue ;)
no way. green is the ultimate blue for platform_get_irq() :-)
Best regards Uwe