Hi Sergey, On Sat, Feb 12, 2022 at 9:17 PM Sergey Shtylyov <s.shtylyov@xxxxxx> wrote: > This patch is based on the former Andy Shevchenko's patch: > > https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@xxxxxxxxxxxxxxx/ > > Currently platform_get_irq_optional() returns an error code even if IRQ > resource simply has not been found. It prevents the callers from being > error code agnostic in their error handling: > > ret = platform_get_irq_optional(...); > if (ret < 0 && ret != -ENXIO) > return ret; // respect deferred probe > if (ret > 0) > ...we get an IRQ... > > All other *_optional() APIs seem to return 0 or NULL in case an optional > resource is not available. Let's follow this good example, so that the > callers would look like: > > ret = platform_get_irq_optional(...); > if (ret < 0) > return ret; > if (ret > 0) > ...we get an IRQ... > > Reported-by: Matthias Schiffer <matthias.schiffer@xxxxxxxxxxxxxxx> > Signed-off-by: Sergey Shtylyov <s.shtylyov@xxxxxx> > --- > Changes in version 2: Thanks for the update! > drivers/base/platform.c | 60 +++++++++++++++--------- The core change LGTM. I'm only looking at Renesas drivers below... > --- a/drivers/mmc/host/sh_mmcif.c > +++ b/drivers/mmc/host/sh_mmcif.c > @@ -1465,14 +1465,14 @@ static int sh_mmcif_probe(struct platform_device *pdev) > sh_mmcif_sync_reset(host); > sh_mmcif_writel(host->addr, MMCIF_CE_INT_MASK, MASK_ALL); > > - name = irq[1] < 0 ? dev_name(dev) : "sh_mmc:error"; > + name = irq[1] <= 0 ? dev_name(dev) : "sh_mmc:error"; "== 0" should be sufficient here, if the code above would bail out on errors returned by platform_get_irq_optional(), which it currently doesn't do. As this adds missing error handling, this is to be fixed by a separate patch later? > ret = devm_request_threaded_irq(dev, irq[0], sh_mmcif_intr, > sh_mmcif_irqt, 0, name, host); > if (ret) { > dev_err(dev, "request_irq error (%s)\n", name); > goto err_clk; > } > - if (irq[1] >= 0) { > + if (irq[1] > 0) { OK. > ret = devm_request_threaded_irq(dev, irq[1], > sh_mmcif_intr, sh_mmcif_irqt, > 0, "sh_mmc:int", host); > --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c > +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c > @@ -439,7 +439,7 @@ static int rcar_gen3_phy_usb2_init(struct phy *p) > u32 val; > int ret; > > - if (!rcar_gen3_is_any_rphy_initialized(channel) && channel->irq >= 0) { > + if (!rcar_gen3_is_any_rphy_initialized(channel) && channel->irq > 0) { > INIT_WORK(&channel->work, rcar_gen3_phy_usb2_work); > ret = request_irq(channel->irq, rcar_gen3_phy_usb2_irq, > IRQF_SHARED, dev_name(channel->dev), channel); > @@ -486,7 +486,7 @@ static int rcar_gen3_phy_usb2_exit(struct phy *p) > val &= ~USB2_INT_ENABLE_UCOM_INTEN; > writel(val, usb2_base + USB2_INT_ENABLE); > > - if (channel->irq >= 0 && !rcar_gen3_is_any_rphy_initialized(channel)) > + if (channel->irq > 0 && !rcar_gen3_is_any_rphy_initialized(channel)) > free_irq(channel->irq, channel); > > return 0; LGTM, but note that all errors returned by platform_get_irq_optional() are currently ignored, even real errors, which should be propagated up. As this adds missing error handling, this is to be fixed by a separate patch later? > --- a/drivers/thermal/rcar_gen3_thermal.c > +++ b/drivers/thermal/rcar_gen3_thermal.c > @@ -432,6 +432,8 @@ static int rcar_gen3_thermal_request_irqs(struct rcar_gen3_thermal_priv *priv, > irq = platform_get_irq_optional(pdev, i); > if (irq < 0) > return irq; > + if (!irq) > + return -ENXIO; While correct, and preserving existing behavior, this looks strange to me. Probably this should return zero instead (i.e. the check above should be changed to "<= 0"), and the caller should start caring about and propagating up real errors. As this adds missing error handling, this is to be fixed by a separate patch later? > > irqname = devm_kasprintf(dev, GFP_KERNEL, "%s:ch%d", > dev_name(dev), i); > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c > index fb65dc601b23..328ab074fd89 100644 > --- a/drivers/tty/serial/sh-sci.c > +++ b/drivers/tty/serial/sh-sci.c I think you missed #define SCIx_IRQ_IS_MUXED(port) \ ((port)->irqs[SCIx_ERI_IRQ] == \ (port)->irqs[SCIx_RXI_IRQ]) || \ ((port)->irqs[SCIx_ERI_IRQ] && \ ((port)->irqs[SCIx_RXI_IRQ] < 0)) above? The last condition should become "<= 0". > @@ -1915,7 +1915,7 @@ static int sci_request_irq(struct sci_port *port) > * Certain port types won't support all of the > * available interrupt sources. > */ > - if (unlikely(irq < 0)) > + if (unlikely(irq <= 0)) > continue; > } > > @@ -1963,7 +1963,7 @@ static void sci_free_irq(struct sci_port *port) > * Certain port types won't support all of the available > * interrupt sources. > */ > - if (unlikely(irq < 0)) > + if (unlikely(irq <= 0)) > continue; > > /* Check if already freed (irq was muxed) */ > @@ -2875,7 +2875,7 @@ static int sci_init_single(struct platform_device *dev, > if (sci_port->irqs[0] < 0) > return -ENXIO; > > - if (sci_port->irqs[1] < 0) > + if (sci_port->irqs[1] <= 0) > for (i = 1; i < ARRAY_SIZE(sci_port->irqs); i++) > sci_port->irqs[i] = sci_port->irqs[0]; > Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
