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

[Sergey Shtylyov]

Hello!

On 1/24/22 6:01 PM, Andy Shevchenko wrote:

>>>>>>> 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.
>>>>>
>>>>> And similar number of people are on the other side.
>>>>
>>>> If someone already opposed to the renaming (and not only the name) I
>>>> must have missed that.
>>>>
>>>> So you think it's a good idea to keep the name
>>>> platform_get_irq_optional() despite the "not found" value returned by it
>>>> isn't usable as if it were a normal irq number?
>>>
>>> I meant that on the other side people who are in favour of Sergey's patch.
>>> Since that I commented already that I opposed the renaming being a standalone
>>> change.
>>>
>>> Do you agree that we have several issues with platform_get_irq*() APIs?
[...]
>>> 2. The vIRQ0 handling: a) WARN() followed by b) returned value 0
>>
>> I'm happy with the vIRQ0 handling. Today platform_get_irq() and it's
>> silent variant returns either a valid and usuable irq number or a
>> negative error value. That's totally fine.
> 
> It might return 0.
> Actually it seems that the WARN() can only be issued in two cases:
> - SPARC with vIRQ0 in one of the array member
> - fallback to ACPI for GPIO IRQ resource with index 0

   You have probably missed the recent discovery that arch/sh/boards/board-aps4*.c
causes IRQ0 to be passed as a direct IRQ resource?

> But the latter is bogus, because it would mean a bug in the ACPI code.

   Worth changing >= 0 to > 0 there, maybe?

> The bottom line here is the SPARC case. Anybody familiar with the platform
> can shed a light on this. If there is no such case, we may remove warning
> along with ret = 0 case from platfrom_get_irq().

   I'm afraid you're too fast here... :-)
   We'll have a really hard time if we continue to allow IRQ0 to be returned by
platform_get_irq() -- we'll have oto fileter it out in the callers then...

>>> 3. The specific cookie for "IRQ not found, while no error happened" case
>>
>> Not sure what you mean here. I have no problem that a situation I can
>> cope with is called an error for the query function. I just do error
>> handling and continue happily. So the part "while no error happened" is
>> irrelevant to me.
> 
> I meant that instead of using special error code, 0 is very much good for
> the cases when IRQ is not found. It allows to distinguish -ENXIO from the
> low layer from -ENXIO with this magic meaning.

   I don't see how -ENXIO can trickle from the lower layers, frankly...

[...]

MBR, Sergey