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

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

	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?

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

		... 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?
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 ;-)).

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?

Now I'm unwilling to continue the discussion unless there pops up a
suggestion that results in a considerable part (say > 10%) of the
drivers using platform_get_irq_optional not having to check if the
return value corresponds to "not found".

   Note that many actual drivers don't follow the pattern prescribed in the comment to
platform_get_irq_optional() and their check for an optional IRQ look like irq < 0
(and, after my patches, irq <= 0). Maybe we shouldn't even bother returning the error
codes and just return 0 for all kinds of misfortunes instead? :-)

Best regards
Uwe

MBR, Sergey

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek