On 1/14/22 11:22 PM, Uwe Kleine-König wrote:

On Fri, Jan 14, 2022 at 10:14:10PM +0300, Sergey Shtylyov wrote:

quoted

On 1/14/22 12:25 PM, Uwe Kleine-König wrote:

quoted

quoted

quoted

quoted

quoted

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.

quoted

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.

quoted

quoted

	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.

quoted

quoted

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

   See below. :-)

quoted

quoted

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

   They would really be the same function if we do that. But...

just because platform_get_irq() is to hard to change.

   It's not just that, of course. If you make platform_get_irq() return 0
ISO -ENXIO, you'd have to add the handling of that 0 to all the callers,
and that won't be as simple as:

	if (irq < 0)
		return irq;

since we can't just propagate 0 upstream, we'd have to return something like
-ENXIO (or whatever error we see fit). Does that really scale well?

quoted

Anyway, we should 1st stop returning 0 for "valid" IRQs -- this is done by my patch
the discussed patch series are atop of.

quoted

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

   There's no win at all, it seems.

clk_get() and clk_get_optional().

quoted

quoted

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.

   Unfortunately, we don't have a single layer in case of IRQs... There's
no platform_request_irq() (yet? :-)).

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

   I do understand that. However, IRQs are a different beast with their
own justifications...

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.

   I haven't been thinking about the IRQ error messages at all (yet?)...
And when I start thinking about it, it doesn't seem that bad, perhaps
even saves a lot of the .rodata section... :-)

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

   I think you are very wrong here. The real reason is to simplify the
callers.

in some cases (and IMHO is platform_get_irq() root fault). It doesn't
simplify handling the "not found" case.

   Oh, it does... you don't have to special-case 0 when handling its result.
In my book, it's a major simplification.

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

   OK, no similarity. But that's well justified.

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

   Your rename really focused on the wrong aspect of the function, I think...

Best regards
Uwe

MBR, Sergey