On Mon, Dec 13, 2021 at 07:39:31AM +0900, Damien Le Moal wrote:

On 2021/12/11 19:25, Sergey Shtylyov wrote:

quoted

On 11.12.2021 2:45, Damien Le Moal wrote:

...

quoted

quoted

quoted

quoted

So 0 will be returned as-is. That is rather weird. That should be fixed to
return -ENXIO:

	if (WARN(ret == 0, "0 is an invalid IRQ number\n"))
		return -ENXIO;
	return ret;

    My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this
but returns -EINVAL instead.

Thinking more about this, shouldn't this change go into platform_get_irq()
instead of platform_get_irq_optional() ?

    Why? platform_get_irq() currently just calls platform_get_irq_optional()...

quoted

The way I see it, I think that the intended behavior for
platform_get_irq_optional() is:
1) If have IRQ, return it, always > 0
2) If no IRQ, return 0

    That does include the IRQ0 case, right?

IRQ 0 being invalid, I think that case should be dealt with internally within
platform_get_irq_optional() and warn/error return. IRQ 0 showing up would thus
be case (3), an error.

quoted

quoted

3) If error, return < 0
no ?

   I completely agree, I (after thinking a bit) have no issues with that...

quoted

And for platform_get_irq(), case (2) becomes an error.
Is this the intended semantic ?

    I don't see how it's different from the current behavior. But we can do 
that as well, I just don't see whether it's really better...

The problem I see is that the current behavior is unclear: what does
platform_get_irq_optional() returning 0 mean ? IRQ == 0 ? or "no IRQ" ? I think
it should be the latter rather than the former. Note that the function could
return ENOENT (or similar) for the "no IRQ" case. With that, case (2) goes away,
but then I do not see any difference between platform_get_irq_optional() and
platform_get_irq().

If the preferred API semantic is to allow returning IRQ 0 with a warning, then
the kdoc comments of platform_get_irq_optional() and platform_get_irq() are
totally broken, and the code for many drivers is probably wrong too.

Yeah, what we need to do is that (roughly a roadmap):
 - revisit callers of platform_get_irq_optional() to be prepared for
   new behaviour
 - rewrite platform_get_irq() to return -ENOENT
 - rewrite platform_get_irq_optional() to return 0 on -ENOENT

This is how other similar (i.e. _optional) APIs do.

-- 
With Best Regards,
Andy Shevchenko