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 error
>>> else:
>>> 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
