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

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 7563 bytes --]

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

> > 		... 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
just because platform_get_irq() is to hard to change.

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

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

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.

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
in some cases (and IMHO is platform_get_irq() root fault). It doesn't
simplify handling the "not found" case. 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().

And as you cannot change platform_get_irq_optional() to return a working
dummy value, IMHO the only sane way out is renaming it.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]