* [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-12 20:16 ` Sergey Shtylyov
0 siblings, 0 replies; 44+ messages in thread
From: Sergey Shtylyov @ 2022-02-12 20:16 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel
Cc: Andy Shevchenko, Corey Minyard, Oleksij Rempel,
Pengutronix Kernel Team, William Breathitt Gray, Mun Yew Tham,
Linus Walleij, Bartosz Golaszewski, Thierry Reding,
Uwe Kleine-König, Lee Jones, Kamal Dasu, Florian Fainelli,
bcm-kernel-feedback-list, Peter Korsgaard, Andrew Lunn,
Ulf Hansson, Brian Norris, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, David S. Miller, Jakub Kicinski,
Joakim Zhang, Yoshihiro Shimoda, Kishon Vijay Abraham I,
Vinod Koul, Benson Leung, Guenter Roeck, Zha Qipeng,
Hans de Goede, Mark Gross, John Garry, Mark Brown,
Matthias Brugger, Niklas Söderlund, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Jiri Slaby, Eric Auger, Alex Williamson,
Cornelia Huck, Liam Girdwood, Jaroslav Kysela, Takashi Iwai,
openipmi-developer, linux-iio, linux-gpio, linux-pwm, linux-i2c,
linux-arm-kernel, linux-mmc, linux-mtd, netdev, linux-renesas-soc,
linux-phy, platform-driver-x86, linux-spi, linux-mediatek,
linux-pm, linux-serial, kvm, alsa-devel, Matthias Schiffer
This patch is based on the former Andy Shevchenko's patch:
https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
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@ew.tq-group.com>
Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
Changes in version 2:
- added the error check using dev_err_probe() to platform_get_irq_optional();
- fixed up the IRQ checks in 'drivers/char/ipmi/bt-bmc.c';
- fixed up the IRQ check in 'drivers/thermal/rcar_gen3_thermal.c';
- removed from the patch 'drivers/edac/xgene_edac.c and
'drivers/power/supply/mp2629_charger.c' as they were switched to calling
platform_get_irq();
- shortened the IRQ checking code in 'drivers/vfio/platform/vfio_platform.c';
- rebased atop of the recent platform_get_irq_byname() patch;
- reformatted the patch description.
drivers/base/platform.c | 60 +++++++++++++++---------
drivers/char/ipmi/bt-bmc.c | 6 +--
drivers/counter/interrupt-cnt.c | 4 +-
drivers/gpio/gpio-altera.c | 3 +-
drivers/gpio/gpio-mvebu.c | 2 +-
drivers/gpio/gpio-tqmx86.c | 2 +-
drivers/i2c/busses/i2c-brcmstb.c | 8 ++--
drivers/i2c/busses/i2c-ocores.c | 4 +-
drivers/mmc/host/sh_mmcif.c | 4 +-
drivers/mtd/nand/raw/brcmnand/brcmnand.c | 4 +-
drivers/net/ethernet/davicom/dm9000.c | 2 +-
drivers/net/ethernet/freescale/fec_ptp.c | 2 +-
drivers/phy/renesas/phy-rcar-gen3-usb2.c | 4 +-
drivers/platform/chrome/cros_ec_lpc.c | 2 +-
drivers/platform/x86/intel/punit_ipc.c | 2 +-
drivers/spi/spi-hisi-sfc-v3xx.c | 2 +-
drivers/spi/spi-mtk-nor.c | 3 +-
drivers/thermal/rcar_gen3_thermal.c | 2 +
drivers/tty/serial/8250/8250_mtk.c | 4 +-
drivers/tty/serial/sh-sci.c | 6 +--
drivers/uio/uio_pdrv_genirq.c | 2 +-
drivers/vfio/platform/vfio_platform.c | 3 +-
sound/soc/dwc/dwc-i2s.c | 4 +-
23 files changed, 76 insertions(+), 59 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 7d08cd8947be..52a8356f8261 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -148,25 +148,7 @@ devm_platform_ioremap_resource_byname(struct platform_device *pdev,
EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource_byname);
#endif /* CONFIG_HAS_IOMEM */
-/**
- * platform_get_irq_optional - get an optional IRQ for a device
- * @dev: platform device
- * @num: IRQ number index
- *
- * Gets an IRQ for a platform device. Device drivers should check the return
- * value for errors so as to not pass a negative integer value to the
- * request_irq() APIs. This is the same as platform_get_irq(), except that it
- * does not print an error message if an IRQ can not be obtained.
- *
- * For example::
- *
- * int irq = platform_get_irq_optional(pdev, 0);
- * if (irq < 0)
- * return irq;
- *
- * Return: non-zero IRQ number on success, negative error number on failure.
- */
-int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
+static int __platform_get_irq(struct platform_device *dev, unsigned int num)
{
int ret;
#ifdef CONFIG_SPARC
@@ -235,6 +217,42 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
return -EINVAL;
return ret;
}
+
+/**
+ * platform_get_irq_optional - get an optional IRQ for a device
+ * @dev: platform device
+ * @num: IRQ number index
+ *
+ * Gets an IRQ for a platform device. Device drivers should check the return
+ * value for errors so as to not pass a negative integer value to the
+ * request_irq() APIs. This is the same as platform_get_irq(), except that it
+ * does not print an error message if an IRQ can not be obtained and returns
+ * 0 when IRQ resource has not been found.
+ *
+ * For example::
+ *
+ * int irq = platform_get_irq_optional(pdev, 0);
+ * if (irq < 0)
+ * return irq;
+ * if (irq > 0)
+ * ...we have IRQ line defined...
+ *
+ * Return: non-zero IRQ number on success, 0 if IRQ wasn't found, negative error
+ * number on failure.
+ */
+int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
+{
+ int ret;
+
+ ret = __platform_get_irq(dev, num);
+ if (ret == -ENXIO)
+ return 0;
+ if (ret < 0)
+ return dev_err_probe(&dev->dev, ret,
+ "IRQ index %u not found\n", num);
+
+ return ret;
+}
EXPORT_SYMBOL_GPL(platform_get_irq_optional);
/**
@@ -258,7 +276,7 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
{
int ret;
- ret = platform_get_irq_optional(dev, num);
+ ret = __platform_get_irq(dev, num);
if (ret < 0)
return dev_err_probe(&dev->dev, ret,
"IRQ index %u not found\n", num);
@@ -277,7 +295,7 @@ int platform_irq_count(struct platform_device *dev)
{
int ret, nr = 0;
- while ((ret = platform_get_irq_optional(dev, nr)) >= 0)
+ while ((ret = __platform_get_irq(dev, nr)) >= 0)
nr++;
if (ret == -EPROBE_DEFER)
diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c
index 7450904e330a..289acaf4e720 100644
--- a/drivers/char/ipmi/bt-bmc.c
+++ b/drivers/char/ipmi/bt-bmc.c
@@ -380,7 +380,7 @@ static int bt_bmc_config_irq(struct bt_bmc *bt_bmc,
u32 reg;
bt_bmc->irq = platform_get_irq_optional(pdev, 0);
- if (bt_bmc->irq < 0)
+ if (bt_bmc->irq <= 0)
return bt_bmc->irq;
rc = devm_request_irq(dev, bt_bmc->irq, bt_bmc_irq, IRQF_SHARED,
@@ -438,7 +438,7 @@ static int bt_bmc_probe(struct platform_device *pdev)
bt_bmc_config_irq(bt_bmc, pdev);
- if (bt_bmc->irq >= 0) {
+ if (bt_bmc->irq > 0) {
dev_info(dev, "Using IRQ %d\n", bt_bmc->irq);
} else {
dev_info(dev, "No IRQ; using timer\n");
@@ -464,7 +464,7 @@ static int bt_bmc_remove(struct platform_device *pdev)
struct bt_bmc *bt_bmc = dev_get_drvdata(&pdev->dev);
misc_deregister(&bt_bmc->miscdev);
- if (bt_bmc->irq < 0)
+ if (bt_bmc->irq <= 0)
del_timer_sync(&bt_bmc->poll_timer);
return 0;
}
diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
index 9e99702470c2..a2443c66330b 100644
--- a/drivers/counter/interrupt-cnt.c
+++ b/drivers/counter/interrupt-cnt.c
@@ -157,9 +157,7 @@ static int interrupt_cnt_probe(struct platform_device *pdev)
priv = counter_priv(counter);
priv->irq = platform_get_irq_optional(pdev, 0);
- if (priv->irq == -ENXIO)
- priv->irq = 0;
- else if (priv->irq < 0)
+ if (priv->irq < 0)
return dev_err_probe(dev, priv->irq, "failed to get IRQ\n");
priv->gpio = devm_gpiod_get_optional(dev, NULL, GPIOD_IN);
diff --git a/drivers/gpio/gpio-altera.c b/drivers/gpio/gpio-altera.c
index b59fae993626..02a2995aa368 100644
--- a/drivers/gpio/gpio-altera.c
+++ b/drivers/gpio/gpio-altera.c
@@ -267,8 +267,7 @@ static int altera_gpio_probe(struct platform_device *pdev)
altera_gc->mmchip.gc.parent = &pdev->dev;
altera_gc->mapped_irq = platform_get_irq_optional(pdev, 0);
-
- if (altera_gc->mapped_irq < 0)
+ if (altera_gc->mapped_irq <= 0)
goto skip_irq;
if (of_property_read_u32(node, "altr,interrupt-type", ®)) {
diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index 4c1f9e1091b7..d94594807697 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -1293,7 +1293,7 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
for (i = 0; i < 4; i++) {
int irq = platform_get_irq_optional(pdev, i);
- if (irq < 0)
+ if (irq <= 0)
continue;
irq_set_chained_handler_and_data(irq, mvebu_gpio_irq_handler,
mvchip);
diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
index 5b103221b58d..dc0f83236ce8 100644
--- a/drivers/gpio/gpio-tqmx86.c
+++ b/drivers/gpio/gpio-tqmx86.c
@@ -237,7 +237,7 @@ static int tqmx86_gpio_probe(struct platform_device *pdev)
int ret, irq;
irq = platform_get_irq_optional(pdev, 0);
- if (irq < 0 && irq != -ENXIO)
+ if (irq < 0)
return irq;
res = platform_get_resource(pdev, IORESOURCE_IO, 0);
diff --git a/drivers/i2c/busses/i2c-brcmstb.c b/drivers/i2c/busses/i2c-brcmstb.c
index 490ee3962645..69395ae27a1a 100644
--- a/drivers/i2c/busses/i2c-brcmstb.c
+++ b/drivers/i2c/busses/i2c-brcmstb.c
@@ -250,7 +250,7 @@ static int brcmstb_i2c_wait_for_completion(struct brcmstb_i2c_dev *dev)
int ret = 0;
unsigned long timeout = msecs_to_jiffies(I2C_TIMEOUT);
- if (dev->irq >= 0) {
+ if (dev->irq > 0) {
if (!wait_for_completion_timeout(&dev->done, timeout))
ret = -ETIMEDOUT;
} else {
@@ -297,7 +297,7 @@ static int brcmstb_send_i2c_cmd(struct brcmstb_i2c_dev *dev,
return rc;
/* only if we are in interrupt mode */
- if (dev->irq >= 0)
+ if (dev->irq > 0)
reinit_completion(&dev->done);
/* enable BSC CTL interrupt line */
@@ -652,7 +652,7 @@ static int brcmstb_i2c_probe(struct platform_device *pdev)
brcmstb_i2c_enable_disable_irq(dev, INT_DISABLE);
/* register the ISR handler */
- if (dev->irq >= 0) {
+ if (dev->irq > 0) {
rc = devm_request_irq(&pdev->dev, dev->irq, brcmstb_i2c_isr,
IRQF_SHARED,
int_name ? int_name : pdev->name,
@@ -696,7 +696,7 @@ static int brcmstb_i2c_probe(struct platform_device *pdev)
dev_info(dev->device, "%s@%dhz registered in %s mode\n",
int_name ? int_name : " ", dev->clk_freq_hz,
- (dev->irq >= 0) ? "interrupt" : "polling");
+ (dev->irq > 0) ? "interrupt" : "polling");
return 0;
diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index a0af027db04c..1f4d5e52ff42 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -691,10 +691,10 @@ static int ocores_i2c_probe(struct platform_device *pdev)
if (of_device_is_compatible(pdev->dev.of_node,
"sifive,fu540-c000-i2c")) {
i2c->flags |= OCORES_FLAG_BROKEN_IRQ;
- irq = -ENXIO;
+ irq = 0;
}
- if (irq == -ENXIO) {
+ if (!irq) {
ocores_algorithm.master_xfer = ocores_xfer_polling;
} else {
if (irq < 0)
diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index bcc595c70a9f..f558b9862032 100644
--- 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";
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) {
ret = devm_request_threaded_irq(dev, irq[1],
sh_mmcif_intr, sh_mmcif_irqt,
0, "sh_mmc:int", host);
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index f75929783b94..ac222985efde 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -1521,7 +1521,7 @@ static irqreturn_t brcmnand_ctlrdy_irq(int irq, void *data)
/* check if you need to piggy back on the ctrlrdy irq */
if (ctrl->edu_pending) {
- if (irq == ctrl->irq && ((int)ctrl->edu_irq >= 0))
+ if (irq == ctrl->irq && ((int)ctrl->edu_irq > 0))
/* Discard interrupts while using dedicated edu irq */
return IRQ_HANDLED;
@@ -2956,7 +2956,7 @@ static int brcmnand_edu_setup(struct platform_device *pdev)
brcmnand_edu_init(ctrl);
ctrl->edu_irq = platform_get_irq_optional(pdev, 1);
- if (ctrl->edu_irq < 0) {
+ if (ctrl->edu_irq <= 0) {
dev_warn(dev,
"FLASH EDU enabled, using ctlrdy irq\n");
} else {
diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
index 0985ab216566..740c660a9411 100644
--- a/drivers/net/ethernet/davicom/dm9000.c
+++ b/drivers/net/ethernet/davicom/dm9000.c
@@ -1509,7 +1509,7 @@ dm9000_probe(struct platform_device *pdev)
}
db->irq_wake = platform_get_irq_optional(pdev, 1);
- if (db->irq_wake >= 0) {
+ if (db->irq_wake > 0) {
dev_dbg(db->dev, "wakeup irq %d\n", db->irq_wake);
ret = request_irq(db->irq_wake, dm9000_wol_interrupt,
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index af99017a5453..de1d23808b6c 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -616,7 +616,7 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
/* Failure to get an irq is not fatal,
* only the PTP_CLOCK_PPS clock events should stop
*/
- if (irq >= 0) {
+ if (irq > 0) {
ret = devm_request_irq(&pdev->dev, irq, fec_pps_interrupt,
0, pdev->name, ndev);
if (ret < 0)
diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
index 9de617ca9daa..4914d6aca208 100644
--- 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;
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index d6306d2a096f..91686d306534 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -400,7 +400,7 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
irq = platform_get_irq_optional(pdev, 0);
if (irq > 0)
ec_dev->irq = irq;
- else if (irq != -ENXIO) {
+ else if (irq < 0) {
dev_err(dev, "couldn't retrieve IRQ number (%d)\n", irq);
return irq;
}
diff --git a/drivers/platform/x86/intel/punit_ipc.c b/drivers/platform/x86/intel/punit_ipc.c
index 66bb39fd0ef9..f3cf5ee1466f 100644
--- a/drivers/platform/x86/intel/punit_ipc.c
+++ b/drivers/platform/x86/intel/punit_ipc.c
@@ -278,7 +278,7 @@ static int intel_punit_ipc_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, punit_ipcdev);
irq = platform_get_irq_optional(pdev, 0);
- if (irq < 0) {
+ if (irq <= 0) {
dev_warn(&pdev->dev, "Invalid IRQ, using polling mode\n");
} else {
ret = devm_request_irq(&pdev->dev, irq, intel_punit_ioc,
diff --git a/drivers/spi/spi-hisi-sfc-v3xx.c b/drivers/spi/spi-hisi-sfc-v3xx.c
index d3a23b1c2a4c..476ddc081c60 100644
--- a/drivers/spi/spi-hisi-sfc-v3xx.c
+++ b/drivers/spi/spi-hisi-sfc-v3xx.c
@@ -467,7 +467,7 @@ static int hisi_sfc_v3xx_probe(struct platform_device *pdev)
dev_err(dev, "failed to request irq%d, ret = %d\n", host->irq, ret);
host->irq = 0;
}
- } else {
+ } else if (host->irq < 0) {
host->irq = 0;
}
diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
index 5c93730615f8..2422b0545936 100644
--- a/drivers/spi/spi-mtk-nor.c
+++ b/drivers/spi/spi-mtk-nor.c
@@ -829,8 +829,7 @@ static int mtk_nor_probe(struct platform_device *pdev)
mtk_nor_init(sp);
irq = platform_get_irq_optional(pdev, 0);
-
- if (irq < 0) {
+ if (irq <= 0) {
dev_warn(sp->dev, "IRQ not available.");
} else {
ret = devm_request_irq(sp->dev, irq, mtk_nor_irq_handler, 0,
diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 43eb25b167bc..359b9941c42b 100644
--- 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;
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/8250/8250_mtk.c
+++ b/drivers/tty/serial/8250/8250_mtk.c
@@ -621,7 +621,7 @@ static int __maybe_unused mtk8250_suspend(struct device *dev)
serial8250_suspend_port(data->line);
pinctrl_pm_select_sleep_state(dev);
- if (irq >= 0) {
+ if (irq > 0) {
err = enable_irq_wake(irq);
if (err) {
dev_err(dev,
@@ -641,7 +641,7 @@ static int __maybe_unused mtk8250_resume(struct device *dev)
struct mtk8250_data *data = dev_get_drvdata(dev);
int irq = data->rx_wakeup_irq;
- if (irq >= 0)
+ if (irq > 0)
disable_irq_wake(irq);
pinctrl_pm_select_default_state(dev);
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 968967d722d4..96c3146ff6a4 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -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];
diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
index 63258b6accc4..7fd275fc6ceb 100644
--- a/drivers/uio/uio_pdrv_genirq.c
+++ b/drivers/uio/uio_pdrv_genirq.c
@@ -162,7 +162,7 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
if (!uioinfo->irq) {
ret = platform_get_irq_optional(pdev, 0);
uioinfo->irq = ret;
- if (ret == -ENXIO)
+ if (!ret)
uioinfo->irq = UIO_IRQ_NONE;
else if (ret == -EPROBE_DEFER)
return ret;
diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
index 68a1c87066d7..f7423d10cefd 100644
--- a/drivers/vfio/platform/vfio_platform.c
+++ b/drivers/vfio/platform/vfio_platform.c
@@ -32,8 +32,9 @@ static struct resource *get_platform_resource(struct vfio_platform_device *vdev,
static int get_platform_irq(struct vfio_platform_device *vdev, int i)
{
struct platform_device *pdev = (struct platform_device *) vdev->opaque;
+ int ret = platform_get_irq_optional(pdev, i);
- return platform_get_irq_optional(pdev, i);
+ return ret ? ret : -ENXIO;
}
static int vfio_platform_probe(struct platform_device *pdev)
diff --git a/sound/soc/dwc/dwc-i2s.c b/sound/soc/dwc/dwc-i2s.c
index 5cb58929090d..ff19c5130459 100644
--- a/sound/soc/dwc/dwc-i2s.c
+++ b/sound/soc/dwc/dwc-i2s.c
@@ -643,7 +643,7 @@ static int dw_i2s_probe(struct platform_device *pdev)
dev->dev = &pdev->dev;
irq = platform_get_irq_optional(pdev, 0);
- if (irq >= 0) {
+ if (irq > 0) {
ret = devm_request_irq(&pdev->dev, irq, i2s_irq_handler, 0,
pdev->name, dev);
if (ret < 0) {
@@ -697,7 +697,7 @@ static int dw_i2s_probe(struct platform_device *pdev)
}
if (!pdata) {
- if (irq >= 0) {
+ if (irq > 0) {
ret = dw_pcm_register(pdev);
dev->use_pio = true;
} else {
--
2.26.3
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-12 20:16 ` Sergey Shtylyov
0 siblings, 0 replies; 44+ messages in thread
From: Sergey Shtylyov @ 2022-02-12 20:16 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel
Cc: Andy Shevchenko, Corey Minyard, Oleksij Rempel,
Pengutronix Kernel Team, William Breathitt Gray, Mun Yew Tham,
Linus Walleij, Bartosz Golaszewski, Thierry Reding,
Uwe Kleine-König, Lee Jones, Kamal Dasu, Florian Fainelli,
bcm-kernel-feedback-list, Peter Korsgaard, Andrew Lunn,
Ulf Hansson, Brian Norris, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, David S. Miller, Jakub Kicinski,
Joakim Zhang, Yoshihiro Shimoda, Kishon Vijay Abraham I,
Vinod Koul, Benson Leung, Guenter Roeck, Zha Qipeng,
Hans de Goede, Mark Gross, John Garry, Mark Brown,
Matthias Brugger, Niklas Söderlund, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Jiri Slaby, Eric Auger, Alex Williamson,
Cornelia Huck, Liam Girdwood, Jaroslav Kysela, Takashi Iwai,
openipmi-developer, linux-iio, linux-gpio, linux-pwm, linux-i2c,
linux-arm-kernel, linux-mmc, linux-mtd, netdev, linux-renesas-soc,
linux-phy, platform-driver-x86, linux-spi, linux-mediatek,
linux-pm, linux-serial, kvm, alsa-devel, Matthias Schiffer
This patch is based on the former Andy Shevchenko's patch:
https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
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@ew.tq-group.com>
Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
Changes in version 2:
- added the error check using dev_err_probe() to platform_get_irq_optional();
- fixed up the IRQ checks in 'drivers/char/ipmi/bt-bmc.c';
- fixed up the IRQ check in 'drivers/thermal/rcar_gen3_thermal.c';
- removed from the patch 'drivers/edac/xgene_edac.c and
'drivers/power/supply/mp2629_charger.c' as they were switched to calling
platform_get_irq();
- shortened the IRQ checking code in 'drivers/vfio/platform/vfio_platform.c';
- rebased atop of the recent platform_get_irq_byname() patch;
- reformatted the patch description.
drivers/base/platform.c | 60 +++++++++++++++---------
drivers/char/ipmi/bt-bmc.c | 6 +--
drivers/counter/interrupt-cnt.c | 4 +-
drivers/gpio/gpio-altera.c | 3 +-
drivers/gpio/gpio-mvebu.c | 2 +-
drivers/gpio/gpio-tqmx86.c | 2 +-
drivers/i2c/busses/i2c-brcmstb.c | 8 ++--
drivers/i2c/busses/i2c-ocores.c | 4 +-
drivers/mmc/host/sh_mmcif.c | 4 +-
drivers/mtd/nand/raw/brcmnand/brcmnand.c | 4 +-
drivers/net/ethernet/davicom/dm9000.c | 2 +-
drivers/net/ethernet/freescale/fec_ptp.c | 2 +-
drivers/phy/renesas/phy-rcar-gen3-usb2.c | 4 +-
drivers/platform/chrome/cros_ec_lpc.c | 2 +-
drivers/platform/x86/intel/punit_ipc.c | 2 +-
drivers/spi/spi-hisi-sfc-v3xx.c | 2 +-
drivers/spi/spi-mtk-nor.c | 3 +-
drivers/thermal/rcar_gen3_thermal.c | 2 +
drivers/tty/serial/8250/8250_mtk.c | 4 +-
drivers/tty/serial/sh-sci.c | 6 +--
drivers/uio/uio_pdrv_genirq.c | 2 +-
drivers/vfio/platform/vfio_platform.c | 3 +-
sound/soc/dwc/dwc-i2s.c | 4 +-
23 files changed, 76 insertions(+), 59 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 7d08cd8947be..52a8356f8261 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -148,25 +148,7 @@ devm_platform_ioremap_resource_byname(struct platform_device *pdev,
EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource_byname);
#endif /* CONFIG_HAS_IOMEM */
-/**
- * platform_get_irq_optional - get an optional IRQ for a device
- * @dev: platform device
- * @num: IRQ number index
- *
- * Gets an IRQ for a platform device. Device drivers should check the return
- * value for errors so as to not pass a negative integer value to the
- * request_irq() APIs. This is the same as platform_get_irq(), except that it
- * does not print an error message if an IRQ can not be obtained.
- *
- * For example::
- *
- * int irq = platform_get_irq_optional(pdev, 0);
- * if (irq < 0)
- * return irq;
- *
- * Return: non-zero IRQ number on success, negative error number on failure.
- */
-int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
+static int __platform_get_irq(struct platform_device *dev, unsigned int num)
{
int ret;
#ifdef CONFIG_SPARC
@@ -235,6 +217,42 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
return -EINVAL;
return ret;
}
+
+/**
+ * platform_get_irq_optional - get an optional IRQ for a device
+ * @dev: platform device
+ * @num: IRQ number index
+ *
+ * Gets an IRQ for a platform device. Device drivers should check the return
+ * value for errors so as to not pass a negative integer value to the
+ * request_irq() APIs. This is the same as platform_get_irq(), except that it
+ * does not print an error message if an IRQ can not be obtained and returns
+ * 0 when IRQ resource has not been found.
+ *
+ * For example::
+ *
+ * int irq = platform_get_irq_optional(pdev, 0);
+ * if (irq < 0)
+ * return irq;
+ * if (irq > 0)
+ * ...we have IRQ line defined...
+ *
+ * Return: non-zero IRQ number on success, 0 if IRQ wasn't found, negative error
+ * number on failure.
+ */
+int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
+{
+ int ret;
+
+ ret = __platform_get_irq(dev, num);
+ if (ret == -ENXIO)
+ return 0;
+ if (ret < 0)
+ return dev_err_probe(&dev->dev, ret,
+ "IRQ index %u not found\n", num);
+
+ return ret;
+}
EXPORT_SYMBOL_GPL(platform_get_irq_optional);
/**
@@ -258,7 +276,7 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
{
int ret;
- ret = platform_get_irq_optional(dev, num);
+ ret = __platform_get_irq(dev, num);
if (ret < 0)
return dev_err_probe(&dev->dev, ret,
"IRQ index %u not found\n", num);
@@ -277,7 +295,7 @@ int platform_irq_count(struct platform_device *dev)
{
int ret, nr = 0;
- while ((ret = platform_get_irq_optional(dev, nr)) >= 0)
+ while ((ret = __platform_get_irq(dev, nr)) >= 0)
nr++;
if (ret == -EPROBE_DEFER)
diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c
index 7450904e330a..289acaf4e720 100644
--- a/drivers/char/ipmi/bt-bmc.c
+++ b/drivers/char/ipmi/bt-bmc.c
@@ -380,7 +380,7 @@ static int bt_bmc_config_irq(struct bt_bmc *bt_bmc,
u32 reg;
bt_bmc->irq = platform_get_irq_optional(pdev, 0);
- if (bt_bmc->irq < 0)
+ if (bt_bmc->irq <= 0)
return bt_bmc->irq;
rc = devm_request_irq(dev, bt_bmc->irq, bt_bmc_irq, IRQF_SHARED,
@@ -438,7 +438,7 @@ static int bt_bmc_probe(struct platform_device *pdev)
bt_bmc_config_irq(bt_bmc, pdev);
- if (bt_bmc->irq >= 0) {
+ if (bt_bmc->irq > 0) {
dev_info(dev, "Using IRQ %d\n", bt_bmc->irq);
} else {
dev_info(dev, "No IRQ; using timer\n");
@@ -464,7 +464,7 @@ static int bt_bmc_remove(struct platform_device *pdev)
struct bt_bmc *bt_bmc = dev_get_drvdata(&pdev->dev);
misc_deregister(&bt_bmc->miscdev);
- if (bt_bmc->irq < 0)
+ if (bt_bmc->irq <= 0)
del_timer_sync(&bt_bmc->poll_timer);
return 0;
}
diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
index 9e99702470c2..a2443c66330b 100644
--- a/drivers/counter/interrupt-cnt.c
+++ b/drivers/counter/interrupt-cnt.c
@@ -157,9 +157,7 @@ static int interrupt_cnt_probe(struct platform_device *pdev)
priv = counter_priv(counter);
priv->irq = platform_get_irq_optional(pdev, 0);
- if (priv->irq == -ENXIO)
- priv->irq = 0;
- else if (priv->irq < 0)
+ if (priv->irq < 0)
return dev_err_probe(dev, priv->irq, "failed to get IRQ\n");
priv->gpio = devm_gpiod_get_optional(dev, NULL, GPIOD_IN);
diff --git a/drivers/gpio/gpio-altera.c b/drivers/gpio/gpio-altera.c
index b59fae993626..02a2995aa368 100644
--- a/drivers/gpio/gpio-altera.c
+++ b/drivers/gpio/gpio-altera.c
@@ -267,8 +267,7 @@ static int altera_gpio_probe(struct platform_device *pdev)
altera_gc->mmchip.gc.parent = &pdev->dev;
altera_gc->mapped_irq = platform_get_irq_optional(pdev, 0);
-
- if (altera_gc->mapped_irq < 0)
+ if (altera_gc->mapped_irq <= 0)
goto skip_irq;
if (of_property_read_u32(node, "altr,interrupt-type", ®)) {
diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index 4c1f9e1091b7..d94594807697 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -1293,7 +1293,7 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
for (i = 0; i < 4; i++) {
int irq = platform_get_irq_optional(pdev, i);
- if (irq < 0)
+ if (irq <= 0)
continue;
irq_set_chained_handler_and_data(irq, mvebu_gpio_irq_handler,
mvchip);
diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
index 5b103221b58d..dc0f83236ce8 100644
--- a/drivers/gpio/gpio-tqmx86.c
+++ b/drivers/gpio/gpio-tqmx86.c
@@ -237,7 +237,7 @@ static int tqmx86_gpio_probe(struct platform_device *pdev)
int ret, irq;
irq = platform_get_irq_optional(pdev, 0);
- if (irq < 0 && irq != -ENXIO)
+ if (irq < 0)
return irq;
res = platform_get_resource(pdev, IORESOURCE_IO, 0);
diff --git a/drivers/i2c/busses/i2c-brcmstb.c b/drivers/i2c/busses/i2c-brcmstb.c
index 490ee3962645..69395ae27a1a 100644
--- a/drivers/i2c/busses/i2c-brcmstb.c
+++ b/drivers/i2c/busses/i2c-brcmstb.c
@@ -250,7 +250,7 @@ static int brcmstb_i2c_wait_for_completion(struct brcmstb_i2c_dev *dev)
int ret = 0;
unsigned long timeout = msecs_to_jiffies(I2C_TIMEOUT);
- if (dev->irq >= 0) {
+ if (dev->irq > 0) {
if (!wait_for_completion_timeout(&dev->done, timeout))
ret = -ETIMEDOUT;
} else {
@@ -297,7 +297,7 @@ static int brcmstb_send_i2c_cmd(struct brcmstb_i2c_dev *dev,
return rc;
/* only if we are in interrupt mode */
- if (dev->irq >= 0)
+ if (dev->irq > 0)
reinit_completion(&dev->done);
/* enable BSC CTL interrupt line */
@@ -652,7 +652,7 @@ static int brcmstb_i2c_probe(struct platform_device *pdev)
brcmstb_i2c_enable_disable_irq(dev, INT_DISABLE);
/* register the ISR handler */
- if (dev->irq >= 0) {
+ if (dev->irq > 0) {
rc = devm_request_irq(&pdev->dev, dev->irq, brcmstb_i2c_isr,
IRQF_SHARED,
int_name ? int_name : pdev->name,
@@ -696,7 +696,7 @@ static int brcmstb_i2c_probe(struct platform_device *pdev)
dev_info(dev->device, "%s@%dhz registered in %s mode\n",
int_name ? int_name : " ", dev->clk_freq_hz,
- (dev->irq >= 0) ? "interrupt" : "polling");
+ (dev->irq > 0) ? "interrupt" : "polling");
return 0;
diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index a0af027db04c..1f4d5e52ff42 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -691,10 +691,10 @@ static int ocores_i2c_probe(struct platform_device *pdev)
if (of_device_is_compatible(pdev->dev.of_node,
"sifive,fu540-c000-i2c")) {
i2c->flags |= OCORES_FLAG_BROKEN_IRQ;
- irq = -ENXIO;
+ irq = 0;
}
- if (irq == -ENXIO) {
+ if (!irq) {
ocores_algorithm.master_xfer = ocores_xfer_polling;
} else {
if (irq < 0)
diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index bcc595c70a9f..f558b9862032 100644
--- 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";
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) {
ret = devm_request_threaded_irq(dev, irq[1],
sh_mmcif_intr, sh_mmcif_irqt,
0, "sh_mmc:int", host);
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index f75929783b94..ac222985efde 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -1521,7 +1521,7 @@ static irqreturn_t brcmnand_ctlrdy_irq(int irq, void *data)
/* check if you need to piggy back on the ctrlrdy irq */
if (ctrl->edu_pending) {
- if (irq == ctrl->irq && ((int)ctrl->edu_irq >= 0))
+ if (irq == ctrl->irq && ((int)ctrl->edu_irq > 0))
/* Discard interrupts while using dedicated edu irq */
return IRQ_HANDLED;
@@ -2956,7 +2956,7 @@ static int brcmnand_edu_setup(struct platform_device *pdev)
brcmnand_edu_init(ctrl);
ctrl->edu_irq = platform_get_irq_optional(pdev, 1);
- if (ctrl->edu_irq < 0) {
+ if (ctrl->edu_irq <= 0) {
dev_warn(dev,
"FLASH EDU enabled, using ctlrdy irq\n");
} else {
diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
index 0985ab216566..740c660a9411 100644
--- a/drivers/net/ethernet/davicom/dm9000.c
+++ b/drivers/net/ethernet/davicom/dm9000.c
@@ -1509,7 +1509,7 @@ dm9000_probe(struct platform_device *pdev)
}
db->irq_wake = platform_get_irq_optional(pdev, 1);
- if (db->irq_wake >= 0) {
+ if (db->irq_wake > 0) {
dev_dbg(db->dev, "wakeup irq %d\n", db->irq_wake);
ret = request_irq(db->irq_wake, dm9000_wol_interrupt,
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index af99017a5453..de1d23808b6c 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -616,7 +616,7 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
/* Failure to get an irq is not fatal,
* only the PTP_CLOCK_PPS clock events should stop
*/
- if (irq >= 0) {
+ if (irq > 0) {
ret = devm_request_irq(&pdev->dev, irq, fec_pps_interrupt,
0, pdev->name, ndev);
if (ret < 0)
diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
index 9de617ca9daa..4914d6aca208 100644
--- 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;
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index d6306d2a096f..91686d306534 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -400,7 +400,7 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
irq = platform_get_irq_optional(pdev, 0);
if (irq > 0)
ec_dev->irq = irq;
- else if (irq != -ENXIO) {
+ else if (irq < 0) {
dev_err(dev, "couldn't retrieve IRQ number (%d)\n", irq);
return irq;
}
diff --git a/drivers/platform/x86/intel/punit_ipc.c b/drivers/platform/x86/intel/punit_ipc.c
index 66bb39fd0ef9..f3cf5ee1466f 100644
--- a/drivers/platform/x86/intel/punit_ipc.c
+++ b/drivers/platform/x86/intel/punit_ipc.c
@@ -278,7 +278,7 @@ static int intel_punit_ipc_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, punit_ipcdev);
irq = platform_get_irq_optional(pdev, 0);
- if (irq < 0) {
+ if (irq <= 0) {
dev_warn(&pdev->dev, "Invalid IRQ, using polling mode\n");
} else {
ret = devm_request_irq(&pdev->dev, irq, intel_punit_ioc,
diff --git a/drivers/spi/spi-hisi-sfc-v3xx.c b/drivers/spi/spi-hisi-sfc-v3xx.c
index d3a23b1c2a4c..476ddc081c60 100644
--- a/drivers/spi/spi-hisi-sfc-v3xx.c
+++ b/drivers/spi/spi-hisi-sfc-v3xx.c
@@ -467,7 +467,7 @@ static int hisi_sfc_v3xx_probe(struct platform_device *pdev)
dev_err(dev, "failed to request irq%d, ret = %d\n", host->irq, ret);
host->irq = 0;
}
- } else {
+ } else if (host->irq < 0) {
host->irq = 0;
}
diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
index 5c93730615f8..2422b0545936 100644
--- a/drivers/spi/spi-mtk-nor.c
+++ b/drivers/spi/spi-mtk-nor.c
@@ -829,8 +829,7 @@ static int mtk_nor_probe(struct platform_device *pdev)
mtk_nor_init(sp);
irq = platform_get_irq_optional(pdev, 0);
-
- if (irq < 0) {
+ if (irq <= 0) {
dev_warn(sp->dev, "IRQ not available.");
} else {
ret = devm_request_irq(sp->dev, irq, mtk_nor_irq_handler, 0,
diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 43eb25b167bc..359b9941c42b 100644
--- 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;
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/8250/8250_mtk.c
+++ b/drivers/tty/serial/8250/8250_mtk.c
@@ -621,7 +621,7 @@ static int __maybe_unused mtk8250_suspend(struct device *dev)
serial8250_suspend_port(data->line);
pinctrl_pm_select_sleep_state(dev);
- if (irq >= 0) {
+ if (irq > 0) {
err = enable_irq_wake(irq);
if (err) {
dev_err(dev,
@@ -641,7 +641,7 @@ static int __maybe_unused mtk8250_resume(struct device *dev)
struct mtk8250_data *data = dev_get_drvdata(dev);
int irq = data->rx_wakeup_irq;
- if (irq >= 0)
+ if (irq > 0)
disable_irq_wake(irq);
pinctrl_pm_select_default_state(dev);
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 968967d722d4..96c3146ff6a4 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -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];
diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
index 63258b6accc4..7fd275fc6ceb 100644
--- a/drivers/uio/uio_pdrv_genirq.c
+++ b/drivers/uio/uio_pdrv_genirq.c
@@ -162,7 +162,7 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
if (!uioinfo->irq) {
ret = platform_get_irq_optional(pdev, 0);
uioinfo->irq = ret;
- if (ret == -ENXIO)
+ if (!ret)
uioinfo->irq = UIO_IRQ_NONE;
else if (ret == -EPROBE_DEFER)
return ret;
diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
index 68a1c87066d7..f7423d10cefd 100644
--- a/drivers/vfio/platform/vfio_platform.c
+++ b/drivers/vfio/platform/vfio_platform.c
@@ -32,8 +32,9 @@ static struct resource *get_platform_resource(struct vfio_platform_device *vdev,
static int get_platform_irq(struct vfio_platform_device *vdev, int i)
{
struct platform_device *pdev = (struct platform_device *) vdev->opaque;
+ int ret = platform_get_irq_optional(pdev, i);
- return platform_get_irq_optional(pdev, i);
+ return ret ? ret : -ENXIO;
}
static int vfio_platform_probe(struct platform_device *pdev)
diff --git a/sound/soc/dwc/dwc-i2s.c b/sound/soc/dwc/dwc-i2s.c
index 5cb58929090d..ff19c5130459 100644
--- a/sound/soc/dwc/dwc-i2s.c
+++ b/sound/soc/dwc/dwc-i2s.c
@@ -643,7 +643,7 @@ static int dw_i2s_probe(struct platform_device *pdev)
dev->dev = &pdev->dev;
irq = platform_get_irq_optional(pdev, 0);
- if (irq >= 0) {
+ if (irq > 0) {
ret = devm_request_irq(&pdev->dev, irq, i2s_irq_handler, 0,
pdev->name, dev);
if (ret < 0) {
@@ -697,7 +697,7 @@ static int dw_i2s_probe(struct platform_device *pdev)
}
if (!pdata) {
- if (irq >= 0) {
+ if (irq > 0) {
ret = dw_pcm_register(pdev);
dev->use_pio = true;
} else {
--
2.26.3
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-12 20:16 ` Sergey Shtylyov
0 siblings, 0 replies; 44+ messages in thread
From: Sergey Shtylyov @ 2022-02-12 20:16 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel
Cc: Andy Shevchenko, Corey Minyard, Oleksij Rempel,
Pengutronix Kernel Team, William Breathitt Gray, Mun Yew Tham,
Linus Walleij, Bartosz Golaszewski, Thierry Reding,
Uwe Kleine-König, Lee Jones, Kamal Dasu, Florian Fainelli,
bcm-kernel-feedback-list, Peter Korsgaard, Andrew Lunn,
Ulf Hansson, Brian Norris, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, David S. Miller, Jakub Kicinski,
Joakim Zhang, Yoshihiro Shimoda, Kishon Vijay Abraham I,
Vinod Koul, Benson Leung, Guenter Roeck, Zha Qipeng,
Hans de Goede, Mark Gross, John Garry, Mark Brown,
Matthias Brugger, Niklas Söderlund, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Jiri Slaby, Eric Auger, Alex Williamson,
Cornelia Huck, Liam Girdwood, Jaroslav Kysela, Takashi Iwai,
openipmi-developer, linux-iio, linux-gpio, linux-pwm, linux-i2c,
linux-arm-kernel, linux-mmc, linux-mtd, netdev, linux-renesas-soc,
linux-phy, platform-driver-x86, linux-spi, linux-mediatek,
linux-pm, linux-serial, kvm, alsa-devel, Matthias Schiffer
This patch is based on the former Andy Shevchenko's patch:
https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
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@ew.tq-group.com>
Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
Changes in version 2:
- added the error check using dev_err_probe() to platform_get_irq_optional();
- fixed up the IRQ checks in 'drivers/char/ipmi/bt-bmc.c';
- fixed up the IRQ check in 'drivers/thermal/rcar_gen3_thermal.c';
- removed from the patch 'drivers/edac/xgene_edac.c and
'drivers/power/supply/mp2629_charger.c' as they were switched to calling
platform_get_irq();
- shortened the IRQ checking code in 'drivers/vfio/platform/vfio_platform.c';
- rebased atop of the recent platform_get_irq_byname() patch;
- reformatted the patch description.
drivers/base/platform.c | 60 +++++++++++++++---------
drivers/char/ipmi/bt-bmc.c | 6 +--
drivers/counter/interrupt-cnt.c | 4 +-
drivers/gpio/gpio-altera.c | 3 +-
drivers/gpio/gpio-mvebu.c | 2 +-
drivers/gpio/gpio-tqmx86.c | 2 +-
drivers/i2c/busses/i2c-brcmstb.c | 8 ++--
drivers/i2c/busses/i2c-ocores.c | 4 +-
drivers/mmc/host/sh_mmcif.c | 4 +-
drivers/mtd/nand/raw/brcmnand/brcmnand.c | 4 +-
drivers/net/ethernet/davicom/dm9000.c | 2 +-
drivers/net/ethernet/freescale/fec_ptp.c | 2 +-
drivers/phy/renesas/phy-rcar-gen3-usb2.c | 4 +-
drivers/platform/chrome/cros_ec_lpc.c | 2 +-
drivers/platform/x86/intel/punit_ipc.c | 2 +-
drivers/spi/spi-hisi-sfc-v3xx.c | 2 +-
drivers/spi/spi-mtk-nor.c | 3 +-
drivers/thermal/rcar_gen3_thermal.c | 2 +
drivers/tty/serial/8250/8250_mtk.c | 4 +-
drivers/tty/serial/sh-sci.c | 6 +--
drivers/uio/uio_pdrv_genirq.c | 2 +-
drivers/vfio/platform/vfio_platform.c | 3 +-
sound/soc/dwc/dwc-i2s.c | 4 +-
23 files changed, 76 insertions(+), 59 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 7d08cd8947be..52a8356f8261 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -148,25 +148,7 @@ devm_platform_ioremap_resource_byname(struct platform_device *pdev,
EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource_byname);
#endif /* CONFIG_HAS_IOMEM */
-/**
- * platform_get_irq_optional - get an optional IRQ for a device
- * @dev: platform device
- * @num: IRQ number index
- *
- * Gets an IRQ for a platform device. Device drivers should check the return
- * value for errors so as to not pass a negative integer value to the
- * request_irq() APIs. This is the same as platform_get_irq(), except that it
- * does not print an error message if an IRQ can not be obtained.
- *
- * For example::
- *
- * int irq = platform_get_irq_optional(pdev, 0);
- * if (irq < 0)
- * return irq;
- *
- * Return: non-zero IRQ number on success, negative error number on failure.
- */
-int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
+static int __platform_get_irq(struct platform_device *dev, unsigned int num)
{
int ret;
#ifdef CONFIG_SPARC
@@ -235,6 +217,42 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
return -EINVAL;
return ret;
}
+
+/**
+ * platform_get_irq_optional - get an optional IRQ for a device
+ * @dev: platform device
+ * @num: IRQ number index
+ *
+ * Gets an IRQ for a platform device. Device drivers should check the return
+ * value for errors so as to not pass a negative integer value to the
+ * request_irq() APIs. This is the same as platform_get_irq(), except that it
+ * does not print an error message if an IRQ can not be obtained and returns
+ * 0 when IRQ resource has not been found.
+ *
+ * For example::
+ *
+ * int irq = platform_get_irq_optional(pdev, 0);
+ * if (irq < 0)
+ * return irq;
+ * if (irq > 0)
+ * ...we have IRQ line defined...
+ *
+ * Return: non-zero IRQ number on success, 0 if IRQ wasn't found, negative error
+ * number on failure.
+ */
+int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
+{
+ int ret;
+
+ ret = __platform_get_irq(dev, num);
+ if (ret == -ENXIO)
+ return 0;
+ if (ret < 0)
+ return dev_err_probe(&dev->dev, ret,
+ "IRQ index %u not found\n", num);
+
+ return ret;
+}
EXPORT_SYMBOL_GPL(platform_get_irq_optional);
/**
@@ -258,7 +276,7 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
{
int ret;
- ret = platform_get_irq_optional(dev, num);
+ ret = __platform_get_irq(dev, num);
if (ret < 0)
return dev_err_probe(&dev->dev, ret,
"IRQ index %u not found\n", num);
@@ -277,7 +295,7 @@ int platform_irq_count(struct platform_device *dev)
{
int ret, nr = 0;
- while ((ret = platform_get_irq_optional(dev, nr)) >= 0)
+ while ((ret = __platform_get_irq(dev, nr)) >= 0)
nr++;
if (ret == -EPROBE_DEFER)
diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c
index 7450904e330a..289acaf4e720 100644
--- a/drivers/char/ipmi/bt-bmc.c
+++ b/drivers/char/ipmi/bt-bmc.c
@@ -380,7 +380,7 @@ static int bt_bmc_config_irq(struct bt_bmc *bt_bmc,
u32 reg;
bt_bmc->irq = platform_get_irq_optional(pdev, 0);
- if (bt_bmc->irq < 0)
+ if (bt_bmc->irq <= 0)
return bt_bmc->irq;
rc = devm_request_irq(dev, bt_bmc->irq, bt_bmc_irq, IRQF_SHARED,
@@ -438,7 +438,7 @@ static int bt_bmc_probe(struct platform_device *pdev)
bt_bmc_config_irq(bt_bmc, pdev);
- if (bt_bmc->irq >= 0) {
+ if (bt_bmc->irq > 0) {
dev_info(dev, "Using IRQ %d\n", bt_bmc->irq);
} else {
dev_info(dev, "No IRQ; using timer\n");
@@ -464,7 +464,7 @@ static int bt_bmc_remove(struct platform_device *pdev)
struct bt_bmc *bt_bmc = dev_get_drvdata(&pdev->dev);
misc_deregister(&bt_bmc->miscdev);
- if (bt_bmc->irq < 0)
+ if (bt_bmc->irq <= 0)
del_timer_sync(&bt_bmc->poll_timer);
return 0;
}
diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
index 9e99702470c2..a2443c66330b 100644
--- a/drivers/counter/interrupt-cnt.c
+++ b/drivers/counter/interrupt-cnt.c
@@ -157,9 +157,7 @@ static int interrupt_cnt_probe(struct platform_device *pdev)
priv = counter_priv(counter);
priv->irq = platform_get_irq_optional(pdev, 0);
- if (priv->irq == -ENXIO)
- priv->irq = 0;
- else if (priv->irq < 0)
+ if (priv->irq < 0)
return dev_err_probe(dev, priv->irq, "failed to get IRQ\n");
priv->gpio = devm_gpiod_get_optional(dev, NULL, GPIOD_IN);
diff --git a/drivers/gpio/gpio-altera.c b/drivers/gpio/gpio-altera.c
index b59fae993626..02a2995aa368 100644
--- a/drivers/gpio/gpio-altera.c
+++ b/drivers/gpio/gpio-altera.c
@@ -267,8 +267,7 @@ static int altera_gpio_probe(struct platform_device *pdev)
altera_gc->mmchip.gc.parent = &pdev->dev;
altera_gc->mapped_irq = platform_get_irq_optional(pdev, 0);
-
- if (altera_gc->mapped_irq < 0)
+ if (altera_gc->mapped_irq <= 0)
goto skip_irq;
if (of_property_read_u32(node, "altr,interrupt-type", ®)) {
diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index 4c1f9e1091b7..d94594807697 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -1293,7 +1293,7 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
for (i = 0; i < 4; i++) {
int irq = platform_get_irq_optional(pdev, i);
- if (irq < 0)
+ if (irq <= 0)
continue;
irq_set_chained_handler_and_data(irq, mvebu_gpio_irq_handler,
mvchip);
diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
index 5b103221b58d..dc0f83236ce8 100644
--- a/drivers/gpio/gpio-tqmx86.c
+++ b/drivers/gpio/gpio-tqmx86.c
@@ -237,7 +237,7 @@ static int tqmx86_gpio_probe(struct platform_device *pdev)
int ret, irq;
irq = platform_get_irq_optional(pdev, 0);
- if (irq < 0 && irq != -ENXIO)
+ if (irq < 0)
return irq;
res = platform_get_resource(pdev, IORESOURCE_IO, 0);
diff --git a/drivers/i2c/busses/i2c-brcmstb.c b/drivers/i2c/busses/i2c-brcmstb.c
index 490ee3962645..69395ae27a1a 100644
--- a/drivers/i2c/busses/i2c-brcmstb.c
+++ b/drivers/i2c/busses/i2c-brcmstb.c
@@ -250,7 +250,7 @@ static int brcmstb_i2c_wait_for_completion(struct brcmstb_i2c_dev *dev)
int ret = 0;
unsigned long timeout = msecs_to_jiffies(I2C_TIMEOUT);
- if (dev->irq >= 0) {
+ if (dev->irq > 0) {
if (!wait_for_completion_timeout(&dev->done, timeout))
ret = -ETIMEDOUT;
} else {
@@ -297,7 +297,7 @@ static int brcmstb_send_i2c_cmd(struct brcmstb_i2c_dev *dev,
return rc;
/* only if we are in interrupt mode */
- if (dev->irq >= 0)
+ if (dev->irq > 0)
reinit_completion(&dev->done);
/* enable BSC CTL interrupt line */
@@ -652,7 +652,7 @@ static int brcmstb_i2c_probe(struct platform_device *pdev)
brcmstb_i2c_enable_disable_irq(dev, INT_DISABLE);
/* register the ISR handler */
- if (dev->irq >= 0) {
+ if (dev->irq > 0) {
rc = devm_request_irq(&pdev->dev, dev->irq, brcmstb_i2c_isr,
IRQF_SHARED,
int_name ? int_name : pdev->name,
@@ -696,7 +696,7 @@ static int brcmstb_i2c_probe(struct platform_device *pdev)
dev_info(dev->device, "%s@%dhz registered in %s mode\n",
int_name ? int_name : " ", dev->clk_freq_hz,
- (dev->irq >= 0) ? "interrupt" : "polling");
+ (dev->irq > 0) ? "interrupt" : "polling");
return 0;
diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index a0af027db04c..1f4d5e52ff42 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -691,10 +691,10 @@ static int ocores_i2c_probe(struct platform_device *pdev)
if (of_device_is_compatible(pdev->dev.of_node,
"sifive,fu540-c000-i2c")) {
i2c->flags |= OCORES_FLAG_BROKEN_IRQ;
- irq = -ENXIO;
+ irq = 0;
}
- if (irq == -ENXIO) {
+ if (!irq) {
ocores_algorithm.master_xfer = ocores_xfer_polling;
} else {
if (irq < 0)
diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index bcc595c70a9f..f558b9862032 100644
--- 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";
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) {
ret = devm_request_threaded_irq(dev, irq[1],
sh_mmcif_intr, sh_mmcif_irqt,
0, "sh_mmc:int", host);
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index f75929783b94..ac222985efde 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -1521,7 +1521,7 @@ static irqreturn_t brcmnand_ctlrdy_irq(int irq, void *data)
/* check if you need to piggy back on the ctrlrdy irq */
if (ctrl->edu_pending) {
- if (irq == ctrl->irq && ((int)ctrl->edu_irq >= 0))
+ if (irq == ctrl->irq && ((int)ctrl->edu_irq > 0))
/* Discard interrupts while using dedicated edu irq */
return IRQ_HANDLED;
@@ -2956,7 +2956,7 @@ static int brcmnand_edu_setup(struct platform_device *pdev)
brcmnand_edu_init(ctrl);
ctrl->edu_irq = platform_get_irq_optional(pdev, 1);
- if (ctrl->edu_irq < 0) {
+ if (ctrl->edu_irq <= 0) {
dev_warn(dev,
"FLASH EDU enabled, using ctlrdy irq\n");
} else {
diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
index 0985ab216566..740c660a9411 100644
--- a/drivers/net/ethernet/davicom/dm9000.c
+++ b/drivers/net/ethernet/davicom/dm9000.c
@@ -1509,7 +1509,7 @@ dm9000_probe(struct platform_device *pdev)
}
db->irq_wake = platform_get_irq_optional(pdev, 1);
- if (db->irq_wake >= 0) {
+ if (db->irq_wake > 0) {
dev_dbg(db->dev, "wakeup irq %d\n", db->irq_wake);
ret = request_irq(db->irq_wake, dm9000_wol_interrupt,
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index af99017a5453..de1d23808b6c 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -616,7 +616,7 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
/* Failure to get an irq is not fatal,
* only the PTP_CLOCK_PPS clock events should stop
*/
- if (irq >= 0) {
+ if (irq > 0) {
ret = devm_request_irq(&pdev->dev, irq, fec_pps_interrupt,
0, pdev->name, ndev);
if (ret < 0)
diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
index 9de617ca9daa..4914d6aca208 100644
--- 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;
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index d6306d2a096f..91686d306534 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -400,7 +400,7 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
irq = platform_get_irq_optional(pdev, 0);
if (irq > 0)
ec_dev->irq = irq;
- else if (irq != -ENXIO) {
+ else if (irq < 0) {
dev_err(dev, "couldn't retrieve IRQ number (%d)\n", irq);
return irq;
}
diff --git a/drivers/platform/x86/intel/punit_ipc.c b/drivers/platform/x86/intel/punit_ipc.c
index 66bb39fd0ef9..f3cf5ee1466f 100644
--- a/drivers/platform/x86/intel/punit_ipc.c
+++ b/drivers/platform/x86/intel/punit_ipc.c
@@ -278,7 +278,7 @@ static int intel_punit_ipc_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, punit_ipcdev);
irq = platform_get_irq_optional(pdev, 0);
- if (irq < 0) {
+ if (irq <= 0) {
dev_warn(&pdev->dev, "Invalid IRQ, using polling mode\n");
} else {
ret = devm_request_irq(&pdev->dev, irq, intel_punit_ioc,
diff --git a/drivers/spi/spi-hisi-sfc-v3xx.c b/drivers/spi/spi-hisi-sfc-v3xx.c
index d3a23b1c2a4c..476ddc081c60 100644
--- a/drivers/spi/spi-hisi-sfc-v3xx.c
+++ b/drivers/spi/spi-hisi-sfc-v3xx.c
@@ -467,7 +467,7 @@ static int hisi_sfc_v3xx_probe(struct platform_device *pdev)
dev_err(dev, "failed to request irq%d, ret = %d\n", host->irq, ret);
host->irq = 0;
}
- } else {
+ } else if (host->irq < 0) {
host->irq = 0;
}
diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
index 5c93730615f8..2422b0545936 100644
--- a/drivers/spi/spi-mtk-nor.c
+++ b/drivers/spi/spi-mtk-nor.c
@@ -829,8 +829,7 @@ static int mtk_nor_probe(struct platform_device *pdev)
mtk_nor_init(sp);
irq = platform_get_irq_optional(pdev, 0);
-
- if (irq < 0) {
+ if (irq <= 0) {
dev_warn(sp->dev, "IRQ not available.");
} else {
ret = devm_request_irq(sp->dev, irq, mtk_nor_irq_handler, 0,
diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 43eb25b167bc..359b9941c42b 100644
--- 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;
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/8250/8250_mtk.c
+++ b/drivers/tty/serial/8250/8250_mtk.c
@@ -621,7 +621,7 @@ static int __maybe_unused mtk8250_suspend(struct device *dev)
serial8250_suspend_port(data->line);
pinctrl_pm_select_sleep_state(dev);
- if (irq >= 0) {
+ if (irq > 0) {
err = enable_irq_wake(irq);
if (err) {
dev_err(dev,
@@ -641,7 +641,7 @@ static int __maybe_unused mtk8250_resume(struct device *dev)
struct mtk8250_data *data = dev_get_drvdata(dev);
int irq = data->rx_wakeup_irq;
- if (irq >= 0)
+ if (irq > 0)
disable_irq_wake(irq);
pinctrl_pm_select_default_state(dev);
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 968967d722d4..96c3146ff6a4 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -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];
diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
index 63258b6accc4..7fd275fc6ceb 100644
--- a/drivers/uio/uio_pdrv_genirq.c
+++ b/drivers/uio/uio_pdrv_genirq.c
@@ -162,7 +162,7 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
if (!uioinfo->irq) {
ret = platform_get_irq_optional(pdev, 0);
uioinfo->irq = ret;
- if (ret == -ENXIO)
+ if (!ret)
uioinfo->irq = UIO_IRQ_NONE;
else if (ret == -EPROBE_DEFER)
return ret;
diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
index 68a1c87066d7..f7423d10cefd 100644
--- a/drivers/vfio/platform/vfio_platform.c
+++ b/drivers/vfio/platform/vfio_platform.c
@@ -32,8 +32,9 @@ static struct resource *get_platform_resource(struct vfio_platform_device *vdev,
static int get_platform_irq(struct vfio_platform_device *vdev, int i)
{
struct platform_device *pdev = (struct platform_device *) vdev->opaque;
+ int ret = platform_get_irq_optional(pdev, i);
- return platform_get_irq_optional(pdev, i);
+ return ret ? ret : -ENXIO;
}
static int vfio_platform_probe(struct platform_device *pdev)
diff --git a/sound/soc/dwc/dwc-i2s.c b/sound/soc/dwc/dwc-i2s.c
index 5cb58929090d..ff19c5130459 100644
--- a/sound/soc/dwc/dwc-i2s.c
+++ b/sound/soc/dwc/dwc-i2s.c
@@ -643,7 +643,7 @@ static int dw_i2s_probe(struct platform_device *pdev)
dev->dev = &pdev->dev;
irq = platform_get_irq_optional(pdev, 0);
- if (irq >= 0) {
+ if (irq > 0) {
ret = devm_request_irq(&pdev->dev, irq, i2s_irq_handler, 0,
pdev->name, dev);
if (ret < 0) {
@@ -697,7 +697,7 @@ static int dw_i2s_probe(struct platform_device *pdev)
}
if (!pdata) {
- if (irq >= 0) {
+ if (irq > 0) {
ret = dw_pcm_register(pdev);
dev->use_pio = true;
} else {
--
2.26.3
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-12 20:16 ` Sergey Shtylyov
0 siblings, 0 replies; 44+ messages in thread
From: Sergey Shtylyov @ 2022-02-12 20:16 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel
Cc: Andy Shevchenko, Corey Minyard, Oleksij Rempel,
Pengutronix Kernel Team, William Breathitt Gray, Mun Yew Tham,
Linus Walleij, Bartosz Golaszewski, Thierry Reding,
Uwe Kleine-König, Lee Jones, Kamal Dasu, Florian Fainelli,
bcm-kernel-feedback-list, Peter Korsgaard, Andrew Lunn,
Ulf Hansson, Brian Norris, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, David S. Miller, Jakub Kicinski,
Joakim Zhang, Yoshihiro Shimoda, Kishon Vijay Abraham I,
Vinod Koul, Benson Leung, Guenter Roeck, Zha Qipeng,
Hans de Goede, Mark Gross, John Garry, Mark Brown,
Matthias Brugger, Niklas Söderlund, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Jiri Slaby, Eric Auger, Alex Williamson,
Cornelia Huck, Liam Girdwood, Jaroslav Kysela, Takashi Iwai,
openipmi-developer, linux-iio, linux-gpio, linux-pwm, linux-i2c,
linux-arm-kernel, linux-mmc, linux-mtd, netdev, linux-renesas-soc,
linux-phy, platform-driver-x86, linux-spi, linux-mediatek,
linux-pm, linux-serial, kvm, alsa-devel, Matthias Schiffer
This patch is based on the former Andy Shevchenko's patch:
https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
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@ew.tq-group.com>
Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
Changes in version 2:
- added the error check using dev_err_probe() to platform_get_irq_optional();
- fixed up the IRQ checks in 'drivers/char/ipmi/bt-bmc.c';
- fixed up the IRQ check in 'drivers/thermal/rcar_gen3_thermal.c';
- removed from the patch 'drivers/edac/xgene_edac.c and
'drivers/power/supply/mp2629_charger.c' as they were switched to calling
platform_get_irq();
- shortened the IRQ checking code in 'drivers/vfio/platform/vfio_platform.c';
- rebased atop of the recent platform_get_irq_byname() patch;
- reformatted the patch description.
drivers/base/platform.c | 60 +++++++++++++++---------
drivers/char/ipmi/bt-bmc.c | 6 +--
drivers/counter/interrupt-cnt.c | 4 +-
drivers/gpio/gpio-altera.c | 3 +-
drivers/gpio/gpio-mvebu.c | 2 +-
drivers/gpio/gpio-tqmx86.c | 2 +-
drivers/i2c/busses/i2c-brcmstb.c | 8 ++--
drivers/i2c/busses/i2c-ocores.c | 4 +-
drivers/mmc/host/sh_mmcif.c | 4 +-
drivers/mtd/nand/raw/brcmnand/brcmnand.c | 4 +-
drivers/net/ethernet/davicom/dm9000.c | 2 +-
drivers/net/ethernet/freescale/fec_ptp.c | 2 +-
drivers/phy/renesas/phy-rcar-gen3-usb2.c | 4 +-
drivers/platform/chrome/cros_ec_lpc.c | 2 +-
drivers/platform/x86/intel/punit_ipc.c | 2 +-
drivers/spi/spi-hisi-sfc-v3xx.c | 2 +-
drivers/spi/spi-mtk-nor.c | 3 +-
drivers/thermal/rcar_gen3_thermal.c | 2 +
drivers/tty/serial/8250/8250_mtk.c | 4 +-
drivers/tty/serial/sh-sci.c | 6 +--
drivers/uio/uio_pdrv_genirq.c | 2 +-
drivers/vfio/platform/vfio_platform.c | 3 +-
sound/soc/dwc/dwc-i2s.c | 4 +-
23 files changed, 76 insertions(+), 59 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 7d08cd8947be..52a8356f8261 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -148,25 +148,7 @@ devm_platform_ioremap_resource_byname(struct platform_device *pdev,
EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource_byname);
#endif /* CONFIG_HAS_IOMEM */
-/**
- * platform_get_irq_optional - get an optional IRQ for a device
- * @dev: platform device
- * @num: IRQ number index
- *
- * Gets an IRQ for a platform device. Device drivers should check the return
- * value for errors so as to not pass a negative integer value to the
- * request_irq() APIs. This is the same as platform_get_irq(), except that it
- * does not print an error message if an IRQ can not be obtained.
- *
- * For example::
- *
- * int irq = platform_get_irq_optional(pdev, 0);
- * if (irq < 0)
- * return irq;
- *
- * Return: non-zero IRQ number on success, negative error number on failure.
- */
-int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
+static int __platform_get_irq(struct platform_device *dev, unsigned int num)
{
int ret;
#ifdef CONFIG_SPARC
@@ -235,6 +217,42 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
return -EINVAL;
return ret;
}
+
+/**
+ * platform_get_irq_optional - get an optional IRQ for a device
+ * @dev: platform device
+ * @num: IRQ number index
+ *
+ * Gets an IRQ for a platform device. Device drivers should check the return
+ * value for errors so as to not pass a negative integer value to the
+ * request_irq() APIs. This is the same as platform_get_irq(), except that it
+ * does not print an error message if an IRQ can not be obtained and returns
+ * 0 when IRQ resource has not been found.
+ *
+ * For example::
+ *
+ * int irq = platform_get_irq_optional(pdev, 0);
+ * if (irq < 0)
+ * return irq;
+ * if (irq > 0)
+ * ...we have IRQ line defined...
+ *
+ * Return: non-zero IRQ number on success, 0 if IRQ wasn't found, negative error
+ * number on failure.
+ */
+int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
+{
+ int ret;
+
+ ret = __platform_get_irq(dev, num);
+ if (ret == -ENXIO)
+ return 0;
+ if (ret < 0)
+ return dev_err_probe(&dev->dev, ret,
+ "IRQ index %u not found\n", num);
+
+ return ret;
+}
EXPORT_SYMBOL_GPL(platform_get_irq_optional);
/**
@@ -258,7 +276,7 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
{
int ret;
- ret = platform_get_irq_optional(dev, num);
+ ret = __platform_get_irq(dev, num);
if (ret < 0)
return dev_err_probe(&dev->dev, ret,
"IRQ index %u not found\n", num);
@@ -277,7 +295,7 @@ int platform_irq_count(struct platform_device *dev)
{
int ret, nr = 0;
- while ((ret = platform_get_irq_optional(dev, nr)) >= 0)
+ while ((ret = __platform_get_irq(dev, nr)) >= 0)
nr++;
if (ret == -EPROBE_DEFER)
diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c
index 7450904e330a..289acaf4e720 100644
--- a/drivers/char/ipmi/bt-bmc.c
+++ b/drivers/char/ipmi/bt-bmc.c
@@ -380,7 +380,7 @@ static int bt_bmc_config_irq(struct bt_bmc *bt_bmc,
u32 reg;
bt_bmc->irq = platform_get_irq_optional(pdev, 0);
- if (bt_bmc->irq < 0)
+ if (bt_bmc->irq <= 0)
return bt_bmc->irq;
rc = devm_request_irq(dev, bt_bmc->irq, bt_bmc_irq, IRQF_SHARED,
@@ -438,7 +438,7 @@ static int bt_bmc_probe(struct platform_device *pdev)
bt_bmc_config_irq(bt_bmc, pdev);
- if (bt_bmc->irq >= 0) {
+ if (bt_bmc->irq > 0) {
dev_info(dev, "Using IRQ %d\n", bt_bmc->irq);
} else {
dev_info(dev, "No IRQ; using timer\n");
@@ -464,7 +464,7 @@ static int bt_bmc_remove(struct platform_device *pdev)
struct bt_bmc *bt_bmc = dev_get_drvdata(&pdev->dev);
misc_deregister(&bt_bmc->miscdev);
- if (bt_bmc->irq < 0)
+ if (bt_bmc->irq <= 0)
del_timer_sync(&bt_bmc->poll_timer);
return 0;
}
diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
index 9e99702470c2..a2443c66330b 100644
--- a/drivers/counter/interrupt-cnt.c
+++ b/drivers/counter/interrupt-cnt.c
@@ -157,9 +157,7 @@ static int interrupt_cnt_probe(struct platform_device *pdev)
priv = counter_priv(counter);
priv->irq = platform_get_irq_optional(pdev, 0);
- if (priv->irq == -ENXIO)
- priv->irq = 0;
- else if (priv->irq < 0)
+ if (priv->irq < 0)
return dev_err_probe(dev, priv->irq, "failed to get IRQ\n");
priv->gpio = devm_gpiod_get_optional(dev, NULL, GPIOD_IN);
diff --git a/drivers/gpio/gpio-altera.c b/drivers/gpio/gpio-altera.c
index b59fae993626..02a2995aa368 100644
--- a/drivers/gpio/gpio-altera.c
+++ b/drivers/gpio/gpio-altera.c
@@ -267,8 +267,7 @@ static int altera_gpio_probe(struct platform_device *pdev)
altera_gc->mmchip.gc.parent = &pdev->dev;
altera_gc->mapped_irq = platform_get_irq_optional(pdev, 0);
-
- if (altera_gc->mapped_irq < 0)
+ if (altera_gc->mapped_irq <= 0)
goto skip_irq;
if (of_property_read_u32(node, "altr,interrupt-type", ®)) {
diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index 4c1f9e1091b7..d94594807697 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -1293,7 +1293,7 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
for (i = 0; i < 4; i++) {
int irq = platform_get_irq_optional(pdev, i);
- if (irq < 0)
+ if (irq <= 0)
continue;
irq_set_chained_handler_and_data(irq, mvebu_gpio_irq_handler,
mvchip);
diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
index 5b103221b58d..dc0f83236ce8 100644
--- a/drivers/gpio/gpio-tqmx86.c
+++ b/drivers/gpio/gpio-tqmx86.c
@@ -237,7 +237,7 @@ static int tqmx86_gpio_probe(struct platform_device *pdev)
int ret, irq;
irq = platform_get_irq_optional(pdev, 0);
- if (irq < 0 && irq != -ENXIO)
+ if (irq < 0)
return irq;
res = platform_get_resource(pdev, IORESOURCE_IO, 0);
diff --git a/drivers/i2c/busses/i2c-brcmstb.c b/drivers/i2c/busses/i2c-brcmstb.c
index 490ee3962645..69395ae27a1a 100644
--- a/drivers/i2c/busses/i2c-brcmstb.c
+++ b/drivers/i2c/busses/i2c-brcmstb.c
@@ -250,7 +250,7 @@ static int brcmstb_i2c_wait_for_completion(struct brcmstb_i2c_dev *dev)
int ret = 0;
unsigned long timeout = msecs_to_jiffies(I2C_TIMEOUT);
- if (dev->irq >= 0) {
+ if (dev->irq > 0) {
if (!wait_for_completion_timeout(&dev->done, timeout))
ret = -ETIMEDOUT;
} else {
@@ -297,7 +297,7 @@ static int brcmstb_send_i2c_cmd(struct brcmstb_i2c_dev *dev,
return rc;
/* only if we are in interrupt mode */
- if (dev->irq >= 0)
+ if (dev->irq > 0)
reinit_completion(&dev->done);
/* enable BSC CTL interrupt line */
@@ -652,7 +652,7 @@ static int brcmstb_i2c_probe(struct platform_device *pdev)
brcmstb_i2c_enable_disable_irq(dev, INT_DISABLE);
/* register the ISR handler */
- if (dev->irq >= 0) {
+ if (dev->irq > 0) {
rc = devm_request_irq(&pdev->dev, dev->irq, brcmstb_i2c_isr,
IRQF_SHARED,
int_name ? int_name : pdev->name,
@@ -696,7 +696,7 @@ static int brcmstb_i2c_probe(struct platform_device *pdev)
dev_info(dev->device, "%s@%dhz registered in %s mode\n",
int_name ? int_name : " ", dev->clk_freq_hz,
- (dev->irq >= 0) ? "interrupt" : "polling");
+ (dev->irq > 0) ? "interrupt" : "polling");
return 0;
diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index a0af027db04c..1f4d5e52ff42 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -691,10 +691,10 @@ static int ocores_i2c_probe(struct platform_device *pdev)
if (of_device_is_compatible(pdev->dev.of_node,
"sifive,fu540-c000-i2c")) {
i2c->flags |= OCORES_FLAG_BROKEN_IRQ;
- irq = -ENXIO;
+ irq = 0;
}
- if (irq == -ENXIO) {
+ if (!irq) {
ocores_algorithm.master_xfer = ocores_xfer_polling;
} else {
if (irq < 0)
diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index bcc595c70a9f..f558b9862032 100644
--- 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";
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) {
ret = devm_request_threaded_irq(dev, irq[1],
sh_mmcif_intr, sh_mmcif_irqt,
0, "sh_mmc:int", host);
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index f75929783b94..ac222985efde 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -1521,7 +1521,7 @@ static irqreturn_t brcmnand_ctlrdy_irq(int irq, void *data)
/* check if you need to piggy back on the ctrlrdy irq */
if (ctrl->edu_pending) {
- if (irq == ctrl->irq && ((int)ctrl->edu_irq >= 0))
+ if (irq == ctrl->irq && ((int)ctrl->edu_irq > 0))
/* Discard interrupts while using dedicated edu irq */
return IRQ_HANDLED;
@@ -2956,7 +2956,7 @@ static int brcmnand_edu_setup(struct platform_device *pdev)
brcmnand_edu_init(ctrl);
ctrl->edu_irq = platform_get_irq_optional(pdev, 1);
- if (ctrl->edu_irq < 0) {
+ if (ctrl->edu_irq <= 0) {
dev_warn(dev,
"FLASH EDU enabled, using ctlrdy irq\n");
} else {
diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
index 0985ab216566..740c660a9411 100644
--- a/drivers/net/ethernet/davicom/dm9000.c
+++ b/drivers/net/ethernet/davicom/dm9000.c
@@ -1509,7 +1509,7 @@ dm9000_probe(struct platform_device *pdev)
}
db->irq_wake = platform_get_irq_optional(pdev, 1);
- if (db->irq_wake >= 0) {
+ if (db->irq_wake > 0) {
dev_dbg(db->dev, "wakeup irq %d\n", db->irq_wake);
ret = request_irq(db->irq_wake, dm9000_wol_interrupt,
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index af99017a5453..de1d23808b6c 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -616,7 +616,7 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
/* Failure to get an irq is not fatal,
* only the PTP_CLOCK_PPS clock events should stop
*/
- if (irq >= 0) {
+ if (irq > 0) {
ret = devm_request_irq(&pdev->dev, irq, fec_pps_interrupt,
0, pdev->name, ndev);
if (ret < 0)
diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
index 9de617ca9daa..4914d6aca208 100644
--- 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;
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index d6306d2a096f..91686d306534 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -400,7 +400,7 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
irq = platform_get_irq_optional(pdev, 0);
if (irq > 0)
ec_dev->irq = irq;
- else if (irq != -ENXIO) {
+ else if (irq < 0) {
dev_err(dev, "couldn't retrieve IRQ number (%d)\n", irq);
return irq;
}
diff --git a/drivers/platform/x86/intel/punit_ipc.c b/drivers/platform/x86/intel/punit_ipc.c
index 66bb39fd0ef9..f3cf5ee1466f 100644
--- a/drivers/platform/x86/intel/punit_ipc.c
+++ b/drivers/platform/x86/intel/punit_ipc.c
@@ -278,7 +278,7 @@ static int intel_punit_ipc_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, punit_ipcdev);
irq = platform_get_irq_optional(pdev, 0);
- if (irq < 0) {
+ if (irq <= 0) {
dev_warn(&pdev->dev, "Invalid IRQ, using polling mode\n");
} else {
ret = devm_request_irq(&pdev->dev, irq, intel_punit_ioc,
diff --git a/drivers/spi/spi-hisi-sfc-v3xx.c b/drivers/spi/spi-hisi-sfc-v3xx.c
index d3a23b1c2a4c..476ddc081c60 100644
--- a/drivers/spi/spi-hisi-sfc-v3xx.c
+++ b/drivers/spi/spi-hisi-sfc-v3xx.c
@@ -467,7 +467,7 @@ static int hisi_sfc_v3xx_probe(struct platform_device *pdev)
dev_err(dev, "failed to request irq%d, ret = %d\n", host->irq, ret);
host->irq = 0;
}
- } else {
+ } else if (host->irq < 0) {
host->irq = 0;
}
diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
index 5c93730615f8..2422b0545936 100644
--- a/drivers/spi/spi-mtk-nor.c
+++ b/drivers/spi/spi-mtk-nor.c
@@ -829,8 +829,7 @@ static int mtk_nor_probe(struct platform_device *pdev)
mtk_nor_init(sp);
irq = platform_get_irq_optional(pdev, 0);
-
- if (irq < 0) {
+ if (irq <= 0) {
dev_warn(sp->dev, "IRQ not available.");
} else {
ret = devm_request_irq(sp->dev, irq, mtk_nor_irq_handler, 0,
diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 43eb25b167bc..359b9941c42b 100644
--- 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;
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/8250/8250_mtk.c
+++ b/drivers/tty/serial/8250/8250_mtk.c
@@ -621,7 +621,7 @@ static int __maybe_unused mtk8250_suspend(struct device *dev)
serial8250_suspend_port(data->line);
pinctrl_pm_select_sleep_state(dev);
- if (irq >= 0) {
+ if (irq > 0) {
err = enable_irq_wake(irq);
if (err) {
dev_err(dev,
@@ -641,7 +641,7 @@ static int __maybe_unused mtk8250_resume(struct device *dev)
struct mtk8250_data *data = dev_get_drvdata(dev);
int irq = data->rx_wakeup_irq;
- if (irq >= 0)
+ if (irq > 0)
disable_irq_wake(irq);
pinctrl_pm_select_default_state(dev);
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 968967d722d4..96c3146ff6a4 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -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];
diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
index 63258b6accc4..7fd275fc6ceb 100644
--- a/drivers/uio/uio_pdrv_genirq.c
+++ b/drivers/uio/uio_pdrv_genirq.c
@@ -162,7 +162,7 @@ static int uio_pdrv_genirq_probe(struct platform_device *pdev)
if (!uioinfo->irq) {
ret = platform_get_irq_optional(pdev, 0);
uioinfo->irq = ret;
- if (ret == -ENXIO)
+ if (!ret)
uioinfo->irq = UIO_IRQ_NONE;
else if (ret == -EPROBE_DEFER)
return ret;
diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
index 68a1c87066d7..f7423d10cefd 100644
--- a/drivers/vfio/platform/vfio_platform.c
+++ b/drivers/vfio/platform/vfio_platform.c
@@ -32,8 +32,9 @@ static struct resource *get_platform_resource(struct vfio_platform_device *vdev,
static int get_platform_irq(struct vfio_platform_device *vdev, int i)
{
struct platform_device *pdev = (struct platform_device *) vdev->opaque;
+ int ret = platform_get_irq_optional(pdev, i);
- return platform_get_irq_optional(pdev, i);
+ return ret ? ret : -ENXIO;
}
static int vfio_platform_probe(struct platform_device *pdev)
diff --git a/sound/soc/dwc/dwc-i2s.c b/sound/soc/dwc/dwc-i2s.c
index 5cb58929090d..ff19c5130459 100644
--- a/sound/soc/dwc/dwc-i2s.c
+++ b/sound/soc/dwc/dwc-i2s.c
@@ -643,7 +643,7 @@ static int dw_i2s_probe(struct platform_device *pdev)
dev->dev = &pdev->dev;
irq = platform_get_irq_optional(pdev, 0);
- if (irq >= 0) {
+ if (irq > 0) {
ret = devm_request_irq(&pdev->dev, irq, i2s_irq_handler, 0,
pdev->name, dev);
if (ret < 0) {
@@ -697,7 +697,7 @@ static int dw_i2s_probe(struct platform_device *pdev)
}
if (!pdata) {
- if (irq >= 0) {
+ if (irq > 0) {
ret = dw_pcm_register(pdev);
dev->use_pio = true;
} else {
--
2.26.3
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
2022-02-12 20:16 ` Sergey Shtylyov
` (2 preceding siblings ...)
(?)
@ 2022-02-14 7:13 ` Uwe Kleine-König
-1 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2022-02-14 7:13 UTC (permalink / raw)
To: Sergey Shtylyov
Cc: Andrew Lunn, Ulf Hansson, Vignesh Raghavendra, kvm,
Rafael J. Wysocki, linux-iio, Linus Walleij, Amit Kucheria,
alsa-devel, Andy Shevchenko, Liam Girdwood, Guenter Roeck,
Thierry Reding, linux-mtd, linux-i2c, Miquel Raynal, linux-phy,
Oleksij Rempel, Lee Jones, David S. Miller, Peter Korsgaard,
Florian Fainelli, Matthias Schiffer, Joakim Zhang, Kamal Dasu,
Jiri Slaby, Bartosz Golaszewski, Daniel Lezcano,
Kishon Vijay Abraham I, bcm-kernel-feedback-list, linux-serial,
Jakub Kicinski, Zhang Rui, platform-driver-x86, linux-pwm,
Pengutronix Kernel Team, Corey Minyard, linux-pm, John Garry,
William Breathitt Gray, Mark Gross, linux-gpio, Alex Williamson,
Mark Brown, linux-mediatek, Eric Auger, Takashi Iwai,
Matthias Brugger, openipmi-developer, Benson Leung,
linux-arm-kernel, Mun Yew Tham, Hans de Goede, Greg Kroah-Hartman,
Yoshihiro Shimoda, Cornelia Huck, linux-mmc, linux-kernel,
linux-spi, linux-renesas-soc, Vinod Koul, Zha Qipeng,
Richard Weinberger, Niklas Söderlund, Brian Norris, netdev
[-- Attachment #1: Type: text/plain, Size: 1621 bytes --]
On Sat, Feb 12, 2022 at 11:16:30PM +0300, Sergey Shtylyov wrote:
> This patch is based on the former Andy Shevchenko's patch:
>
> https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
>
> 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@ew.tq-group.com>
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
While this patch is better than v1, I still don't like it for the
reasons discussed for v1. (i.e. 0 isn't usable as a dummy value which I
consider the real advantage for the other _get_optional() functions.)
Apart from that, I think the subject is badly chosen. With "Make
somefunc() optional" I would expect that you introduce a Kconfig symbol
that results in the function not being available when disabled.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 7:13 ` Uwe Kleine-König
0 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2022-02-14 7:13 UTC (permalink / raw)
To: Sergey Shtylyov
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel, Andrew Lunn,
Ulf Hansson, Vignesh Raghavendra, Jiri Slaby, linux-iio,
Linus Walleij, Amit Kucheria, alsa-devel, Liam Girdwood,
linux-phy, Thierry Reding, linux-mtd, linux-i2c, linux-gpio,
Miquel Raynal, Guenter Roeck, linux-spi, Lee Jones,
openipmi-developer, Peter Korsgaard, Florian Fainelli,
Matthias Schiffer, kvm, Kamal Dasu, Richard Weinberger,
Bartosz Golaszewski, Daniel Lezcano, Kishon Vijay Abraham I,
bcm-kernel-feedback-list, linux-serial, Jakub Kicinski, Zhang Rui,
Jaroslav Kysela, platform-driver-x86, linux-pwm, Zha Qipeng,
Corey Minyard, linux-pm, John Garry, William Breathitt Gray,
Mark Gross, Hans de Goede, Alex Williamson, Mark Brown,
linux-mediatek, Matthias Brugger, Takashi Iwai, Andy Shevchenko,
Benson Leung, linux-arm-kernel, Mun Yew Tham, Eric Auger, netdev,
Yoshihiro Shimoda, Cornelia Huck, linux-mmc, Joakim Zhang,
Oleksij Rempel, linux-renesas-soc, Vinod Koul,
Pengutronix Kernel Team, Niklas Söderlund, Brian Norris,
David S. Miller
[-- Attachment #1.1: Type: text/plain, Size: 1621 bytes --]
On Sat, Feb 12, 2022 at 11:16:30PM +0300, Sergey Shtylyov wrote:
> This patch is based on the former Andy Shevchenko's patch:
>
> https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
>
> 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@ew.tq-group.com>
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
While this patch is better than v1, I still don't like it for the
reasons discussed for v1. (i.e. 0 isn't usable as a dummy value which I
consider the real advantage for the other _get_optional() functions.)
Apart from that, I think the subject is badly chosen. With "Make
somefunc() optional" I would expect that you introduce a Kconfig symbol
that results in the function not being available when disabled.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 112 bytes --]
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 7:13 ` Uwe Kleine-König
0 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2022-02-14 7:13 UTC (permalink / raw)
To: Sergey Shtylyov
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel, Andrew Lunn,
Ulf Hansson, Vignesh Raghavendra, Jiri Slaby, linux-iio,
Linus Walleij, Amit Kucheria, alsa-devel, Liam Girdwood,
linux-phy, Thierry Reding, linux-mtd, linux-i2c, linux-gpio,
Miquel Raynal, Guenter Roeck, linux-spi, Lee Jones,
openipmi-developer, Peter Korsgaard, Florian Fainelli,
Matthias Schiffer, kvm, Kamal Dasu, Richard Weinberger,
Bartosz Golaszewski, Daniel Lezcano, Kishon Vijay Abraham I,
bcm-kernel-feedback-list, linux-serial, Jakub Kicinski, Zhang Rui,
Jaroslav Kysela, platform-driver-x86, linux-pwm, Zha Qipeng,
Corey Minyard, linux-pm, John Garry, William Breathitt Gray,
Mark Gross, Hans de Goede, Alex Williamson, Mark Brown,
linux-mediatek, Matthias Brugger, Takashi Iwai, Andy Shevchenko,
Benson Leung, linux-arm-kernel, Mun Yew Tham, Eric Auger, netdev,
Yoshihiro Shimoda, Cornelia Huck, linux-mmc, Joakim Zhang,
Oleksij Rempel, linux-renesas-soc, Vinod Koul,
Pengutronix Kernel Team, Niklas Söderlund, Brian Norris,
David S. Miller
[-- Attachment #1.1: Type: text/plain, Size: 1621 bytes --]
On Sat, Feb 12, 2022 at 11:16:30PM +0300, Sergey Shtylyov wrote:
> This patch is based on the former Andy Shevchenko's patch:
>
> https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
>
> 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@ew.tq-group.com>
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
While this patch is better than v1, I still don't like it for the
reasons discussed for v1. (i.e. 0 isn't usable as a dummy value which I
consider the real advantage for the other _get_optional() functions.)
Apart from that, I think the subject is badly chosen. With "Make
somefunc() optional" I would expect that you introduce a Kconfig symbol
that results in the function not being available when disabled.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 144 bytes --]
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 7:13 ` Uwe Kleine-König
0 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2022-02-14 7:13 UTC (permalink / raw)
To: Sergey Shtylyov
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel, Andrew Lunn,
Ulf Hansson, Vignesh Raghavendra, Jiri Slaby, linux-iio,
Linus Walleij, Amit Kucheria, alsa-devel, Liam Girdwood,
linux-phy, Thierry Reding, linux-mtd, linux-i2c, linux-gpio,
Miquel Raynal, Guenter Roeck, linux-spi, Lee Jones,
openipmi-developer, Peter Korsgaard, Florian Fainelli,
Matthias Schiffer, kvm, Kamal Dasu, Richard Weinberger,
Bartosz Golaszewski, Daniel Lezcano, Kishon Vijay Abraham I,
bcm-kernel-feedback-list, linux-serial, Jakub Kicinski, Zhang Rui,
Jaroslav Kysela, platform-driver-x86, linux-pwm, Zha Qipeng,
Corey Minyard, linux-pm, John Garry, William Breathitt Gray,
Mark Gross, Hans de Goede, Alex Williamson, Mark Brown,
linux-mediatek, Matthias Brugger, Takashi Iwai, Andy Shevchenko,
Benson Leung, linux-arm-kernel, Mun Yew Tham, Eric Auger, netdev,
Yoshihiro Shimoda, Cornelia Huck, linux-mmc, Joakim Zhang,
Oleksij Rempel, linux-renesas-soc, Vinod Koul,
Pengutronix Kernel Team, Niklas Söderlund, Brian Norris,
David S. Miller
[-- Attachment #1.1: Type: text/plain, Size: 1621 bytes --]
On Sat, Feb 12, 2022 at 11:16:30PM +0300, Sergey Shtylyov wrote:
> This patch is based on the former Andy Shevchenko's patch:
>
> https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
>
> 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@ew.tq-group.com>
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
While this patch is better than v1, I still don't like it for the
reasons discussed for v1. (i.e. 0 isn't usable as a dummy value which I
consider the real advantage for the other _get_optional() functions.)
Apart from that, I think the subject is badly chosen. With "Make
somefunc() optional" I would expect that you introduce a Kconfig symbol
that results in the function not being available when disabled.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 170 bytes --]
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 7:13 ` Uwe Kleine-König
0 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2022-02-14 7:13 UTC (permalink / raw)
To: Sergey Shtylyov
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel, Andrew Lunn,
Ulf Hansson, Vignesh Raghavendra, Jiri Slaby, linux-iio,
Linus Walleij, Amit Kucheria, alsa-devel, Liam Girdwood,
linux-phy, Thierry Reding, linux-mtd, linux-i2c, linux-gpio,
Miquel Raynal, Guenter Roeck, linux-spi, Lee Jones,
openipmi-developer, Peter Korsgaard, Florian Fainelli,
Matthias Schiffer, kvm, Kamal Dasu, Richard Weinberger,
Bartosz Golaszewski, Daniel Lezcano, Kishon Vijay Abraham I,
bcm-kernel-feedback-list, linux-serial, Jakub Kicinski, Zhang Rui,
Jaroslav Kysela, platform-driver-x86, linux-pwm, Zha Qipeng,
Corey Minyard, linux-pm, John Garry, William Breathitt Gray,
Mark Gross, Hans de Goede, Alex Williamson, Mark Brown,
linux-mediatek, Matthias Brugger, Takashi Iwai, Andy Shevchenko,
Benson Leung, linux-arm-kernel, Mun Yew Tham, Eric Auger, netdev,
Yoshihiro Shimoda, Cornelia Huck, linux-mmc, Joakim Zhang,
Oleksij Rempel, linux-renesas-soc, Vinod Koul,
Pengutronix Kernel Team, Niklas Söderlund, Brian Norris,
David S. Miller
[-- Attachment #1: Type: text/plain, Size: 1621 bytes --]
On Sat, Feb 12, 2022 at 11:16:30PM +0300, Sergey Shtylyov wrote:
> This patch is based on the former Andy Shevchenko's patch:
>
> https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
>
> 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@ew.tq-group.com>
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
While this patch is better than v1, I still don't like it for the
reasons discussed for v1. (i.e. 0 isn't usable as a dummy value which I
consider the real advantage for the other _get_optional() functions.)
Apart from that, I think the subject is badly chosen. With "Make
somefunc() optional" I would expect that you introduce a Kconfig symbol
that results in the function not being available when disabled.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
2022-02-14 7:13 ` Uwe Kleine-König
` (2 preceding siblings ...)
(?)
@ 2022-02-14 9:01 ` Geert Uytterhoeven
-1 siblings, 0 replies; 44+ messages in thread
From: Geert Uytterhoeven @ 2022-02-14 9:01 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Andrew Lunn, Ulf Hansson, Vignesh Raghavendra, kvm,
Rafael J. Wysocki, linux-iio, Linus Walleij, Amit Kucheria,
alsa-devel, Andy Shevchenko, Liam Girdwood, Guenter Roeck,
Thierry Reding, linux-mtd, linux-i2c, Miquel Raynal, linux-phy,
Oleksij Rempel, Lee Jones, David S. Miller, Peter Korsgaard,
Florian Fainelli, Matthias Schiffer, Joakim Zhang, Kamal Dasu,
Jiri Slaby, Bartosz Golaszewski, Daniel Lezcano,
Kishon Vijay Abraham I, bcm-kernel-feedback-list, linux-serial,
Jakub Kicinski, Zhang Rui, platform-driver-x86, linux-pwm,
Pengutronix Kernel Team, Corey Minyard, linux-pm, John Garry,
William Breathitt Gray, Mark Gross, linux-gpio, Alex Williamson,
Mark Brown, linux-mediatek, Eric Auger, Takashi Iwai,
Matthias Brugger, openipmi-developer, Benson Leung,
linux-arm-kernel, Sergey Shtylyov, Mun Yew Tham, Hans de Goede,
Greg Kroah-Hartman, Yoshihiro Shimoda, Cornelia Huck, linux-mmc,
linux-kernel, linux-spi, linux-renesas-soc, Vinod Koul,
Zha Qipeng, Richard Weinberger, Niklas Söderlund,
Brian Norris, netdev
Hi Uwe,
On Mon, Feb 14, 2022 at 8:29 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Sat, Feb 12, 2022 at 11:16:30PM +0300, Sergey Shtylyov wrote:
> > This patch is based on the former Andy Shevchenko's patch:
> >
> > https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
> >
> > 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@ew.tq-group.com>
> > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>
> While this patch is better than v1, I still don't like it for the
> reasons discussed for v1. (i.e. 0 isn't usable as a dummy value which I
> consider the real advantage for the other _get_optional() functions.)
IMHO the real advantage is the simplified error handling, which is the
area where most of the current bugs are. So I applaud the core change.
Also IMHO, the dummy value handling is a red herring. Contrary to
optional clocks and resets, a missing optional interrupt does not
always mean there is nothing to do: in case of polling, something
else must definitely be done. So even if request_irq() would accept
a dummy interrupt zero and just do nothing, it would give the false
impression that that is all there is to do, while an actual check
for zero with polling code handling may still need to be present,
thus leading to more not less bugs.
> Apart from that, I think the subject is badly chosen. With "Make
> somefunc() optional" I would expect that you introduce a Kconfig symbol
> that results in the function not being available when disabled.
Agreed.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
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
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 9:01 ` Geert Uytterhoeven
0 siblings, 0 replies; 44+ messages in thread
From: Geert Uytterhoeven @ 2022-02-14 9:01 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Sergey Shtylyov, Greg Kroah-Hartman, Rafael J. Wysocki,
linux-kernel, Andrew Lunn, Ulf Hansson, Vignesh Raghavendra,
Jiri Slaby, linux-iio, Linus Walleij, Amit Kucheria, alsa-devel,
Liam Girdwood, linux-phy, Thierry Reding, linux-mtd, linux-i2c,
linux-gpio, Miquel Raynal, Guenter Roeck, linux-spi, Lee Jones,
openipmi-developer, Peter Korsgaard, Florian Fainelli,
Matthias Schiffer, kvm, Kamal Dasu, Richard Weinberger,
Bartosz Golaszewski, Daniel Lezcano, Kishon Vijay Abraham I,
bcm-kernel-feedback-list, linux-serial, Jakub Kicinski, Zhang Rui,
Jaroslav Kysela, platform-driver-x86, linux-pwm, Zha Qipeng,
Corey Minyard, linux-pm, John Garry, William Breathitt Gray,
Mark Gross, Hans de Goede, Alex Williamson, Mark Brown,
linux-mediatek, Matthias Brugger, Takashi Iwai, Andy Shevchenko,
Benson Leung, linux-arm-kernel, Mun Yew Tham, Eric Auger, netdev,
Yoshihiro Shimoda, Cornelia Huck, linux-mmc, Joakim Zhang,
Oleksij Rempel, linux-renesas-soc, Vinod Koul,
Pengutronix Kernel Team, Niklas Söderlund, Brian Norris,
David S. Miller
Hi Uwe,
On Mon, Feb 14, 2022 at 8:29 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Sat, Feb 12, 2022 at 11:16:30PM +0300, Sergey Shtylyov wrote:
> > This patch is based on the former Andy Shevchenko's patch:
> >
> > https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
> >
> > 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@ew.tq-group.com>
> > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>
> While this patch is better than v1, I still don't like it for the
> reasons discussed for v1. (i.e. 0 isn't usable as a dummy value which I
> consider the real advantage for the other _get_optional() functions.)
IMHO the real advantage is the simplified error handling, which is the
area where most of the current bugs are. So I applaud the core change.
Also IMHO, the dummy value handling is a red herring. Contrary to
optional clocks and resets, a missing optional interrupt does not
always mean there is nothing to do: in case of polling, something
else must definitely be done. So even if request_irq() would accept
a dummy interrupt zero and just do nothing, it would give the false
impression that that is all there is to do, while an actual check
for zero with polling code handling may still need to be present,
thus leading to more not less bugs.
> Apart from that, I think the subject is badly chosen. With "Make
> somefunc() optional" I would expect that you introduce a Kconfig symbol
> that results in the function not being available when disabled.
Agreed.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
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
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 9:01 ` Geert Uytterhoeven
0 siblings, 0 replies; 44+ messages in thread
From: Geert Uytterhoeven @ 2022-02-14 9:01 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Sergey Shtylyov, Greg Kroah-Hartman, Rafael J. Wysocki,
linux-kernel, Andrew Lunn, Ulf Hansson, Vignesh Raghavendra,
Jiri Slaby, linux-iio, Linus Walleij, Amit Kucheria, alsa-devel,
Liam Girdwood, linux-phy, Thierry Reding, linux-mtd, linux-i2c,
linux-gpio, Miquel Raynal, Guenter Roeck, linux-spi, Lee Jones,
openipmi-developer, Peter Korsgaard, Florian Fainelli,
Matthias Schiffer, kvm, Kamal Dasu, Richard Weinberger,
Bartosz Golaszewski, Daniel Lezcano, Kishon Vijay Abraham I,
bcm-kernel-feedback-list, linux-serial, Jakub Kicinski, Zhang Rui,
Jaroslav Kysela, platform-driver-x86, linux-pwm, Zha Qipeng,
Corey Minyard, linux-pm, John Garry, William Breathitt Gray,
Mark Gross, Hans de Goede, Alex Williamson, Mark Brown,
linux-mediatek, Matthias Brugger, Takashi Iwai, Andy Shevchenko,
Benson Leung, linux-arm-kernel, Mun Yew Tham, Eric Auger, netdev,
Yoshihiro Shimoda, Cornelia Huck, linux-mmc, Joakim Zhang,
Oleksij Rempel, linux-renesas-soc, Vinod Koul,
Pengutronix Kernel Team, Niklas Söderlund, Brian Norris,
David S. Miller
Hi Uwe,
On Mon, Feb 14, 2022 at 8:29 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Sat, Feb 12, 2022 at 11:16:30PM +0300, Sergey Shtylyov wrote:
> > This patch is based on the former Andy Shevchenko's patch:
> >
> > https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
> >
> > 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@ew.tq-group.com>
> > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>
> While this patch is better than v1, I still don't like it for the
> reasons discussed for v1. (i.e. 0 isn't usable as a dummy value which I
> consider the real advantage for the other _get_optional() functions.)
IMHO the real advantage is the simplified error handling, which is the
area where most of the current bugs are. So I applaud the core change.
Also IMHO, the dummy value handling is a red herring. Contrary to
optional clocks and resets, a missing optional interrupt does not
always mean there is nothing to do: in case of polling, something
else must definitely be done. So even if request_irq() would accept
a dummy interrupt zero and just do nothing, it would give the false
impression that that is all there is to do, while an actual check
for zero with polling code handling may still need to be present,
thus leading to more not less bugs.
> Apart from that, I think the subject is badly chosen. With "Make
> somefunc() optional" I would expect that you introduce a Kconfig symbol
> that results in the function not being available when disabled.
Agreed.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 9:01 ` Geert Uytterhoeven
0 siblings, 0 replies; 44+ messages in thread
From: Geert Uytterhoeven @ 2022-02-14 9:01 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Sergey Shtylyov, Greg Kroah-Hartman, Rafael J. Wysocki,
linux-kernel, Andrew Lunn, Ulf Hansson, Vignesh Raghavendra,
Jiri Slaby, linux-iio, Linus Walleij, Amit Kucheria, alsa-devel,
Liam Girdwood, linux-phy, Thierry Reding, linux-mtd, linux-i2c,
linux-gpio, Miquel Raynal, Guenter Roeck, linux-spi, Lee Jones,
openipmi-developer, Peter Korsgaard, Florian Fainelli,
Matthias Schiffer, kvm, Kamal Dasu, Richard Weinberger,
Bartosz Golaszewski, Daniel Lezcano, Kishon Vijay Abraham I,
bcm-kernel-feedback-list, linux-serial, Jakub Kicinski, Zhang Rui,
Jaroslav Kysela, platform-driver-x86, linux-pwm, Zha Qipeng,
Corey Minyard, linux-pm, John Garry, William Breathitt Gray,
Mark Gross, Hans de Goede, Alex Williamson, Mark Brown,
linux-mediatek, Matthias Brugger, Takashi Iwai, Andy Shevchenko,
Benson Leung, linux-arm-kernel, Mun Yew Tham, Eric Auger, netdev,
Yoshihiro Shimoda, Cornelia Huck, linux-mmc, Joakim Zhang,
Oleksij Rempel, linux-renesas-soc, Vinod Koul,
Pengutronix Kernel Team, Niklas Söderlund, Brian Norris,
David S. Miller
Hi Uwe,
On Mon, Feb 14, 2022 at 8:29 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Sat, Feb 12, 2022 at 11:16:30PM +0300, Sergey Shtylyov wrote:
> > This patch is based on the former Andy Shevchenko's patch:
> >
> > https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
> >
> > 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@ew.tq-group.com>
> > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>
> While this patch is better than v1, I still don't like it for the
> reasons discussed for v1. (i.e. 0 isn't usable as a dummy value which I
> consider the real advantage for the other _get_optional() functions.)
IMHO the real advantage is the simplified error handling, which is the
area where most of the current bugs are. So I applaud the core change.
Also IMHO, the dummy value handling is a red herring. Contrary to
optional clocks and resets, a missing optional interrupt does not
always mean there is nothing to do: in case of polling, something
else must definitely be done. So even if request_irq() would accept
a dummy interrupt zero and just do nothing, it would give the false
impression that that is all there is to do, while an actual check
for zero with polling code handling may still need to be present,
thus leading to more not less bugs.
> Apart from that, I think the subject is badly chosen. With "Make
> somefunc() optional" I would expect that you introduce a Kconfig symbol
> that results in the function not being available when disabled.
Agreed.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
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
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 9:01 ` Geert Uytterhoeven
0 siblings, 0 replies; 44+ messages in thread
From: Geert Uytterhoeven @ 2022-02-14 9:01 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Sergey Shtylyov, Greg Kroah-Hartman, Rafael J. Wysocki,
linux-kernel, Andrew Lunn, Ulf Hansson, Vignesh Raghavendra,
Jiri Slaby, linux-iio, Linus Walleij, Amit Kucheria, alsa-devel,
Liam Girdwood, linux-phy, Thierry Reding, linux-mtd, linux-i2c,
linux-gpio, Miquel Raynal, Guenter Roeck, linux-spi, Lee Jones,
openipmi-developer, Peter Korsgaard, Florian Fainelli,
Matthias Schiffer, kvm, Kamal Dasu, Richard Weinberger,
Bartosz Golaszewski, Daniel Lezcano, Kishon Vijay Abraham I,
bcm-kernel-feedback-list, linux-serial, Jakub Kicinski, Zhang Rui,
Jaroslav Kysela, platform-driver-x86, linux-pwm, Zha Qipeng,
Corey Minyard, linux-pm, John Garry, William Breathitt Gray,
Mark Gross, Hans de Goede, Alex Williamson, Mark Brown,
linux-mediatek, Matthias Brugger, Takashi Iwai, Andy Shevchenko,
Benson Leung, linux-arm-kernel, Mun Yew Tham, Eric Auger, netdev,
Yoshihiro Shimoda, Cornelia Huck, linux-mmc, Joakim Zhang,
Oleksij Rempel, linux-renesas-soc, Vinod Koul,
Pengutronix Kernel Team, Niklas Söderlund, Brian Norris,
David S. Miller
Hi Uwe,
On Mon, Feb 14, 2022 at 8:29 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Sat, Feb 12, 2022 at 11:16:30PM +0300, Sergey Shtylyov wrote:
> > This patch is based on the former Andy Shevchenko's patch:
> >
> > https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
> >
> > 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@ew.tq-group.com>
> > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>
> While this patch is better than v1, I still don't like it for the
> reasons discussed for v1. (i.e. 0 isn't usable as a dummy value which I
> consider the real advantage for the other _get_optional() functions.)
IMHO the real advantage is the simplified error handling, which is the
area where most of the current bugs are. So I applaud the core change.
Also IMHO, the dummy value handling is a red herring. Contrary to
optional clocks and resets, a missing optional interrupt does not
always mean there is nothing to do: in case of polling, something
else must definitely be done. So even if request_irq() would accept
a dummy interrupt zero and just do nothing, it would give the false
impression that that is all there is to do, while an actual check
for zero with polling code handling may still need to be present,
thus leading to more not less bugs.
> Apart from that, I think the subject is badly chosen. With "Make
> somefunc() optional" I would expect that you introduce a Kconfig symbol
> that results in the function not being available when disabled.
Agreed.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
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
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
2022-02-14 9:01 ` Geert Uytterhoeven
` (2 preceding siblings ...)
(?)
@ 2022-02-14 14:00 ` Uwe Kleine-König
-1 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2022-02-14 14:00 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Andrew Lunn, Ulf Hansson, Vignesh Raghavendra, kvm,
Rafael J. Wysocki, linux-iio, Linus Walleij, Amit Kucheria,
alsa-devel, Joakim Zhang, Guenter Roeck, Thierry Reding,
linux-mtd, linux-i2c, Miquel Raynal, linux-phy, netdev, linux-spi,
Lee Jones, openipmi-developer, Peter Korsgaard, Florian Fainelli,
Matthias Schiffer, Kamal Dasu, Jiri Slaby, Bartosz Golaszewski,
Daniel Lezcano, Kishon Vijay Abraham I, bcm-kernel-feedback-list,
linux-serial, Jakub Kicinski, Zhang Rui, platform-driver-x86,
linux-pwm, Hans de Goede, Zha Qipeng, Corey Minyard, linux-pm,
linux-kernel, John Garry, William Breathitt Gray, Mark Gross,
linux-gpio, Alex Williamson, Mark Brown, linux-mediatek,
Matthias Brugger, Takashi Iwai, Andy Shevchenko, Benson Leung,
linux-arm-kernel, Sergey Shtylyov, Mun Yew Tham, Eric Auger,
Greg Kroah-Hartman, Yoshihiro Shimoda, Cornelia Huck, linux-mmc,
Liam Girdwood, Oleksij Rempel, linux-renesas-soc, Vinod Koul,
Pengutronix Kernel Team, Richard Weinberger,
Niklas Söderlund, Brian Norris, David S. Miller
[-- Attachment #1: Type: text/plain, Size: 1011 bytes --]
Hello Geert,
On Mon, Feb 14, 2022 at 10:01:14AM +0100, Geert Uytterhoeven wrote:
> Also IMHO, the dummy value handling is a red herring. Contrary to
> optional clocks and resets, a missing optional interrupt does not
> always mean there is nothing to do: in case of polling, something
> else must definitely be done.
Note this is exactly why I don't like this change. I'd even go so far
and claim that "a missing optional interrupt hardly ever means there is
nothing to do".
> So even if request_irq() would accept a dummy interrupt zero and just
> do nothing, it would give the false impression that that is all there
> is to do, while an actual check for zero with polling code handling
> may still need to be present, thus leading to more not less bugs.
Yes, a dummy irq value will just not be possible.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 14:00 ` Uwe Kleine-König
0 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2022-02-14 14:00 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Andrew Lunn, Ulf Hansson, Vignesh Raghavendra, kvm,
Rafael J. Wysocki, linux-iio, Linus Walleij, Amit Kucheria,
alsa-devel, Andy Shevchenko, Liam Girdwood, Guenter Roeck,
Thierry Reding, linux-mtd, linux-i2c, Miquel Raynal, linux-phy,
Oleksij Rempel, Lee Jones, David S. Miller, Peter Korsgaard,
Florian Fainelli, Matthias Schiffer, Joakim Zhang, Kamal Dasu,
Jiri Slaby, Bartosz Golaszewski, Daniel Lezcano,
Kishon Vijay Abraham I, bcm-kernel-feedback-list, linux-serial,
Jakub Kicinski, Zhang Rui, platform-driver-x86, linux-pwm,
Pengutronix Kernel Team, Corey Minyard, linux-pm, John Garry,
William Breathitt Gray, Mark Gross, linux-gpio, Alex Williamson,
Mark Brown, linux-mediatek, Eric Auger, Takashi Iwai,
Matthias Brugger, openipmi-developer, Jaroslav Kysela,
Benson Leung, linux-arm-kernel, Sergey Shtylyov, Mun Yew Tham,
Hans de Goede, Greg Kroah-Hartman, Yoshihiro Shimoda,
Cornelia Huck, linux-mmc, linux-kernel, linux-spi,
linux-renesas-soc, Vinod Koul, Zha Qipeng, Richard Weinberger,
Niklas Söderlund, Brian Norris, netdev
[-- Attachment #1.1: Type: text/plain, Size: 1011 bytes --]
Hello Geert,
On Mon, Feb 14, 2022 at 10:01:14AM +0100, Geert Uytterhoeven wrote:
> Also IMHO, the dummy value handling is a red herring. Contrary to
> optional clocks and resets, a missing optional interrupt does not
> always mean there is nothing to do: in case of polling, something
> else must definitely be done.
Note this is exactly why I don't like this change. I'd even go so far
and claim that "a missing optional interrupt hardly ever means there is
nothing to do".
> So even if request_irq() would accept a dummy interrupt zero and just
> do nothing, it would give the false impression that that is all there
> is to do, while an actual check for zero with polling code handling
> may still need to be present, thus leading to more not less bugs.
Yes, a dummy irq value will just not be possible.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 112 bytes --]
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 14:00 ` Uwe Kleine-König
0 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2022-02-14 14:00 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Andrew Lunn, Ulf Hansson, Vignesh Raghavendra, kvm,
Rafael J. Wysocki, linux-iio, Linus Walleij, Amit Kucheria,
alsa-devel, Andy Shevchenko, Liam Girdwood, Guenter Roeck,
Thierry Reding, linux-mtd, linux-i2c, Miquel Raynal, linux-phy,
Oleksij Rempel, Lee Jones, David S. Miller, Peter Korsgaard,
Florian Fainelli, Matthias Schiffer, Joakim Zhang, Kamal Dasu,
Jiri Slaby, Bartosz Golaszewski, Daniel Lezcano,
Kishon Vijay Abraham I, bcm-kernel-feedback-list, linux-serial,
Jakub Kicinski, Zhang Rui, platform-driver-x86, linux-pwm,
Pengutronix Kernel Team, Corey Minyard, linux-pm, John Garry,
William Breathitt Gray, Mark Gross, linux-gpio, Alex Williamson,
Mark Brown, linux-mediatek, Eric Auger, Takashi Iwai,
Matthias Brugger, openipmi-developer, Jaroslav Kysela,
Benson Leung, linux-arm-kernel, Sergey Shtylyov, Mun Yew Tham,
Hans de Goede, Greg Kroah-Hartman, Yoshihiro Shimoda,
Cornelia Huck, linux-mmc, linux-kernel, linux-spi,
linux-renesas-soc, Vinod Koul, Zha Qipeng, Richard Weinberger,
Niklas Söderlund, Brian Norris, netdev
[-- Attachment #1.1: Type: text/plain, Size: 1011 bytes --]
Hello Geert,
On Mon, Feb 14, 2022 at 10:01:14AM +0100, Geert Uytterhoeven wrote:
> Also IMHO, the dummy value handling is a red herring. Contrary to
> optional clocks and resets, a missing optional interrupt does not
> always mean there is nothing to do: in case of polling, something
> else must definitely be done.
Note this is exactly why I don't like this change. I'd even go so far
and claim that "a missing optional interrupt hardly ever means there is
nothing to do".
> So even if request_irq() would accept a dummy interrupt zero and just
> do nothing, it would give the false impression that that is all there
> is to do, while an actual check for zero with polling code handling
> may still need to be present, thus leading to more not less bugs.
Yes, a dummy irq value will just not be possible.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 144 bytes --]
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 14:00 ` Uwe Kleine-König
0 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2022-02-14 14:00 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Andrew Lunn, Ulf Hansson, Vignesh Raghavendra, kvm,
Rafael J. Wysocki, linux-iio, Linus Walleij, Amit Kucheria,
alsa-devel, Andy Shevchenko, Liam Girdwood, Guenter Roeck,
Thierry Reding, linux-mtd, linux-i2c, Miquel Raynal, linux-phy,
Oleksij Rempel, Lee Jones, David S. Miller, Peter Korsgaard,
Florian Fainelli, Matthias Schiffer, Joakim Zhang, Kamal Dasu,
Jiri Slaby, Bartosz Golaszewski, Daniel Lezcano,
Kishon Vijay Abraham I, bcm-kernel-feedback-list, linux-serial,
Jakub Kicinski, Zhang Rui, platform-driver-x86, linux-pwm,
Pengutronix Kernel Team, Corey Minyard, linux-pm, John Garry,
William Breathitt Gray, Mark Gross, linux-gpio, Alex Williamson,
Mark Brown, linux-mediatek, Eric Auger, Takashi Iwai,
Matthias Brugger, openipmi-developer, Jaroslav Kysela,
Benson Leung, linux-arm-kernel, Sergey Shtylyov, Mun Yew Tham,
Hans de Goede, Greg Kroah-Hartman, Yoshihiro Shimoda,
Cornelia Huck, linux-mmc, linux-kernel, linux-spi,
linux-renesas-soc, Vinod Koul, Zha Qipeng, Richard Weinberger,
Niklas Söderlund, Brian Norris, netdev
[-- Attachment #1.1: Type: text/plain, Size: 1011 bytes --]
Hello Geert,
On Mon, Feb 14, 2022 at 10:01:14AM +0100, Geert Uytterhoeven wrote:
> Also IMHO, the dummy value handling is a red herring. Contrary to
> optional clocks and resets, a missing optional interrupt does not
> always mean there is nothing to do: in case of polling, something
> else must definitely be done.
Note this is exactly why I don't like this change. I'd even go so far
and claim that "a missing optional interrupt hardly ever means there is
nothing to do".
> So even if request_irq() would accept a dummy interrupt zero and just
> do nothing, it would give the false impression that that is all there
> is to do, while an actual check for zero with polling code handling
> may still need to be present, thus leading to more not less bugs.
Yes, a dummy irq value will just not be possible.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 170 bytes --]
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 14:00 ` Uwe Kleine-König
0 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2022-02-14 14:00 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Andrew Lunn, Ulf Hansson, Vignesh Raghavendra, kvm,
Rafael J. Wysocki, linux-iio, Linus Walleij, Amit Kucheria,
alsa-devel, Andy Shevchenko, Liam Girdwood, Guenter Roeck,
Thierry Reding, linux-mtd, linux-i2c, Miquel Raynal, linux-phy,
Oleksij Rempel, Lee Jones, David S. Miller, Peter Korsgaard,
Florian Fainelli, Matthias Schiffer, Joakim Zhang, Kamal Dasu,
Jiri Slaby, Bartosz Golaszewski, Daniel Lezcano,
Kishon Vijay Abraham I, bcm-kernel-feedback-list, linux-serial,
Jakub Kicinski, Zhang Rui, platform-driver-x86, linux-pwm,
Pengutronix Kernel Team, Corey Minyard, linux-pm, John Garry,
William Breathitt Gray, Mark Gross, linux-gpio, Alex Williamson,
Mark Brown, linux-mediatek, Eric Auger, Takashi Iwai,
Matthias Brugger, openipmi-developer, Jaroslav Kysela,
Benson Leung, linux-arm-kernel, Sergey Shtylyov, Mun Yew Tham,
Hans de Goede, Greg Kroah-Hartman, Yoshihiro Shimoda,
Cornelia Huck, linux-mmc, linux-kernel, linux-spi,
linux-renesas-soc, Vinod Koul, Zha Qipeng, Richard Weinberger,
Niklas Söderlund, Brian Norris, netdev
[-- Attachment #1: Type: text/plain, Size: 1011 bytes --]
Hello Geert,
On Mon, Feb 14, 2022 at 10:01:14AM +0100, Geert Uytterhoeven wrote:
> Also IMHO, the dummy value handling is a red herring. Contrary to
> optional clocks and resets, a missing optional interrupt does not
> always mean there is nothing to do: in case of polling, something
> else must definitely be done.
Note this is exactly why I don't like this change. I'd even go so far
and claim that "a missing optional interrupt hardly ever means there is
nothing to do".
> So even if request_irq() would accept a dummy interrupt zero and just
> do nothing, it would give the false impression that that is all there
> is to do, while an actual check for zero with polling code handling
> may still need to be present, thus leading to more not less bugs.
Yes, a dummy irq value will just not be possible.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
2022-02-14 7:13 ` Uwe Kleine-König
` (2 preceding siblings ...)
(?)
@ 2022-02-14 10:13 ` Andy Shevchenko
-1 siblings, 0 replies; 44+ messages in thread
From: Andy Shevchenko @ 2022-02-14 10:13 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Andrew Lunn, Ulf Hansson, Vignesh Raghavendra, kvm,
Rafael J. Wysocki, linux-iio, Linus Walleij, Amit Kucheria,
alsa-devel, Liam Girdwood, Guenter Roeck, Thierry Reding,
linux-mtd, linux-i2c, Miquel Raynal, linux-phy, Oleksij Rempel,
Lee Jones, David S. Miller, Peter Korsgaard, Florian Fainelli,
Matthias Schiffer, Joakim Zhang, Kamal Dasu, Jiri Slaby,
Bartosz Golaszewski, Daniel Lezcano, Kishon Vijay Abraham I,
bcm-kernel-feedback-list, linux-serial, Jakub Kicinski, Zhang Rui,
platform-driver-x86, linux-pwm, Pengutronix Kernel Team,
Corey Minyard, linux-pm, John Garry, William Breathitt Gray,
Mark Gross, linux-gpio, Alex Williamson, Mark Brown,
linux-mediatek, Eric Auger, Takashi Iwai, Matthias Brugger,
openipmi-developer, Benson Leung, linux-arm-kernel,
Sergey Shtylyov, Mun Yew Tham, Hans de Goede, Greg Kroah-Hartman,
Yoshihiro Shimoda, Cornelia Huck, linux-mmc, linux-kernel,
linux-spi, linux-renesas-soc, Vinod Koul, Zha Qipeng,
Richard Weinberger, Niklas Söderlund, Brian Norris, netdev
On Mon, Feb 14, 2022 at 08:13:51AM +0100, Uwe Kleine-König wrote:
> On Sat, Feb 12, 2022 at 11:16:30PM +0300, Sergey Shtylyov wrote:
> > This patch is based on the former Andy Shevchenko's patch:
> >
> > https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
> >
> > 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@ew.tq-group.com>
> > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>
> While this patch is better than v1, I still don't like it for the
> reasons discussed for v1. (i.e. 0 isn't usable as a dummy value which I
> consider the real advantage for the other _get_optional() functions.)
I think you haven't reacted anyhow to my point that you mixing apples and
bananas together when comparing this 0 to the others _optional APIs.
> Apart from that, I think the subject is badly chosen. With "Make
> somefunc() optional" I would expect that you introduce a Kconfig symbol
> that results in the function not being available when disabled.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 10:13 ` Andy Shevchenko
0 siblings, 0 replies; 44+ messages in thread
From: Andy Shevchenko @ 2022-02-14 10:13 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Sergey Shtylyov, Greg Kroah-Hartman, Rafael J. Wysocki,
linux-kernel, Andrew Lunn, Ulf Hansson, Vignesh Raghavendra,
Jiri Slaby, linux-iio, Linus Walleij, Amit Kucheria, alsa-devel,
Liam Girdwood, linux-phy, Thierry Reding, linux-mtd, linux-i2c,
linux-gpio, Miquel Raynal, Guenter Roeck, linux-spi, Lee Jones,
openipmi-developer, Peter Korsgaard, Florian Fainelli,
Matthias Schiffer, kvm, Kamal Dasu, Richard Weinberger,
Bartosz Golaszewski, Daniel Lezcano, Kishon Vijay Abraham I,
bcm-kernel-feedback-list, linux-serial, Jakub Kicinski, Zhang Rui,
Jaroslav Kysela, platform-driver-x86, linux-pwm, Zha Qipeng,
Corey Minyard, linux-pm, John Garry, William Breathitt Gray,
Mark Gross, Hans de Goede, Alex Williamson, Mark Brown,
linux-mediatek, Matthias Brugger, Takashi Iwai, Benson Leung,
linux-arm-kernel, Mun Yew Tham, Eric Auger, netdev,
Yoshihiro Shimoda, Cornelia Huck, linux-mmc, Joakim Zhang,
Oleksij Rempel, linux-renesas-soc, Vinod Koul,
Pengutronix Kernel Team, Niklas Söderlund, Brian Norris,
David S. Miller
On Mon, Feb 14, 2022 at 08:13:51AM +0100, Uwe Kleine-König wrote:
> On Sat, Feb 12, 2022 at 11:16:30PM +0300, Sergey Shtylyov wrote:
> > This patch is based on the former Andy Shevchenko's patch:
> >
> > https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
> >
> > 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@ew.tq-group.com>
> > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>
> While this patch is better than v1, I still don't like it for the
> reasons discussed for v1. (i.e. 0 isn't usable as a dummy value which I
> consider the real advantage for the other _get_optional() functions.)
I think you haven't reacted anyhow to my point that you mixing apples and
bananas together when comparing this 0 to the others _optional APIs.
> Apart from that, I think the subject is badly chosen. With "Make
> somefunc() optional" I would expect that you introduce a Kconfig symbol
> that results in the function not being available when disabled.
--
With Best Regards,
Andy Shevchenko
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 10:13 ` Andy Shevchenko
0 siblings, 0 replies; 44+ messages in thread
From: Andy Shevchenko @ 2022-02-14 10:13 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Sergey Shtylyov, Greg Kroah-Hartman, Rafael J. Wysocki,
linux-kernel, Andrew Lunn, Ulf Hansson, Vignesh Raghavendra,
Jiri Slaby, linux-iio, Linus Walleij, Amit Kucheria, alsa-devel,
Liam Girdwood, linux-phy, Thierry Reding, linux-mtd, linux-i2c,
linux-gpio, Miquel Raynal, Guenter Roeck, linux-spi, Lee Jones,
openipmi-developer, Peter Korsgaard, Florian Fainelli,
Matthias Schiffer, kvm, Kamal Dasu, Richard Weinberger,
Bartosz Golaszewski, Daniel Lezcano, Kishon Vijay Abraham I,
bcm-kernel-feedback-list, linux-serial, Jakub Kicinski, Zhang Rui,
Jaroslav Kysela, platform-driver-x86, linux-pwm, Zha Qipeng,
Corey Minyard, linux-pm, John Garry, William Breathitt Gray,
Mark Gross, Hans de Goede, Alex Williamson, Mark Brown,
linux-mediatek, Matthias Brugger, Takashi Iwai, Benson Leung,
linux-arm-kernel, Mun Yew Tham, Eric Auger, netdev,
Yoshihiro Shimoda, Cornelia Huck, linux-mmc, Joakim Zhang,
Oleksij Rempel, linux-renesas-soc, Vinod Koul,
Pengutronix Kernel Team, Niklas Söderlund, Brian Norris,
David S. Miller
On Mon, Feb 14, 2022 at 08:13:51AM +0100, Uwe Kleine-König wrote:
> On Sat, Feb 12, 2022 at 11:16:30PM +0300, Sergey Shtylyov wrote:
> > This patch is based on the former Andy Shevchenko's patch:
> >
> > https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
> >
> > 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@ew.tq-group.com>
> > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>
> While this patch is better than v1, I still don't like it for the
> reasons discussed for v1. (i.e. 0 isn't usable as a dummy value which I
> consider the real advantage for the other _get_optional() functions.)
I think you haven't reacted anyhow to my point that you mixing apples and
bananas together when comparing this 0 to the others _optional APIs.
> Apart from that, I think the subject is badly chosen. With "Make
> somefunc() optional" I would expect that you introduce a Kconfig symbol
> that results in the function not being available when disabled.
--
With Best Regards,
Andy Shevchenko
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 10:13 ` Andy Shevchenko
0 siblings, 0 replies; 44+ messages in thread
From: Andy Shevchenko @ 2022-02-14 10:13 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Sergey Shtylyov, Greg Kroah-Hartman, Rafael J. Wysocki,
linux-kernel, Andrew Lunn, Ulf Hansson, Vignesh Raghavendra,
Jiri Slaby, linux-iio, Linus Walleij, Amit Kucheria, alsa-devel,
Liam Girdwood, linux-phy, Thierry Reding, linux-mtd, linux-i2c,
linux-gpio, Miquel Raynal, Guenter Roeck, linux-spi, Lee Jones,
openipmi-developer, Peter Korsgaard, Florian Fainelli,
Matthias Schiffer, kvm, Kamal Dasu, Richard Weinberger,
Bartosz Golaszewski, Daniel Lezcano, Kishon Vijay Abraham I,
bcm-kernel-feedback-list, linux-serial, Jakub Kicinski, Zhang Rui,
Jaroslav Kysela, platform-driver-x86, linux-pwm, Zha Qipeng,
Corey Minyard, linux-pm, John Garry, William Breathitt Gray,
Mark Gross, Hans de Goede, Alex Williamson, Mark Brown,
linux-mediatek, Matthias Brugger, Takashi Iwai, Benson Leung,
linux-arm-kernel, Mun Yew Tham, Eric Auger, netdev,
Yoshihiro Shimoda, Cornelia Huck, linux-mmc, Joakim Zhang,
Oleksij Rempel, linux-renesas-soc, Vinod Koul,
Pengutronix Kernel Team, Niklas Söderlund, Brian Norris,
David S. Miller
On Mon, Feb 14, 2022 at 08:13:51AM +0100, Uwe Kleine-König wrote:
> On Sat, Feb 12, 2022 at 11:16:30PM +0300, Sergey Shtylyov wrote:
> > This patch is based on the former Andy Shevchenko's patch:
> >
> > https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
> >
> > 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@ew.tq-group.com>
> > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>
> While this patch is better than v1, I still don't like it for the
> reasons discussed for v1. (i.e. 0 isn't usable as a dummy value which I
> consider the real advantage for the other _get_optional() functions.)
I think you haven't reacted anyhow to my point that you mixing apples and
bananas together when comparing this 0 to the others _optional APIs.
> Apart from that, I think the subject is badly chosen. With "Make
> somefunc() optional" I would expect that you introduce a Kconfig symbol
> that results in the function not being available when disabled.
--
With Best Regards,
Andy Shevchenko
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 10:13 ` Andy Shevchenko
0 siblings, 0 replies; 44+ messages in thread
From: Andy Shevchenko @ 2022-02-14 10:13 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Sergey Shtylyov, Greg Kroah-Hartman, Rafael J. Wysocki,
linux-kernel, Andrew Lunn, Ulf Hansson, Vignesh Raghavendra,
Jiri Slaby, linux-iio, Linus Walleij, Amit Kucheria, alsa-devel,
Liam Girdwood, linux-phy, Thierry Reding, linux-mtd, linux-i2c,
linux-gpio, Miquel Raynal, Guenter Roeck, linux-spi, Lee Jones,
openipmi-developer, Peter Korsgaard, Florian Fainelli,
Matthias Schiffer, kvm, Kamal Dasu, Richard Weinberger,
Bartosz Golaszewski, Daniel Lezcano, Kishon Vijay Abraham I,
bcm-kernel-feedback-list, linux-serial, Jakub Kicinski, Zhang Rui,
Jaroslav Kysela, platform-driver-x86, linux-pwm, Zha Qipeng,
Corey Minyard, linux-pm, John Garry, William Breathitt Gray,
Mark Gross, Hans de Goede, Alex Williamson, Mark Brown,
linux-mediatek, Matthias Brugger, Takashi Iwai, Benson Leung,
linux-arm-kernel, Mun Yew Tham, Eric Auger, netdev,
Yoshihiro Shimoda, Cornelia Huck, linux-mmc, Joakim Zhang,
Oleksij Rempel, linux-renesas-soc, Vinod Koul,
Pengutronix Kernel Team, Niklas Söderlund, Brian Norris,
David S. Miller
On Mon, Feb 14, 2022 at 08:13:51AM +0100, Uwe Kleine-König wrote:
> On Sat, Feb 12, 2022 at 11:16:30PM +0300, Sergey Shtylyov wrote:
> > This patch is based on the former Andy Shevchenko's patch:
> >
> > https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
> >
> > 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@ew.tq-group.com>
> > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>
> While this patch is better than v1, I still don't like it for the
> reasons discussed for v1. (i.e. 0 isn't usable as a dummy value which I
> consider the real advantage for the other _get_optional() functions.)
I think you haven't reacted anyhow to my point that you mixing apples and
bananas together when comparing this 0 to the others _optional APIs.
> Apart from that, I think the subject is badly chosen. With "Make
> somefunc() optional" I would expect that you introduce a Kconfig symbol
> that results in the function not being available when disabled.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
2022-02-14 10:13 ` Andy Shevchenko
` (2 preceding siblings ...)
(?)
@ 2022-02-14 13:56 ` Uwe Kleine-König
-1 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2022-02-14 13:56 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andrew Lunn, Ulf Hansson, Vignesh Raghavendra, kvm,
Rafael J. Wysocki, linux-iio, Linus Walleij, Amit Kucheria,
alsa-devel, Joakim Zhang, Guenter Roeck, Thierry Reding,
linux-mtd, linux-i2c, Miquel Raynal, linux-phy, netdev, linux-spi,
Lee Jones, openipmi-developer, Peter Korsgaard, Florian Fainelli,
Matthias Schiffer, Kamal Dasu, Jiri Slaby, Bartosz Golaszewski,
Daniel Lezcano, Kishon Vijay Abraham I, bcm-kernel-feedback-list,
linux-serial, Jakub Kicinski, Zhang Rui, linux-pwm, Hans de Goede,
Zha Qipeng, Corey Minyard, linux-pm, linux-kernel, John Garry,
William Breathitt Gray, Mark Gross, linux-gpio, Alex Williamson,
Mark Brown, linux-mediatek, Matthias Brugger, Takashi Iwai,
platform-driver-x86, Benson Leung, linux-arm-kernel,
Sergey Shtylyov, Mun Yew Tham, Eric Auger, Greg Kroah-Hartman,
Yoshihiro Shimoda, Cornelia Huck, linux-mmc, Liam Girdwood,
Oleksij Rempel, linux-renesas-soc, Vinod Koul,
Pengutronix Kernel Team, Richard Weinberger,
Niklas Söderlund, Brian Norris, David S. Miller
[-- Attachment #1: Type: text/plain, Size: 2151 bytes --]
Hello Andy,
On Mon, Feb 14, 2022 at 12:13:01PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 14, 2022 at 08:13:51AM +0100, Uwe Kleine-König wrote:
> > On Sat, Feb 12, 2022 at 11:16:30PM +0300, Sergey Shtylyov wrote:
> > > This patch is based on the former Andy Shevchenko's patch:
> > >
> > > https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
> > >
> > > 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@ew.tq-group.com>
> > > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> >
> > While this patch is better than v1, I still don't like it for the
> > reasons discussed for v1. (i.e. 0 isn't usable as a dummy value which I
> > consider the real advantage for the other _get_optional() functions.)
>
> I think you haven't reacted anyhow to my point that you mixing apples and
> bananas together when comparing this 0 to the others _optional APIs.
Is this a question to me or Sergey?
I fully agree, when the 0 of platform_get_irq_optional is an apple and
the NULL of gpio_get_optional is a banana, I doubt "All other
*_optional() APIs seem to return 0 or NULL in case an optional resource
is not available. Let's follow this good example, [...]".
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 13:56 ` Uwe Kleine-König
0 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2022-02-14 13:56 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andrew Lunn, Ulf Hansson, Vignesh Raghavendra, kvm,
Rafael J. Wysocki, linux-iio, Linus Walleij, Amit Kucheria,
alsa-devel, Liam Girdwood, Guenter Roeck, Thierry Reding,
linux-mtd, linux-i2c, Miquel Raynal, linux-phy, Oleksij Rempel,
Lee Jones, David S. Miller, Peter Korsgaard, Florian Fainelli,
Matthias Schiffer, Joakim Zhang, Kamal Dasu, Jiri Slaby,
Bartosz Golaszewski, Daniel Lezcano, Kishon Vijay Abraham I,
bcm-kernel-feedback-list, linux-serial, Jakub Kicinski, Zhang Rui,
platform-driver-x86, linux-pwm, Pengutronix Kernel Team,
Corey Minyard, linux-pm, John Garry, William Breathitt Gray,
Mark Gross, linux-gpio, Alex Williamson, Mark Brown,
linux-mediatek, Eric Auger, Takashi Iwai, Matthias Brugger,
openipmi-developer, Jaroslav Kysela, Benson Leung,
linux-arm-kernel, Sergey Shtylyov, Mun Yew Tham, Hans de Goede,
Greg Kroah-Hartman, Yoshihiro Shimoda, Cornelia Huck, linux-mmc,
linux-kernel, linux-spi, linux-renesas-soc, Vinod Koul,
Zha Qipeng, Richard Weinberger, Niklas Söderlund,
Brian Norris, netdev
[-- Attachment #1.1: Type: text/plain, Size: 2151 bytes --]
Hello Andy,
On Mon, Feb 14, 2022 at 12:13:01PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 14, 2022 at 08:13:51AM +0100, Uwe Kleine-König wrote:
> > On Sat, Feb 12, 2022 at 11:16:30PM +0300, Sergey Shtylyov wrote:
> > > This patch is based on the former Andy Shevchenko's patch:
> > >
> > > https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
> > >
> > > 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@ew.tq-group.com>
> > > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> >
> > While this patch is better than v1, I still don't like it for the
> > reasons discussed for v1. (i.e. 0 isn't usable as a dummy value which I
> > consider the real advantage for the other _get_optional() functions.)
>
> I think you haven't reacted anyhow to my point that you mixing apples and
> bananas together when comparing this 0 to the others _optional APIs.
Is this a question to me or Sergey?
I fully agree, when the 0 of platform_get_irq_optional is an apple and
the NULL of gpio_get_optional is a banana, I doubt "All other
*_optional() APIs seem to return 0 or NULL in case an optional resource
is not available. Let's follow this good example, [...]".
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 112 bytes --]
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 13:56 ` Uwe Kleine-König
0 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2022-02-14 13:56 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andrew Lunn, Ulf Hansson, Vignesh Raghavendra, kvm,
Rafael J. Wysocki, linux-iio, Linus Walleij, Amit Kucheria,
alsa-devel, Liam Girdwood, Guenter Roeck, Thierry Reding,
linux-mtd, linux-i2c, Miquel Raynal, linux-phy, Oleksij Rempel,
Lee Jones, David S. Miller, Peter Korsgaard, Florian Fainelli,
Matthias Schiffer, Joakim Zhang, Kamal Dasu, Jiri Slaby,
Bartosz Golaszewski, Daniel Lezcano, Kishon Vijay Abraham I,
bcm-kernel-feedback-list, linux-serial, Jakub Kicinski, Zhang Rui,
platform-driver-x86, linux-pwm, Pengutronix Kernel Team,
Corey Minyard, linux-pm, John Garry, William Breathitt Gray,
Mark Gross, linux-gpio, Alex Williamson, Mark Brown,
linux-mediatek, Eric Auger, Takashi Iwai, Matthias Brugger,
openipmi-developer, Jaroslav Kysela, Benson Leung,
linux-arm-kernel, Sergey Shtylyov, Mun Yew Tham, Hans de Goede,
Greg Kroah-Hartman, Yoshihiro Shimoda, Cornelia Huck, linux-mmc,
linux-kernel, linux-spi, linux-renesas-soc, Vinod Koul,
Zha Qipeng, Richard Weinberger, Niklas Söderlund,
Brian Norris, netdev
[-- Attachment #1.1: Type: text/plain, Size: 2151 bytes --]
Hello Andy,
On Mon, Feb 14, 2022 at 12:13:01PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 14, 2022 at 08:13:51AM +0100, Uwe Kleine-König wrote:
> > On Sat, Feb 12, 2022 at 11:16:30PM +0300, Sergey Shtylyov wrote:
> > > This patch is based on the former Andy Shevchenko's patch:
> > >
> > > https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
> > >
> > > 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@ew.tq-group.com>
> > > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> >
> > While this patch is better than v1, I still don't like it for the
> > reasons discussed for v1. (i.e. 0 isn't usable as a dummy value which I
> > consider the real advantage for the other _get_optional() functions.)
>
> I think you haven't reacted anyhow to my point that you mixing apples and
> bananas together when comparing this 0 to the others _optional APIs.
Is this a question to me or Sergey?
I fully agree, when the 0 of platform_get_irq_optional is an apple and
the NULL of gpio_get_optional is a banana, I doubt "All other
*_optional() APIs seem to return 0 or NULL in case an optional resource
is not available. Let's follow this good example, [...]".
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 144 bytes --]
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 13:56 ` Uwe Kleine-König
0 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2022-02-14 13:56 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andrew Lunn, Ulf Hansson, Vignesh Raghavendra, kvm,
Rafael J. Wysocki, linux-iio, Linus Walleij, Amit Kucheria,
alsa-devel, Liam Girdwood, Guenter Roeck, Thierry Reding,
linux-mtd, linux-i2c, Miquel Raynal, linux-phy, Oleksij Rempel,
Lee Jones, David S. Miller, Peter Korsgaard, Florian Fainelli,
Matthias Schiffer, Joakim Zhang, Kamal Dasu, Jiri Slaby,
Bartosz Golaszewski, Daniel Lezcano, Kishon Vijay Abraham I,
bcm-kernel-feedback-list, linux-serial, Jakub Kicinski, Zhang Rui,
platform-driver-x86, linux-pwm, Pengutronix Kernel Team,
Corey Minyard, linux-pm, John Garry, William Breathitt Gray,
Mark Gross, linux-gpio, Alex Williamson, Mark Brown,
linux-mediatek, Eric Auger, Takashi Iwai, Matthias Brugger,
openipmi-developer, Jaroslav Kysela, Benson Leung,
linux-arm-kernel, Sergey Shtylyov, Mun Yew Tham, Hans de Goede,
Greg Kroah-Hartman, Yoshihiro Shimoda, Cornelia Huck, linux-mmc,
linux-kernel, linux-spi, linux-renesas-soc, Vinod Koul,
Zha Qipeng, Richard Weinberger, Niklas Söderlund,
Brian Norris, netdev
[-- Attachment #1.1: Type: text/plain, Size: 2151 bytes --]
Hello Andy,
On Mon, Feb 14, 2022 at 12:13:01PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 14, 2022 at 08:13:51AM +0100, Uwe Kleine-König wrote:
> > On Sat, Feb 12, 2022 at 11:16:30PM +0300, Sergey Shtylyov wrote:
> > > This patch is based on the former Andy Shevchenko's patch:
> > >
> > > https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
> > >
> > > 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@ew.tq-group.com>
> > > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> >
> > While this patch is better than v1, I still don't like it for the
> > reasons discussed for v1. (i.e. 0 isn't usable as a dummy value which I
> > consider the real advantage for the other _get_optional() functions.)
>
> I think you haven't reacted anyhow to my point that you mixing apples and
> bananas together when comparing this 0 to the others _optional APIs.
Is this a question to me or Sergey?
I fully agree, when the 0 of platform_get_irq_optional is an apple and
the NULL of gpio_get_optional is a banana, I doubt "All other
*_optional() APIs seem to return 0 or NULL in case an optional resource
is not available. Let's follow this good example, [...]".
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 170 bytes --]
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 13:56 ` Uwe Kleine-König
0 siblings, 0 replies; 44+ messages in thread
From: Uwe Kleine-König @ 2022-02-14 13:56 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andrew Lunn, Ulf Hansson, Vignesh Raghavendra, kvm,
Rafael J. Wysocki, linux-iio, Linus Walleij, Amit Kucheria,
alsa-devel, Liam Girdwood, Guenter Roeck, Thierry Reding,
linux-mtd, linux-i2c, Miquel Raynal, linux-phy, Oleksij Rempel,
Lee Jones, David S. Miller, Peter Korsgaard, Florian Fainelli,
Matthias Schiffer, Joakim Zhang, Kamal Dasu, Jiri Slaby,
Bartosz Golaszewski, Daniel Lezcano, Kishon Vijay Abraham I,
bcm-kernel-feedback-list, linux-serial, Jakub Kicinski, Zhang Rui,
platform-driver-x86, linux-pwm, Pengutronix Kernel Team,
Corey Minyard, linux-pm, John Garry, William Breathitt Gray,
Mark Gross, linux-gpio, Alex Williamson, Mark Brown,
linux-mediatek, Eric Auger, Takashi Iwai, Matthias Brugger,
openipmi-developer, Jaroslav Kysela, Benson Leung,
linux-arm-kernel, Sergey Shtylyov, Mun Yew Tham, Hans de Goede,
Greg Kroah-Hartman, Yoshihiro Shimoda, Cornelia Huck, linux-mmc,
linux-kernel, linux-spi, linux-renesas-soc, Vinod Koul,
Zha Qipeng, Richard Weinberger, Niklas Söderlund,
Brian Norris, netdev
[-- Attachment #1: Type: text/plain, Size: 2151 bytes --]
Hello Andy,
On Mon, Feb 14, 2022 at 12:13:01PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 14, 2022 at 08:13:51AM +0100, Uwe Kleine-König wrote:
> > On Sat, Feb 12, 2022 at 11:16:30PM +0300, Sergey Shtylyov wrote:
> > > This patch is based on the former Andy Shevchenko's patch:
> > >
> > > https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
> > >
> > > 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@ew.tq-group.com>
> > > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> >
> > While this patch is better than v1, I still don't like it for the
> > reasons discussed for v1. (i.e. 0 isn't usable as a dummy value which I
> > consider the real advantage for the other _get_optional() functions.)
>
> I think you haven't reacted anyhow to my point that you mixing apples and
> bananas together when comparing this 0 to the others _optional APIs.
Is this a question to me or Sergey?
I fully agree, when the 0 of platform_get_irq_optional is an apple and
the NULL of gpio_get_optional is a banana, I doubt "All other
*_optional() APIs seem to return 0 or NULL in case an optional resource
is not available. Let's follow this good example, [...]".
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
2022-02-12 20:16 ` Sergey Shtylyov
` (2 preceding siblings ...)
(?)
@ 2022-02-14 8:54 ` Geert Uytterhoeven
-1 siblings, 0 replies; 44+ messages in thread
From: Geert Uytterhoeven @ 2022-02-14 8:54 UTC (permalink / raw)
To: Sergey Shtylyov
Cc: Andrew Lunn, Ulf Hansson, Vignesh Raghavendra, Jiri Slaby,
Rafael J. Wysocki, linux-iio, Linus Walleij, Amit Kucheria,
alsa-devel, Joakim Zhang, linux-phy, Thierry Reding, linux-mtd,
linux-i2c, linux-gpio, Miquel Raynal, Guenter Roeck, linux-spi,
Lee Jones, openipmi-developer, Peter Korsgaard, Florian Fainelli,
Matthias Schiffer, kvm, Kamal Dasu, Richard Weinberger,
Bartosz Golaszewski, Daniel Lezcano, Kishon Vijay Abraham I,
bcm-kernel-feedback-list, linux-serial, Uwe Kleine-König,
Jakub Kicinski, Zhang Rui, platform-driver-x86, linux-pwm,
Zha Qipeng, Corey Minyard, linux-pm, Liam Girdwood, John Garry,
William Breathitt Gray, Mark Gross, Hans de Goede,
Alex Williamson, Mark Brown, linux-mediatek, Matthias Brugger,
Takashi Iwai, Andy Shevchenko, Benson Leung, linux-arm-kernel,
Mun Yew Tham, Eric Auger, Greg Kroah-Hartman, Yoshihiro Shimoda,
Cornelia Huck, linux-mmc, linux-kernel, Oleksij Rempel,
linux-renesas-soc, Vinod Koul, Pengutronix Kernel Team, netdev,
Niklas Söderlund, Brian Norris, David S. Miller
Hi Sergey,
On Sat, Feb 12, 2022 at 9:17 PM Sergey Shtylyov <s.shtylyov@omp.ru> wrote:
> This patch is based on the former Andy Shevchenko's patch:
>
> https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
>
> 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@ew.tq-group.com>
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> ---
> 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@linux-m68k.org
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
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 8:54 ` Geert Uytterhoeven
0 siblings, 0 replies; 44+ messages in thread
From: Geert Uytterhoeven @ 2022-02-14 8:54 UTC (permalink / raw)
To: Sergey Shtylyov
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel,
Andy Shevchenko, Corey Minyard, Oleksij Rempel,
Pengutronix Kernel Team, William Breathitt Gray, Mun Yew Tham,
Linus Walleij, Bartosz Golaszewski, Thierry Reding,
Uwe Kleine-König, Lee Jones, Kamal Dasu, Florian Fainelli,
bcm-kernel-feedback-list, Peter Korsgaard, Andrew Lunn,
Ulf Hansson, Brian Norris, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, David S. Miller, Jakub Kicinski,
Joakim Zhang, Yoshihiro Shimoda, Kishon Vijay Abraham I,
Vinod Koul, Benson Leung, Guenter Roeck, Zha Qipeng,
Hans de Goede, Mark Gross, John Garry, Mark Brown,
Matthias Brugger, Niklas Söderlund, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Jiri Slaby, Eric Auger, Alex Williamson,
Cornelia Huck, Liam Girdwood, Jaroslav Kysela, Takashi Iwai,
openipmi-developer, linux-iio, linux-gpio, linux-pwm, linux-i2c,
linux-arm-kernel, linux-mmc, linux-mtd, netdev, linux-renesas-soc,
linux-phy, platform-driver-x86, linux-spi, linux-mediatek,
linux-pm, linux-serial, kvm, alsa-devel, Matthias Schiffer
Hi Sergey,
On Sat, Feb 12, 2022 at 9:17 PM Sergey Shtylyov <s.shtylyov@omp.ru> wrote:
> This patch is based on the former Andy Shevchenko's patch:
>
> https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
>
> 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@ew.tq-group.com>
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> ---
> 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@linux-m68k.org
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
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 8:54 ` Geert Uytterhoeven
0 siblings, 0 replies; 44+ messages in thread
From: Geert Uytterhoeven @ 2022-02-14 8:54 UTC (permalink / raw)
To: Sergey Shtylyov
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel,
Andy Shevchenko, Corey Minyard, Oleksij Rempel,
Pengutronix Kernel Team, William Breathitt Gray, Mun Yew Tham,
Linus Walleij, Bartosz Golaszewski, Thierry Reding,
Uwe Kleine-König, Lee Jones, Kamal Dasu, Florian Fainelli,
bcm-kernel-feedback-list, Peter Korsgaard, Andrew Lunn,
Ulf Hansson, Brian Norris, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, David S. Miller, Jakub Kicinski,
Joakim Zhang, Yoshihiro Shimoda, Kishon Vijay Abraham I,
Vinod Koul, Benson Leung, Guenter Roeck, Zha Qipeng,
Hans de Goede, Mark Gross, John Garry, Mark Brown,
Matthias Brugger, Niklas Söderlund, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Jiri Slaby, Eric Auger, Alex Williamson,
Cornelia Huck, Liam Girdwood, Jaroslav Kysela, Takashi Iwai,
openipmi-developer, linux-iio, linux-gpio, linux-pwm, linux-i2c,
linux-arm-kernel, linux-mmc, linux-mtd, netdev, linux-renesas-soc,
linux-phy, platform-driver-x86, linux-spi, linux-mediatek,
linux-pm, linux-serial, kvm, alsa-devel, Matthias Schiffer
Hi Sergey,
On Sat, Feb 12, 2022 at 9:17 PM Sergey Shtylyov <s.shtylyov@omp.ru> wrote:
> This patch is based on the former Andy Shevchenko's patch:
>
> https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
>
> 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@ew.tq-group.com>
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> ---
> 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@linux-m68k.org
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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 8:54 ` Geert Uytterhoeven
0 siblings, 0 replies; 44+ messages in thread
From: Geert Uytterhoeven @ 2022-02-14 8:54 UTC (permalink / raw)
To: Sergey Shtylyov
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel,
Andy Shevchenko, Corey Minyard, Oleksij Rempel,
Pengutronix Kernel Team, William Breathitt Gray, Mun Yew Tham,
Linus Walleij, Bartosz Golaszewski, Thierry Reding,
Uwe Kleine-König, Lee Jones, Kamal Dasu, Florian Fainelli,
bcm-kernel-feedback-list, Peter Korsgaard, Andrew Lunn,
Ulf Hansson, Brian Norris, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, David S. Miller, Jakub Kicinski,
Joakim Zhang, Yoshihiro Shimoda, Kishon Vijay Abraham I,
Vinod Koul, Benson Leung, Guenter Roeck, Zha Qipeng,
Hans de Goede, Mark Gross, John Garry, Mark Brown,
Matthias Brugger, Niklas Söderlund, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Jiri Slaby, Eric Auger, Alex Williamson,
Cornelia Huck, Liam Girdwood, Jaroslav Kysela, Takashi Iwai,
openipmi-developer, linux-iio, linux-gpio, linux-pwm, linux-i2c,
linux-arm-kernel, linux-mmc, linux-mtd, netdev, linux-renesas-soc,
linux-phy, platform-driver-x86, linux-spi, linux-mediatek,
linux-pm, linux-serial, kvm, alsa-devel, Matthias Schiffer
Hi Sergey,
On Sat, Feb 12, 2022 at 9:17 PM Sergey Shtylyov <s.shtylyov@omp.ru> wrote:
> This patch is based on the former Andy Shevchenko's patch:
>
> https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
>
> 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@ew.tq-group.com>
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> ---
> 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@linux-m68k.org
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
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 8:54 ` Geert Uytterhoeven
0 siblings, 0 replies; 44+ messages in thread
From: Geert Uytterhoeven @ 2022-02-14 8:54 UTC (permalink / raw)
To: Sergey Shtylyov
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel,
Andy Shevchenko, Corey Minyard, Oleksij Rempel,
Pengutronix Kernel Team, William Breathitt Gray, Mun Yew Tham,
Linus Walleij, Bartosz Golaszewski, Thierry Reding,
Uwe Kleine-König, Lee Jones, Kamal Dasu, Florian Fainelli,
bcm-kernel-feedback-list, Peter Korsgaard, Andrew Lunn,
Ulf Hansson, Brian Norris, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, David S. Miller, Jakub Kicinski,
Joakim Zhang, Yoshihiro Shimoda, Kishon Vijay Abraham I,
Vinod Koul, Benson Leung, Guenter Roeck, Zha Qipeng,
Hans de Goede, Mark Gross, John Garry, Mark Brown,
Matthias Brugger, Niklas Söderlund, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Jiri Slaby, Eric Auger, Alex Williamson,
Cornelia Huck, Liam Girdwood, Jaroslav Kysela, Takashi Iwai,
openipmi-developer, linux-iio, linux-gpio, linux-pwm, linux-i2c,
linux-arm-kernel, linux-mmc, linux-mtd, netdev, linux-renesas-soc,
linux-phy, platform-driver-x86, linux-spi, linux-mediatek,
linux-pm, linux-serial, kvm, alsa-devel, Matthias Schiffer
Hi Sergey,
On Sat, Feb 12, 2022 at 9:17 PM Sergey Shtylyov <s.shtylyov@omp.ru> wrote:
> This patch is based on the former Andy Shevchenko's patch:
>
> https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
>
> 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@ew.tq-group.com>
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> ---
> 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@linux-m68k.org
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
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
2022-02-14 8:54 ` Geert Uytterhoeven
` (2 preceding siblings ...)
(?)
@ 2022-02-14 10:43 ` Sergey Shtylyov
-1 siblings, 0 replies; 44+ messages in thread
From: Sergey Shtylyov @ 2022-02-14 10:43 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Andrew Lunn, Ulf Hansson, Vignesh Raghavendra, Jiri Slaby,
Rafael J. Wysocki, linux-iio, Linus Walleij, Amit Kucheria,
alsa-devel, Joakim Zhang, linux-phy, Thierry Reding, linux-mtd,
linux-i2c, linux-gpio, Miquel Raynal, Guenter Roeck, linux-spi,
Lee Jones, openipmi-developer, Peter Korsgaard, Florian Fainelli,
Matthias Schiffer, kvm, Kamal Dasu, Richard Weinberger,
Bartosz Golaszewski, Daniel Lezcano, Kishon Vijay Abraham I,
bcm-kernel-feedback-list, linux-serial, Uwe Kleine-König,
Jakub Kicinski, Zhang Rui, platform-driver-x86, linux-pwm,
Zha Qipeng, Corey Minyard, linux-pm, Liam Girdwood, John Garry,
William Breathitt Gray, Mark Gross, Hans de Goede,
Alex Williamson, Mark Brown, linux-mediatek, Matthias Brugger,
Takashi Iwai, Andy Shevchenko, Benson Leung, linux-arm-kernel,
Mun Yew Tham, Eric Auger, Greg Kroah-Hartman, Yoshihiro Shimoda,
Cornelia Huck, linux-mmc, linux-kernel, Oleksij Rempel,
linux-renesas-soc, Vinod Koul, Pengutronix Kernel Team, netdev,
Niklas Söderlund, Brian Norris, David S. Miller
Hello!
On 2/14/22 11:54 AM, Geert Uytterhoeven wrote:
[...]
>> This patch is based on the former Andy Shevchenko's patch:
>>
>> https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
>>
>> 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@ew.tq-group.com>
>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>> ---
>> Changes in version 2:
>
> Thanks for the update!
>
>> drivers/base/platform.c | 60 +++++++++++++++---------
>
> The core change LGTM.
Thanx! :-)
> 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?
Yes.
[...]
>> 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?
Yes.
>> --- 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.
Hm, you're right... should be <= 0 there, it seems.
> As this adds missing error handling, this is to be fixed by a separate
> patch later?
Propagating errors from the probe() method is a matter of separate patch, yes.
>>
>> 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".
Yes, probably... TY!
> Gr{oetje,eeting}s,
>
> Geert
MBR, Sergey
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 10:43 ` Sergey Shtylyov
0 siblings, 0 replies; 44+ messages in thread
From: Sergey Shtylyov @ 2022-02-14 10:43 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel,
Andy Shevchenko, Corey Minyard, Oleksij Rempel,
Pengutronix Kernel Team, William Breathitt Gray, Mun Yew Tham,
Linus Walleij, Bartosz Golaszewski, Thierry Reding,
Uwe Kleine-König, Lee Jones, Kamal Dasu, Florian Fainelli,
bcm-kernel-feedback-list, Peter Korsgaard, Andrew Lunn,
Ulf Hansson, Brian Norris, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, David S. Miller, Jakub Kicinski,
Joakim Zhang, Yoshihiro Shimoda, Kishon Vijay Abraham I,
Vinod Koul, Benson Leung, Guenter Roeck, Zha Qipeng,
Hans de Goede, Mark Gross, John Garry, Mark Brown,
Matthias Brugger, Niklas Söderlund, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Jiri Slaby, Eric Auger, Alex Williamson,
Cornelia Huck, Liam Girdwood, Jaroslav Kysela, Takashi Iwai,
openipmi-developer, linux-iio, linux-gpio, linux-pwm, linux-i2c,
linux-arm-kernel, linux-mmc, linux-mtd, netdev, linux-renesas-soc,
linux-phy, platform-driver-x86, linux-spi, linux-mediatek,
linux-pm, linux-serial, kvm, alsa-devel, Matthias Schiffer
Hello!
On 2/14/22 11:54 AM, Geert Uytterhoeven wrote:
[...]
>> This patch is based on the former Andy Shevchenko's patch:
>>
>> https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
>>
>> 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@ew.tq-group.com>
>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>> ---
>> Changes in version 2:
>
> Thanks for the update!
>
>> drivers/base/platform.c | 60 +++++++++++++++---------
>
> The core change LGTM.
Thanx! :-)
> 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?
Yes.
[...]
>> 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?
Yes.
>> --- 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.
Hm, you're right... should be <= 0 there, it seems.
> As this adds missing error handling, this is to be fixed by a separate
> patch later?
Propagating errors from the probe() method is a matter of separate patch, yes.
>>
>> 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".
Yes, probably... TY!
> Gr{oetje,eeting}s,
>
> Geert
MBR, Sergey
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 10:43 ` Sergey Shtylyov
0 siblings, 0 replies; 44+ messages in thread
From: Sergey Shtylyov @ 2022-02-14 10:43 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel,
Andy Shevchenko, Corey Minyard, Oleksij Rempel,
Pengutronix Kernel Team, William Breathitt Gray, Mun Yew Tham,
Linus Walleij, Bartosz Golaszewski, Thierry Reding,
Uwe Kleine-König, Lee Jones, Kamal Dasu, Florian Fainelli,
bcm-kernel-feedback-list, Peter Korsgaard, Andrew Lunn,
Ulf Hansson, Brian Norris, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, David S. Miller, Jakub Kicinski,
Joakim Zhang, Yoshihiro Shimoda, Kishon Vijay Abraham I,
Vinod Koul, Benson Leung, Guenter Roeck, Zha Qipeng,
Hans de Goede, Mark Gross, John Garry, Mark Brown,
Matthias Brugger, Niklas Söderlund, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Jiri Slaby, Eric Auger, Alex Williamson,
Cornelia Huck, Liam Girdwood, Jaroslav Kysela, Takashi Iwai,
openipmi-developer, linux-iio, linux-gpio, linux-pwm, linux-i2c,
linux-arm-kernel, linux-mmc, linux-mtd, netdev, linux-renesas-soc,
linux-phy, platform-driver-x86, linux-spi, linux-mediatek,
linux-pm, linux-serial, kvm, alsa-devel, Matthias Schiffer
Hello!
On 2/14/22 11:54 AM, Geert Uytterhoeven wrote:
[...]
>> This patch is based on the former Andy Shevchenko's patch:
>>
>> https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
>>
>> 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@ew.tq-group.com>
>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>> ---
>> Changes in version 2:
>
> Thanks for the update!
>
>> drivers/base/platform.c | 60 +++++++++++++++---------
>
> The core change LGTM.
Thanx! :-)
> 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?
Yes.
[...]
>> 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?
Yes.
>> --- 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.
Hm, you're right... should be <= 0 there, it seems.
> As this adds missing error handling, this is to be fixed by a separate
> patch later?
Propagating errors from the probe() method is a matter of separate patch, yes.
>>
>> 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".
Yes, probably... TY!
> Gr{oetje,eeting}s,
>
> Geert
MBR, Sergey
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 10:43 ` Sergey Shtylyov
0 siblings, 0 replies; 44+ messages in thread
From: Sergey Shtylyov @ 2022-02-14 10:43 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel,
Andy Shevchenko, Corey Minyard, Oleksij Rempel,
Pengutronix Kernel Team, William Breathitt Gray, Mun Yew Tham,
Linus Walleij, Bartosz Golaszewski, Thierry Reding,
Uwe Kleine-König, Lee Jones, Kamal Dasu, Florian Fainelli,
bcm-kernel-feedback-list, Peter Korsgaard, Andrew Lunn,
Ulf Hansson, Brian Norris, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, David S. Miller, Jakub Kicinski,
Joakim Zhang, Yoshihiro Shimoda, Kishon Vijay Abraham I,
Vinod Koul, Benson Leung, Guenter Roeck, Zha Qipeng,
Hans de Goede, Mark Gross, John Garry, Mark Brown,
Matthias Brugger, Niklas Söderlund, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Jiri Slaby, Eric Auger, Alex Williamson,
Cornelia Huck, Liam Girdwood, Jaroslav Kysela, Takashi Iwai,
openipmi-developer, linux-iio, linux-gpio, linux-pwm, linux-i2c,
linux-arm-kernel, linux-mmc, linux-mtd, netdev, linux-renesas-soc,
linux-phy, platform-driver-x86, linux-spi, linux-mediatek,
linux-pm, linux-serial, kvm, alsa-devel, Matthias Schiffer
Hello!
On 2/14/22 11:54 AM, Geert Uytterhoeven wrote:
[...]
>> This patch is based on the former Andy Shevchenko's patch:
>>
>> https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
>>
>> 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@ew.tq-group.com>
>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>> ---
>> Changes in version 2:
>
> Thanks for the update!
>
>> drivers/base/platform.c | 60 +++++++++++++++---------
>
> The core change LGTM.
Thanx! :-)
> 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?
Yes.
[...]
>> 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?
Yes.
>> --- 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.
Hm, you're right... should be <= 0 there, it seems.
> As this adds missing error handling, this is to be fixed by a separate
> patch later?
Propagating errors from the probe() method is a matter of separate patch, yes.
>>
>> 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".
Yes, probably... TY!
> Gr{oetje,eeting}s,
>
> Geert
MBR, Sergey
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v2 1/2] platform: make platform_get_irq_optional() optional
@ 2022-02-14 10:43 ` Sergey Shtylyov
0 siblings, 0 replies; 44+ messages in thread
From: Sergey Shtylyov @ 2022-02-14 10:43 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel,
Andy Shevchenko, Corey Minyard, Oleksij Rempel,
Pengutronix Kernel Team, William Breathitt Gray, Mun Yew Tham,
Linus Walleij, Bartosz Golaszewski, Thierry Reding,
Uwe Kleine-König, Lee Jones, Kamal Dasu, Florian Fainelli,
bcm-kernel-feedback-list, Peter Korsgaard, Andrew Lunn,
Ulf Hansson, Brian Norris, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, David S. Miller, Jakub Kicinski,
Joakim Zhang, Yoshihiro Shimoda, Kishon Vijay Abraham I,
Vinod Koul, Benson Leung, Guenter Roeck, Zha Qipeng,
Hans de Goede, Mark Gross, John Garry, Mark Brown,
Matthias Brugger, Niklas Söderlund, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Jiri Slaby, Eric Auger, Alex Williamson,
Cornelia Huck, Liam Girdwood, Jaroslav Kysela, Takashi Iwai,
openipmi-developer, linux-iio, linux-gpio, linux-pwm, linux-i2c,
linux-arm-kernel, linux-mmc, linux-mtd, netdev, linux-renesas-soc,
linux-phy, platform-driver-x86, linux-spi, linux-mediatek,
linux-pm, linux-serial, kvm, alsa-devel, Matthias Schiffer
Hello!
On 2/14/22 11:54 AM, Geert Uytterhoeven wrote:
[...]
>> This patch is based on the former Andy Shevchenko's patch:
>>
>> https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/
>>
>> 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@ew.tq-group.com>
>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
>> ---
>> Changes in version 2:
>
> Thanks for the update!
>
>> drivers/base/platform.c | 60 +++++++++++++++---------
>
> The core change LGTM.
Thanx! :-)
> 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?
Yes.
[...]
>> 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?
Yes.
>> --- 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.
Hm, you're right... should be <= 0 there, it seems.
> As this adds missing error handling, this is to be fixed by a separate
> patch later?
Propagating errors from the probe() method is a matter of separate patch, yes.
>>
>> 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".
Yes, probably... TY!
> Gr{oetje,eeting}s,
>
> Geert
MBR, Sergey
^ permalink raw reply [flat|nested] 44+ messages in thread