Re: [PATCH] driver core: platform: Rename platform_get_irq_optional() to platform_get_irq_silent()

[Sergey Shtylyov]

On 1/15/22 6:21 PM, 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.
>>>>
>>>>    This is only a current difference but I'm still going to return 0 ISO
>>>> -ENXIO from latform_get_irq_optional(), no way I'd leave that -ENXIO there
>>>> alone... :-)
>>>
>>> This would address a bit of the critic in my commit log. But as 0 isn't
>>> a dummy value like the dummy values that exist for clk, gpiod and
>>> regulator I still think that the naming is a bad idea because it's not
>>> in the spirit of the other *_get_optional functions.
>>>
>>> Seeing you say that -ENXIO is a bad return value for
>>> platform_get_irq_optional() and 0 should be used instead, I wonder why
>>> not changing platform_get_irq() to return 0 instead of -ENXIO, too.
>>> This question is for now only about a sensible semantic. That actually
>>> changing platform_get_irq() is probably harder than changing
>>> platform_get_irq_optional() is a different story.
>>>
>>> If only platform_get_irq_optional() is changed and given that the
>>> callers have to do something like:
>>>
>>>         if (this_irq_exists()):
>>>                 ... (e.g. request_irq)
>>>         else:
>>>                 ... (e.g. setup polling)
>>>
>>> I really think it's a bad idea that this_irq_exists() has to be
>>> different for platform_get_irq() vs. platform_get_irq_optional().
>>
>> For platform_get_irq(), the IRQ being absent is an error condition,
>> hence it should return an error code.
>> For platform_get_irq_optional(), the IRQ being absent is not an error
>> condition, hence it should not return an error code, and 0 is OK.
> 
> Please show a few examples how this simplifies the code. If it's only

   As for platform_get_irq(), returning -ENXIO simplifies things a lot: you don't
have to check for 0 at every freaking call site and have s/th like (every
time!):

	irq = platform_get_irq();
	if (irq <= 0)
		return irq ?: -ENXIO; // any error code you choose

instead of just:

	irq = platform_get_irq();
	if (irq < 0)
		return irq;

This scales better in my book.

> that a driver has to check for == 0 instead of == -ENXIO, than that's
> not a good enough motivation to make platform_get_irq_optional()
> different to platform_get_irq().

   Again, it scales better... good motivation in my eyes.

> Best regards
> Uwe

MBR, Sergey