Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional

[Sergey Shtylyov <s.shtylyov@xxxxxx>]

Hello!

On 1/14/22 11:22 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.

   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. ;-)

[...]

> Best regards
> Uwe

MBR, Sergey