Hello, On Sun, Jan 16, 2022 at 09:15:20PM +0300, Sergey Shtylyov wrote: > On 1/14/22 11:22 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. > > I have noting to say here, rather than optional IRQ could well have a different > meaning than for clk/gpio/etc. > > [...] > >>> 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 ... > > Note that we should absolutely (and first of all) stop returning 0 from platform_get_irq() > on a "real" IRQ0. Handling that "still good" zero absolutely doesn't scale e.g. for the subsystems > (like libata) which take 0 as an indication that the polling mode should be used... We can't afford > to be sloppy here. ;-) Then maybe do that really first? I didn't recheck, but is this what the driver changes in your patch is about? After some more thoughts I wonder if your focus isn't to align platform_get_irq_optional to (clk|gpiod|regulator)_get_optional, but to simplify return code checking. Because with your change we have: - < 0 -> error - == 0 -> no irq - > 0 -> irq For my part I'd say this doesn't justify the change, but at least I could better life with the reasoning. If you start at: irq = platform_get_irq_optional(...) if (irq < 0 && irq != -ENXIO) return irq else if (irq > 0) setup_irq(irq); else setup_polling() I'd change that to irq = platform_get_irq_optional(...) if (irq > 0) /* or >= 0 ? */ setup_irq(irq) else if (irq == -ENXIO) setup_polling() else return irq This still has to mention -ENXIO, but this is ok and checking for 0 just hardcodes a different return value. Anyhow, I think if you still want to change platform_get_irq_optional you should add a few patches converting some drivers which demonstrates the improvement for the callers. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature