On Mon, Jan 17, 2022 at 02:08:19PM +0100, Geert Uytterhoeven wrote:

Hi Uwe,

On Mon, Jan 17, 2022 at 12:49 PM Uwe Kleine-König
[off-list ref] wrote:

quoted

On Mon, Jan 17, 2022 at 11:35:52AM +0100, Geert Uytterhoeven wrote:

quoted

On Mon, Jan 17, 2022 at 10:24 AM Uwe Kleine-König
[off-list ref] wrote:

quoted

On Mon, Jan 17, 2022 at 09:41:42AM +0100, Geert Uytterhoeven wrote:

quoted

On Sat, Jan 15, 2022 at 9:22 PM Sergey Shtylyov [off-list ref] wrote:

quoted

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

quoted

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

quoted

quoted

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.

Indeed.

The commit that introduced platform_get_irq_optional() said:

        Introduce a new platform_get_irq_optional() that works much like
        platform_get_irq() but does not output an error on failure to
        find the interrupt.

So the author of 8973ea47901c81a1912bd05f1577bed9b5b52506 failed to
mention the real reason? Or look at
31a8d8fa84c51d3ab00bf059158d5de6178cf890:

        [...] use platform_get_irq_optional() to get second/third IRQ
        which are optional to avoid below error message during probe:
        [...]

Look through the output of

        git log -Splatform_get_irq_optional

to find several more of these.

Commit 8973ea47901c81a1 ("driver core: platform: Introduce
platform_get_irq_optional()") and the various fixups fixed the ugly
printing of error messages that were not applicable.
In hindsight, probably commit 7723f4c5ecdb8d83 ("driver core:
platform: Add an error message to platform_get_irq*()") should have
been reverted instead, until a platform_get_irq_optional() with proper
semantics was introduced.

ack.

quoted

But as we were all in a hurry to kill the non-applicable error
message, we went for the quick and dirty fix.

quoted

Also I fail to see how a caller of (today's) platform_get_irq_optional()
is simpler than a caller of platform_get_irq() given that there is no
semantic difference between the two. Please show me a single
conversion from platform_get_irq to platform_get_irq_optional that
yielded a simplification.

That's exactly why we want to change the latter to return 0 ;-)

OK. So you agree to my statement "The reason for
platform_get_irq_optional()'s existence is just that platform_get_irq()
emits an error message [...]". Actually you don't want to oppose but
say: It's unfortunate that the silent variant of platform_get_irq() took
the obvious name of a function that could have an improved return code
semantic.

So my suggestion to rename todays platform_get_irq_optional() to
platform_get_irq_silently() and then introducing
platform_get_irq_optional() with your suggested semantic seems
intriguing and straigt forward to me.

I don't really see the point of needing platform_get_irq_silently(),
unless as an intermediary step, where it's going to be removed again
once the conversion has completed.

We agree that one of the two functions is enough, just differ in which
of the two we want to have. :-)

If you think platform_get_irq_silently() is a good intermediate step for
your goal, then we agree to rename platform_get_irq_optional(). So I
suggest you ack my patch.

Still, the rename would touch all users at once anyway.

It would be more easy to keep the conversion regression-free however. A
plain rename is simple to verify. And then converting to the new
platform_get_irq_optional() can be done individually and without the
need to do everything in a single step.

quoted

Another thought: platform_get_irq emits an error message for all
problems. Wouldn't it be consistent to let platform_get_irq_optional()
emit an error message for all problems but "not found"?
Alternatively remove the error printk from platform_get_irq().

Yes, all problems but not found are real errors.

If you want to make platform_get_irq and its optional variant more
similar to the others, dropping the error message is the way to go.

This is not a simplification, just looking at the line count and the
added gotos. That's because it also improves error handling and so the
effect isn't easily spotted.

I like it when the "if (ret < ) ..." error handling is the first check to do.

That's a relevant difference between us.

With -ENXIO, it becomes more convoluted. and looks less nice (IMHO).

quoted

Preferably first simplify in-driver logic to make the conversion to the
new platform_get_irq_optional() actually reviewable.

So I have to choose between

    if (ret < 0 && ret != -ENXIO)
            return ret;

    if (ret) {
            ...
    }

and

    if (ret == -ENXIO) {
            ...
    } else if (ret < 0)
            return ret;
    }

I would do the latter, then it's in the normal order for error handling

	handle some specific errors;
	forward unhandled errors up the stack;
	handle success;

but it seems you prefer to not call "not found" an error. Actually I
think it's an advantage that the driver has to mention -ENXIO, feels
like proper error handling to me. I guess we won't agree about that
though.

What about the following idea (in pythonic pseudo code for simplicity):

	# the rspi device either has two irqs, one for rx and one for
	# tx, or a single one for both together.

	def muxed_hander(irq):
		status = readl(STATUS)
		if status & IF_RX:
			rx_handler()
		if status & IF_TX:
			tx_handler()

	def probe_muxed_irq():
		irq = platform_get_irq_by_name("mux")
		if irq < 0:
			return irq;

		request_irq(irq, muxed_handler)

	def probe_separate_irqs():
		txirq = platform_get_irq_by_name("tx")
		if txirq < 0:
			return txirq

		rxirq = platform_get_irq_by_name("rx")
		if rxirq < 0:
			return rxirq

		request_irq(txirq, tx_handler)
		request_irq(rxirq, rx_handler)

	def probe():
		ret = probe_separate_irqs()
		if ret == -ENXIO:
			ret = probe_muxed_irq()

		if ret < 0:
			return ret

looks clean (to me that is) and allows to skip the demuxing in
tx_handler and rx_handler (which might or might not yield improved
runtime behaviour). Maybe a bit more verbose, but simpler to grasp for a
human, isn't it?

with the final target being

    if (ret < 0)
            return ret;

    if (ret) {
            ...
    }

So the first option means the final change is smaller, but it looks less
nice than the second option (IMHO).
But the second option means more churn.

quoted

quoted

So there are three reasons: because the absence of an optional IRQ
is not an error, and thus that should not cause (a) an error code
to be returned, and (b) an error message to be printed, and (c)
because it can simplify the logic in device drivers.

I don't agree to (a). If the value signaling not-found is -ENXIO or 0
(or -ENODEV) doesn't matter much. I wouldn't deviate from the return
code semantics of platform_get_irq() just for having to check against 0
instead of -ENXIO. Zero is then just another magic value.

Zero is a natural magic value (also for pointers).
Errors are always negative.
Positive values are cookies (or pointers) associated with success.

Yeah, the issue where we don't agree is if "not-found" is special enough
to deserve the natural magic value. For me -ENXIO is magic enough to
handle the absence of an irq line. I consider it even the better magic
value.

quoted

(c) still has to be proven, see above.

Best regards
Uwe

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