It will call devm_request_irq() after platform_get_irq() function
in many drivers, sometimes, it is not right for the error handling
of these two functions in some drivers. so provide this function
to simplify the driver.
the first patch will provide devm_platform_request_irq(), and the
other patch will convert to devm_platform_request_irq() in some
i2c bus dirver.
v1 -> v2:
- I give up this series of patches in v1 version. I resend this
patches v2 by that discussion:
https://patchwork.ozlabs.org/project/linux-i2c/patch/[email protected]/
The patch content has not changed.
Dejin Zheng (2):
drivers: provide devm_platform_request_irq()
i2c: busses: convert to devm_platform_request_irq()
drivers/base/platform.c | 33 ++++++++++++++++++++++++++++++
drivers/i2c/busses/i2c-bcm-kona.c | 16 +++------------
drivers/i2c/busses/i2c-cadence.c | 10 +++------
drivers/i2c/busses/i2c-digicolor.c | 10 +++------
drivers/i2c/busses/i2c-emev2.c | 5 ++---
drivers/i2c/busses/i2c-jz4780.c | 5 ++---
drivers/i2c/busses/i2c-meson.c | 13 ++++--------
drivers/i2c/busses/i2c-mxs.c | 9 +++-----
drivers/i2c/busses/i2c-pnx.c | 9 ++------
drivers/i2c/busses/i2c-rcar.c | 9 +++-----
drivers/i2c/busses/i2c-rk3x.c | 14 +++----------
drivers/i2c/busses/i2c-sirf.c | 10 ++-------
drivers/i2c/busses/i2c-stu300.c | 4 ++--
drivers/i2c/busses/i2c-synquacer.c | 12 +++--------
include/linux/platform_device.h | 4 ++++
15 files changed, 72 insertions(+), 91 deletions(-)
--
2.25.0
It will call devm_request_irq() after platform_get_irq() function
in many drivers, sometimes, it is not right for the error handling
of these two functions in some drivers. so provide this function
to simplify the driver.
Cc: Michal Simek <[email protected]>
Cc: Wolfram Sang <[email protected]>
Signed-off-by: Dejin Zheng <[email protected]>
---
v1 -> v2:
- The patch content has not changed. just resend it by this discussion:
https://patchwork.ozlabs.org/project/linux-i2c/patch/[email protected]/
drivers/base/platform.c | 33 +++++++++++++++++++++++++++++++++
include/linux/platform_device.h | 4 ++++
2 files changed, 37 insertions(+)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index c0d0a5490ac6..1d2fd1ea3bc5 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -275,6 +275,39 @@ int platform_irq_count(struct platform_device *dev)
}
EXPORT_SYMBOL_GPL(platform_irq_count);
+/**
+ * devm_platform_request_irq - get an irq and allocate an interrupt
+ * line for a managed device
+ * @pdev: platform device
+ * @num: IRQ number index
+ * @irq: get an IRQ for a device if irq != NULL
+ * @handler: function to be called when the IRQ occurs
+ * @irqflags: interrupt type flags
+ * @devname: an ascii name for the claiming device, dev_name(dev) if NULL
+ * @dev_id: a cookie passed back to the handler function
+ *
+ * Return: zero on success, negative error number on failure.
+ */
+int devm_platform_request_irq(struct platform_device *pdev, unsigned int num,
+ unsigned int *irq, irq_handler_t handler,
+ unsigned long irqflags, const char *devname, void *dev_id)
+{
+ int tmp_irq, ret;
+
+ tmp_irq = platform_get_irq(pdev, num);
+ if (tmp_irq < 0)
+ return tmp_irq;
+
+ ret = devm_request_irq(&pdev->dev, tmp_irq, handler, irqflags,
+ devname, dev_id);
+ if (ret < 0)
+ dev_err(&pdev->dev, "can't request IRQ\n");
+ else if (irq != NULL)
+ *irq = tmp_irq;
+ return ret;
+}
+EXPORT_SYMBOL_GPL(devm_platform_request_irq);
+
/**
* platform_get_resource_byname - get a resource for a device by name
* @dev: platform device
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 77a2aada106d..d94652deea5c 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -11,6 +11,7 @@
#define _PLATFORM_DEVICE_H_
#include <linux/device.h>
+#include <linux/interrupt.h>
#define PLATFORM_DEVID_NONE (-1)
#define PLATFORM_DEVID_AUTO (-2)
@@ -70,6 +71,9 @@ devm_platform_ioremap_resource_byname(struct platform_device *pdev,
extern int platform_get_irq(struct platform_device *, unsigned int);
extern int platform_get_irq_optional(struct platform_device *, unsigned int);
extern int platform_irq_count(struct platform_device *);
+extern int devm_platform_request_irq(struct platform_device *pdev,
+ unsigned int num, unsigned int *irq, irq_handler_t handler,
+ unsigned long irqflags, const char *devname, void *dev_id);
extern struct resource *platform_get_resource_byname(struct platform_device *,
unsigned int,
const char *);
--
2.25.0
Use devm_platform_request_irq() to simplify code, and it contains
platform_get_irq() and devm_request_irq().
I resend this patch by that discussion.
https://patchwork.ozlabs.org/project/linux-i2c/patch/[email protected]/
Cc: Michal Simek <[email protected]>
Cc: Wolfram Sang <[email protected]>
Signed-off-by: Dejin Zheng <[email protected]>
---
v1 -> v2:
- The patch content has not changed. just resend it by this discussion:
https://patchwork.ozlabs.org/project/linux-i2c/patch/[email protected]/
drivers/i2c/busses/i2c-bcm-kona.c | 16 +++-------------
drivers/i2c/busses/i2c-cadence.c | 10 +++-------
drivers/i2c/busses/i2c-digicolor.c | 10 +++-------
drivers/i2c/busses/i2c-emev2.c | 5 ++---
drivers/i2c/busses/i2c-jz4780.c | 5 ++---
drivers/i2c/busses/i2c-meson.c | 13 ++++---------
drivers/i2c/busses/i2c-mxs.c | 9 +++------
drivers/i2c/busses/i2c-pnx.c | 9 ++-------
drivers/i2c/busses/i2c-rcar.c | 9 +++------
drivers/i2c/busses/i2c-rk3x.c | 14 +++-----------
drivers/i2c/busses/i2c-sirf.c | 10 ++--------
drivers/i2c/busses/i2c-stu300.c | 4 ++--
drivers/i2c/busses/i2c-synquacer.c | 12 +++---------
13 files changed, 35 insertions(+), 91 deletions(-)
diff --git a/drivers/i2c/busses/i2c-bcm-kona.c b/drivers/i2c/busses/i2c-bcm-kona.c
index ed5e1275ae46..f45acb47552a 100644
--- a/drivers/i2c/busses/i2c-bcm-kona.c
+++ b/drivers/i2c/busses/i2c-bcm-kona.c
@@ -818,20 +818,10 @@ static int bcm_kona_i2c_probe(struct platform_device *pdev)
ISR_NOACK_MASK,
dev->base + ISR_OFFSET);
- /* Get the interrupt number */
- dev->irq = platform_get_irq(pdev, 0);
- if (dev->irq < 0) {
- rc = dev->irq;
- goto probe_disable_clk;
- }
-
- /* register the ISR handler */
- rc = devm_request_irq(&pdev->dev, dev->irq, bcm_kona_i2c_isr,
- IRQF_SHARED, pdev->name, dev);
- if (rc) {
- dev_err(dev->device, "failed to request irq %i\n", dev->irq);
+ rc = devm_platform_request_irq(pdev, 0, &dev->irq, bcm_kona_i2c_isr,
+ IRQF_SHARED, pdev->name, dev);
+ if (rc)
goto probe_disable_clk;
- }
/* Enable the controller but leave it idle */
bcm_kona_i2c_send_cmd_to_ctrl(dev, BCM_CMD_NOACTION);
diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index 4b72398af505..9ffae4d231dc 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -1204,8 +1204,6 @@ static int cdns_i2c_probe(struct platform_device *pdev)
if (IS_ERR(id->membase))
return PTR_ERR(id->membase);
- id->irq = platform_get_irq(pdev, 0);
-
id->adap.owner = THIS_MODULE;
id->adap.dev.of_node = pdev->dev.of_node;
id->adap.algo = &cdns_i2c_algo;
@@ -1256,12 +1254,10 @@ static int cdns_i2c_probe(struct platform_device *pdev)
goto err_clk_dis;
}
- ret = devm_request_irq(&pdev->dev, id->irq, cdns_i2c_isr, 0,
- DRIVER_NAME, id);
- if (ret) {
- dev_err(&pdev->dev, "cannot get irq %d\n", id->irq);
+ ret = devm_platform_request_irq(pdev, 0, &id->irq, cdns_i2c_isr, 0,
+ DRIVER_NAME, id);
+ if (ret)
goto err_clk_dis;
- }
/*
* Cadence I2C controller has a bug wherein it generates
diff --git a/drivers/i2c/busses/i2c-digicolor.c b/drivers/i2c/busses/i2c-digicolor.c
index 332f00437479..9ea965f41396 100644
--- a/drivers/i2c/busses/i2c-digicolor.c
+++ b/drivers/i2c/busses/i2c-digicolor.c
@@ -290,7 +290,7 @@ static int dc_i2c_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
struct dc_i2c *i2c;
- int ret = 0, irq;
+ int ret = 0;
i2c = devm_kzalloc(&pdev->dev, sizeof(struct dc_i2c), GFP_KERNEL);
if (!i2c)
@@ -314,12 +314,8 @@ static int dc_i2c_probe(struct platform_device *pdev)
if (IS_ERR(i2c->regs))
return PTR_ERR(i2c->regs);
- irq = platform_get_irq(pdev, 0);
- if (irq < 0)
- return irq;
-
- ret = devm_request_irq(&pdev->dev, irq, dc_i2c_irq, 0,
- dev_name(&pdev->dev), i2c);
+ ret = devm_platform_request_irq(pdev, 0, NULL, dc_i2c_irq, 0,
+ dev_name(&pdev->dev), i2c);
if (ret < 0)
return ret;
diff --git a/drivers/i2c/busses/i2c-emev2.c b/drivers/i2c/busses/i2c-emev2.c
index 1a319352e51b..cae00a9ec86f 100644
--- a/drivers/i2c/busses/i2c-emev2.c
+++ b/drivers/i2c/busses/i2c-emev2.c
@@ -395,9 +395,8 @@ static int em_i2c_probe(struct platform_device *pdev)
em_i2c_reset(&priv->adap);
- priv->irq = platform_get_irq(pdev, 0);
- ret = devm_request_irq(&pdev->dev, priv->irq, em_i2c_irq_handler, 0,
- "em_i2c", priv);
+ ret = devm_platform_request_irq(pdev, 0, &priv->irq,
+ em_i2c_irq_handler, 0, "em_i2c", priv);
if (ret)
goto err_clk;
diff --git a/drivers/i2c/busses/i2c-jz4780.c b/drivers/i2c/busses/i2c-jz4780.c
index ba831df6661e..27de0309f211 100644
--- a/drivers/i2c/busses/i2c-jz4780.c
+++ b/drivers/i2c/busses/i2c-jz4780.c
@@ -825,9 +825,8 @@ static int jz4780_i2c_probe(struct platform_device *pdev)
jz4780_i2c_writew(i2c, JZ4780_I2C_INTM, 0x0);
- i2c->irq = platform_get_irq(pdev, 0);
- ret = devm_request_irq(&pdev->dev, i2c->irq, jz4780_i2c_irq, 0,
- dev_name(&pdev->dev), i2c);
+ ret = devm_platform_request_irq(pdev, 0, &i2c->irq, jz4780_i2c_irq, 0,
+ dev_name(&pdev->dev), i2c);
if (ret)
goto err;
diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index c5dec572fc48..2e5a855ff20a 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -398,7 +398,7 @@ static int meson_i2c_probe(struct platform_device *pdev)
struct device_node *np = pdev->dev.of_node;
struct meson_i2c *i2c;
struct i2c_timings timings;
- int irq, ret = 0;
+ int ret = 0;
i2c = devm_kzalloc(&pdev->dev, sizeof(struct meson_i2c), GFP_KERNEL);
if (!i2c)
@@ -425,15 +425,10 @@ static int meson_i2c_probe(struct platform_device *pdev)
if (IS_ERR(i2c->regs))
return PTR_ERR(i2c->regs);
- irq = platform_get_irq(pdev, 0);
- if (irq < 0)
- return irq;
-
- ret = devm_request_irq(&pdev->dev, irq, meson_i2c_irq, 0, NULL, i2c);
- if (ret < 0) {
- dev_err(&pdev->dev, "can't request IRQ\n");
+ ret = devm_platform_request_irq(pdev, 0, NULL, meson_i2c_irq,
+ 0, NULL, i2c);
+ if (ret < 0)
return ret;
- }
ret = clk_prepare(i2c->clk);
if (ret < 0) {
diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 9587347447f0..cff82af3208a 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -802,7 +802,7 @@ static int mxs_i2c_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct mxs_i2c_dev *i2c;
struct i2c_adapter *adap;
- int err, irq;
+ int err;
i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
if (!i2c)
@@ -817,11 +817,8 @@ static int mxs_i2c_probe(struct platform_device *pdev)
if (IS_ERR(i2c->regs))
return PTR_ERR(i2c->regs);
- irq = platform_get_irq(pdev, 0);
- if (irq < 0)
- return irq;
-
- err = devm_request_irq(dev, irq, mxs_i2c_isr, 0, dev_name(dev), i2c);
+ err = devm_platform_request_irq(pdev, 0, NULL, mxs_i2c_isr, 0,
+ dev_name(dev), i2c);
if (err)
return err;
diff --git a/drivers/i2c/busses/i2c-pnx.c b/drivers/i2c/busses/i2c-pnx.c
index 5d7207c10f1d..3e249373375f 100644
--- a/drivers/i2c/busses/i2c-pnx.c
+++ b/drivers/i2c/busses/i2c-pnx.c
@@ -718,13 +718,8 @@ static int i2c_pnx_probe(struct platform_device *pdev)
}
init_completion(&alg_data->mif.complete);
- alg_data->irq = platform_get_irq(pdev, 0);
- if (alg_data->irq < 0) {
- ret = alg_data->irq;
- goto out_clock;
- }
- ret = devm_request_irq(&pdev->dev, alg_data->irq, i2c_pnx_interrupt,
- 0, pdev->name, alg_data);
+ ret = devm_platform_request_irq(pdev, 0, &alg_data->irq,
+ i2c_pnx_interrupt, 0, pdev->name, alg_data);
if (ret)
goto out_clock;
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index a45c4bf1ec01..bd59a13de707 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -984,13 +984,10 @@ static int rcar_i2c_probe(struct platform_device *pdev)
else
pm_runtime_put(dev);
-
- priv->irq = platform_get_irq(pdev, 0);
- ret = devm_request_irq(dev, priv->irq, rcar_i2c_irq, 0, dev_name(dev), priv);
- if (ret < 0) {
- dev_err(dev, "cannot get irq %d\n", priv->irq);
+ ret = devm_platform_request_irq(pdev, 0, &priv->irq, rcar_i2c_irq, 0,
+ dev_name(dev), priv);
+ if (ret < 0)
goto out_pm_disable;
- }
platform_set_drvdata(pdev, priv);
diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index bc698240c4aa..a8d47689de0a 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -1196,7 +1196,6 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
int ret = 0;
int bus_nr;
u32 value;
- int irq;
unsigned long clk_rate;
i2c = devm_kzalloc(&pdev->dev, sizeof(struct rk3x_i2c), GFP_KERNEL);
@@ -1258,17 +1257,10 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
}
}
- /* IRQ setup */
- irq = platform_get_irq(pdev, 0);
- if (irq < 0)
- return irq;
-
- ret = devm_request_irq(&pdev->dev, irq, rk3x_i2c_irq,
- 0, dev_name(&pdev->dev), i2c);
- if (ret < 0) {
- dev_err(&pdev->dev, "cannot request IRQ\n");
+ ret = devm_platform_request_irq(pdev, 0, NULL, rk3x_i2c_irq,
+ 0, dev_name(&pdev->dev), i2c);
+ if (ret < 0)
return ret;
- }
platform_set_drvdata(pdev, i2c);
diff --git a/drivers/i2c/busses/i2c-sirf.c b/drivers/i2c/busses/i2c-sirf.c
index d7f72ec331e8..a593c15bfbf5 100644
--- a/drivers/i2c/busses/i2c-sirf.c
+++ b/drivers/i2c/busses/i2c-sirf.c
@@ -274,7 +274,6 @@ static int i2c_sirfsoc_probe(struct platform_device *pdev)
struct clk *clk;
int bitrate;
int ctrl_speed;
- int irq;
int err;
u32 regval;
@@ -314,13 +313,8 @@ static int i2c_sirfsoc_probe(struct platform_device *pdev)
goto out;
}
- irq = platform_get_irq(pdev, 0);
- if (irq < 0) {
- err = irq;
- goto out;
- }
- err = devm_request_irq(&pdev->dev, irq, i2c_sirfsoc_irq, 0,
- dev_name(&pdev->dev), siic);
+ err = devm_platform_request_irq(pdev, 0, NULL, i2c_sirfsoc_irq, 0,
+ dev_name(&pdev->dev), siic);
if (err)
goto out;
diff --git a/drivers/i2c/busses/i2c-stu300.c b/drivers/i2c/busses/i2c-stu300.c
index 64d739baf480..7893c532b8f2 100644
--- a/drivers/i2c/busses/i2c-stu300.c
+++ b/drivers/i2c/busses/i2c-stu300.c
@@ -881,8 +881,8 @@ static int stu300_probe(struct platform_device *pdev)
if (IS_ERR(dev->virtbase))
return PTR_ERR(dev->virtbase);
- dev->irq = platform_get_irq(pdev, 0);
- ret = devm_request_irq(&pdev->dev, dev->irq, stu300_irh, 0, NAME, dev);
+ ret = devm_platform_request_irq(pdev, 0, &dev->irq, stu300_irh,
+ 0, NAME, dev);
if (ret < 0)
return ret;
diff --git a/drivers/i2c/busses/i2c-synquacer.c b/drivers/i2c/busses/i2c-synquacer.c
index c9a3dba6a75d..d9143373e688 100644
--- a/drivers/i2c/busses/i2c-synquacer.c
+++ b/drivers/i2c/busses/i2c-synquacer.c
@@ -577,16 +577,10 @@ static int synquacer_i2c_probe(struct platform_device *pdev)
if (IS_ERR(i2c->base))
return PTR_ERR(i2c->base);
- i2c->irq = platform_get_irq(pdev, 0);
- if (i2c->irq < 0)
- return -ENODEV;
-
- ret = devm_request_irq(&pdev->dev, i2c->irq, synquacer_i2c_isr,
- 0, dev_name(&pdev->dev), i2c);
- if (ret < 0) {
- dev_err(&pdev->dev, "cannot claim IRQ %d\n", i2c->irq);
+ ret = devm_platform_request_irq(pdev, 0, &i2c->irq, synquacer_i2c_isr,
+ 0, dev_name(&pdev->dev), i2c);
+ if (ret < 0)
return ret;
- }
i2c->state = STATE_IDLE;
i2c->dev = &pdev->dev;
--
2.25.0
On Sat, May 23, 2020 at 10:51:55PM +0800, Dejin Zheng wrote:
> It will call devm_request_irq() after platform_get_irq() function
> in many drivers, sometimes, it is not right for the error handling
> of these two functions in some drivers. so provide this function
> to simplify the driver.
>
> the first patch will provide devm_platform_request_irq(), and the
> other patch will convert to devm_platform_request_irq() in some
> i2c bus dirver.
>
> v1 -> v2:
> - I give up this series of patches in v1 version. I resend this
> patches v2 by that discussion:
> https://patchwork.ozlabs.org/project/linux-i2c/patch/[email protected]/
> The patch content has not changed.
I don't understand. v1 has been nacked because of technical reasons. How
did the discussion above change the situation? Am I missing something?
>
> Dejin Zheng (2):
> drivers: provide devm_platform_request_irq()
> i2c: busses: convert to devm_platform_request_irq()
>
> drivers/base/platform.c | 33 ++++++++++++++++++++++++++++++
> drivers/i2c/busses/i2c-bcm-kona.c | 16 +++------------
> drivers/i2c/busses/i2c-cadence.c | 10 +++------
> drivers/i2c/busses/i2c-digicolor.c | 10 +++------
> drivers/i2c/busses/i2c-emev2.c | 5 ++---
> drivers/i2c/busses/i2c-jz4780.c | 5 ++---
> drivers/i2c/busses/i2c-meson.c | 13 ++++--------
> drivers/i2c/busses/i2c-mxs.c | 9 +++-----
> drivers/i2c/busses/i2c-pnx.c | 9 ++------
> drivers/i2c/busses/i2c-rcar.c | 9 +++-----
> drivers/i2c/busses/i2c-rk3x.c | 14 +++----------
> drivers/i2c/busses/i2c-sirf.c | 10 ++-------
> drivers/i2c/busses/i2c-stu300.c | 4 ++--
> drivers/i2c/busses/i2c-synquacer.c | 12 +++--------
> include/linux/platform_device.h | 4 ++++
> 15 files changed, 72 insertions(+), 91 deletions(-)
>
> --
> 2.25.0
>
On Sat, May 23, 2020 at 06:08:29PM +0200, Wolfram Sang wrote:
> On Sat, May 23, 2020 at 10:51:55PM +0800, Dejin Zheng wrote:
> > It will call devm_request_irq() after platform_get_irq() function
> > in many drivers, sometimes, it is not right for the error handling
> > of these two functions in some drivers. so provide this function
> > to simplify the driver.
> >
> > the first patch will provide devm_platform_request_irq(), and the
> > other patch will convert to devm_platform_request_irq() in some
> > i2c bus dirver.
> >
> > v1 -> v2:
> > - I give up this series of patches in v1 version. I resend this
> > patches v2 by that discussion:
> > https://patchwork.ozlabs.org/project/linux-i2c/patch/[email protected]/
> > The patch content has not changed.
>
> I don't understand. v1 has been nacked because of technical reasons. How
> did the discussion above change the situation? Am I missing something?
>
No, you are not missing something. Maybe I did not explain clearly.
The v1 has been nacked because Grygorii told me that the
function platform_get_irq() should be done as early as possible to avoid
unnecessary initialization steps, and the function devm_request_irq()
should be done late in probe when driver and HW are actually ready to
handle IRQs. It can do the other things between the two funtions. I agree
with him that it may be necessary in some complex drives. So abandon the
patch v1.
Base on the discussion of you and Michal, I think maybe this patch is also
needed for the simple driver or the driver of already use it like that:
irq = platform_get_irq(pdev, 0);
if (irq < 0)
return irq;
ret = devm_request_irq()
It provides a common error handling and reduce one function call for each
drivers, more easier to use and simplify code. So resend it.
BR,
Dejin
> >
> > Dejin Zheng (2):
> > drivers: provide devm_platform_request_irq()
> > i2c: busses: convert to devm_platform_request_irq()
> >
> > drivers/base/platform.c | 33 ++++++++++++++++++++++++++++++
> > drivers/i2c/busses/i2c-bcm-kona.c | 16 +++------------
> > drivers/i2c/busses/i2c-cadence.c | 10 +++------
> > drivers/i2c/busses/i2c-digicolor.c | 10 +++------
> > drivers/i2c/busses/i2c-emev2.c | 5 ++---
> > drivers/i2c/busses/i2c-jz4780.c | 5 ++---
> > drivers/i2c/busses/i2c-meson.c | 13 ++++--------
> > drivers/i2c/busses/i2c-mxs.c | 9 +++-----
> > drivers/i2c/busses/i2c-pnx.c | 9 ++------
> > drivers/i2c/busses/i2c-rcar.c | 9 +++-----
> > drivers/i2c/busses/i2c-rk3x.c | 14 +++----------
> > drivers/i2c/busses/i2c-sirf.c | 10 ++-------
> > drivers/i2c/busses/i2c-stu300.c | 4 ++--
> > drivers/i2c/busses/i2c-synquacer.c | 12 +++--------
> > include/linux/platform_device.h | 4 ++++
> > 15 files changed, 72 insertions(+), 91 deletions(-)
> >
> > --
> > 2.25.0
> >
> It will call devm_request_irq() after platform_get_irq() function
> in many drivers, sometimes, it is not right for the error handling
> of these two functions in some drivers. so provide this function
> to simplify the driver.
I recommend to improve also this change description.
How do you think about a wording variant like the following?
The function “devm_request_irq” is called after the
function “platform_get_irq” in many drivers.
The exception handling is incomplete there sometimes.
Thus add a corresponding wrapper function for the simplification
of the drivers.
Will a companion script for the semantic patch language (Coccinelle software)
become helpful for further support of collateral evolution?
Regards,
Markus
On Sat, May 23, 2020 at 4:52 PM Dejin Zheng <[email protected]> wrote:
> Use devm_platform_request_irq() to simplify code, and it contains
> platform_get_irq() and devm_request_irq().
>
> I resend this patch by that discussion.
> https://patchwork.ozlabs.org/project/linux-i2c/patch/[email protected]/
>
> Cc: Michal Simek <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Signed-off-by: Dejin Zheng <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On 23. 05. 20 19:09, Dejin Zheng wrote:
> On Sat, May 23, 2020 at 06:08:29PM +0200, Wolfram Sang wrote:
>> On Sat, May 23, 2020 at 10:51:55PM +0800, Dejin Zheng wrote:
>>> It will call devm_request_irq() after platform_get_irq() function
>>> in many drivers, sometimes, it is not right for the error handling
>>> of these two functions in some drivers. so provide this function
>>> to simplify the driver.
>>>
>>> the first patch will provide devm_platform_request_irq(), and the
>>> other patch will convert to devm_platform_request_irq() in some
>>> i2c bus dirver.
>>>
>>> v1 -> v2:
>>> - I give up this series of patches in v1 version. I resend this
>>> patches v2 by that discussion:
>>> https://patchwork.ozlabs.org/project/linux-i2c/patch/[email protected]/
>>> The patch content has not changed.
>>
>> I don't understand. v1 has been nacked because of technical reasons. How
>> did the discussion above change the situation? Am I missing something?
>>
> No, you are not missing something. Maybe I did not explain clearly.
>
> The v1 has been nacked because Grygorii told me that the
> function platform_get_irq() should be done as early as possible to avoid
> unnecessary initialization steps, and the function devm_request_irq()
> should be done late in probe when driver and HW are actually ready to
> handle IRQs. It can do the other things between the two funtions. I agree
> with him that it may be necessary in some complex drives. So abandon the
> patch v1.
>
> Base on the discussion of you and Michal, I think maybe this patch is also
> needed for the simple driver or the driver of already use it like that:
>
> irq = platform_get_irq(pdev, 0);
> if (irq < 0)
> return irq;
> ret = devm_request_irq()
>
> It provides a common error handling and reduce one function call for each
> drivers, more easier to use and simplify code. So resend it.
>
> BR,
> Dejin
>
>>>
>>> Dejin Zheng (2):
>>> drivers: provide devm_platform_request_irq()
>>> i2c: busses: convert to devm_platform_request_irq()
>>>
>>> drivers/base/platform.c | 33 ++++++++++++++++++++++++++++++
>>> drivers/i2c/busses/i2c-bcm-kona.c | 16 +++------------
>>> drivers/i2c/busses/i2c-cadence.c | 10 +++------
>>> drivers/i2c/busses/i2c-digicolor.c | 10 +++------
>>> drivers/i2c/busses/i2c-emev2.c | 5 ++---
>>> drivers/i2c/busses/i2c-jz4780.c | 5 ++---
>>> drivers/i2c/busses/i2c-meson.c | 13 ++++--------
>>> drivers/i2c/busses/i2c-mxs.c | 9 +++-----
>>> drivers/i2c/busses/i2c-pnx.c | 9 ++------
>>> drivers/i2c/busses/i2c-rcar.c | 9 +++-----
>>> drivers/i2c/busses/i2c-rk3x.c | 14 +++----------
>>> drivers/i2c/busses/i2c-sirf.c | 10 ++-------
>>> drivers/i2c/busses/i2c-stu300.c | 4 ++--
>>> drivers/i2c/busses/i2c-synquacer.c | 12 +++--------
>>> include/linux/platform_device.h | 4 ++++
>>> 15 files changed, 72 insertions(+), 91 deletions(-)
If you look at all driver except for cadence one it doesn't do any
change and I can't see any issue with it because sequences are the same
as were before.
Regarding Cadence and Grygorii's comments:
We are not checking that id->irq is valid that's why even if that fails
driver continues to work. Which means that this change doesn't increase
boot time or change code flow.
On Xilinx devices cadence i2c is connected to ARM GIC which is
initialized very early and IRC controller should be up and running all
the time.
That's why I can't see any issue which this change on Cadence driver too.
Thanks,
Michal
On 23/05/2020 17:51, Dejin Zheng wrote:
> It will call devm_request_irq() after platform_get_irq() function
> in many drivers, sometimes, it is not right for the error handling
> of these two functions in some drivers. so provide this function
> to simplify the driver.
>
> Cc: Michal Simek <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Signed-off-by: Dejin Zheng <[email protected]>
> ---
> v1 -> v2:
> - The patch content has not changed. just resend it by this discussion:
> https://patchwork.ozlabs.org/project/linux-i2c/patch/[email protected]/
>
> drivers/base/platform.c | 33 +++++++++++++++++++++++++++++++++
> include/linux/platform_device.h | 4 ++++
> 2 files changed, 37 insertions(+)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index c0d0a5490ac6..1d2fd1ea3bc5 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -275,6 +275,39 @@ int platform_irq_count(struct platform_device *dev)
> }
> EXPORT_SYMBOL_GPL(platform_irq_count);
>
> +/**
> + * devm_platform_request_irq - get an irq and allocate an interrupt
> + * line for a managed device
> + * @pdev: platform device
> + * @num: IRQ number index
> + * @irq: get an IRQ for a device if irq != NULL
> + * @handler: function to be called when the IRQ occurs
> + * @irqflags: interrupt type flags
> + * @devname: an ascii name for the claiming device, dev_name(dev) if NULL
> + * @dev_id: a cookie passed back to the handler function
> + *
> + * Return: zero on success, negative error number on failure.
> + */
> +int devm_platform_request_irq(struct platform_device *pdev, unsigned int num,
> + unsigned int *irq, irq_handler_t handler,
> + unsigned long irqflags, const char *devname, void *dev_id)
> +{
> + int tmp_irq, ret;
> +
> + tmp_irq = platform_get_irq(pdev, num);
> + if (tmp_irq < 0)
> + return tmp_irq;
> +
> + ret = devm_request_irq(&pdev->dev, tmp_irq, handler, irqflags,
> + devname, dev_id);
> + if (ret < 0)
> + dev_err(&pdev->dev, "can't request IRQ\n");
> + else if (irq != NULL)
> + *irq = tmp_irq;
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(devm_platform_request_irq);
> +
> /**
> * platform_get_resource_byname - get a resource for a device by name
> * @dev: platform device
> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> index 77a2aada106d..d94652deea5c 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -11,6 +11,7 @@
> #define _PLATFORM_DEVICE_H_
>
> #include <linux/device.h>
> +#include <linux/interrupt.h>
>
> #define PLATFORM_DEVID_NONE (-1)
> #define PLATFORM_DEVID_AUTO (-2)
> @@ -70,6 +71,9 @@ devm_platform_ioremap_resource_byname(struct platform_device *pdev,
> extern int platform_get_irq(struct platform_device *, unsigned int);
> extern int platform_get_irq_optional(struct platform_device *, unsigned int);
> extern int platform_irq_count(struct platform_device *);
> +extern int devm_platform_request_irq(struct platform_device *pdev,
> + unsigned int num, unsigned int *irq, irq_handler_t handler,
> + unsigned long irqflags, const char *devname, void *dev_id);
it has to be documented in devres.rst
> extern struct resource *platform_get_resource_byname(struct platform_device *,
> unsigned int,
> const char *);
>
--
Best regards,
grygorii
On 25/05/2020 10:05, Michal Simek wrote:
> On 23. 05. 20 19:09, Dejin Zheng wrote:
>> On Sat, May 23, 2020 at 06:08:29PM +0200, Wolfram Sang wrote:
>>> On Sat, May 23, 2020 at 10:51:55PM +0800, Dejin Zheng wrote:
>>>> It will call devm_request_irq() after platform_get_irq() function
>>>> in many drivers, sometimes, it is not right for the error handling
>>>> of these two functions in some drivers. so provide this function
>>>> to simplify the driver.
>>>>
>>>> the first patch will provide devm_platform_request_irq(), and the
>>>> other patch will convert to devm_platform_request_irq() in some
>>>> i2c bus dirver.
>>>>
>>>> v1 -> v2:
>>>> - I give up this series of patches in v1 version. I resend this
>>>> patches v2 by that discussion:
>>>> https://patchwork.ozlabs.org/project/linux-i2c/patch/[email protected]/
>>>> The patch content has not changed.
>>>
>>> I don't understand. v1 has been nacked because of technical reasons. How
>>> did the discussion above change the situation? Am I missing something?
>>>
>> No, you are not missing something. Maybe I did not explain clearly.
>>
>> The v1 has been nacked because Grygorii told me that the
>> function platform_get_irq() should be done as early as possible to avoid
>> unnecessary initialization steps, and the function devm_request_irq()
>> should be done late in probe when driver and HW are actually ready to
>> handle IRQs. It can do the other things between the two funtions. I agree
>> with him that it may be necessary in some complex drives. So abandon the
>> patch v1.
>>
>> Base on the discussion of you and Michal, I think maybe this patch is also
>> needed for the simple driver or the driver of already use it like that:
>>
>> irq = platform_get_irq(pdev, 0);
>> if (irq < 0)
>> return irq;
>> ret = devm_request_irq()
>>
>> It provides a common error handling and reduce one function call for each
>> drivers, more easier to use and simplify code. So resend it.
>>
>> BR,
>> Dejin
>>
>>>>
>>>> Dejin Zheng (2):
>>>> drivers: provide devm_platform_request_irq()
>>>> i2c: busses: convert to devm_platform_request_irq()
>>>>
>>>> drivers/base/platform.c | 33 ++++++++++++++++++++++++++++++
>>>> drivers/i2c/busses/i2c-bcm-kona.c | 16 +++------------
>>>> drivers/i2c/busses/i2c-cadence.c | 10 +++------
>>>> drivers/i2c/busses/i2c-digicolor.c | 10 +++------
>>>> drivers/i2c/busses/i2c-emev2.c | 5 ++---
>>>> drivers/i2c/busses/i2c-jz4780.c | 5 ++---
>>>> drivers/i2c/busses/i2c-meson.c | 13 ++++--------
>>>> drivers/i2c/busses/i2c-mxs.c | 9 +++-----
>>>> drivers/i2c/busses/i2c-pnx.c | 9 ++------
>>>> drivers/i2c/busses/i2c-rcar.c | 9 +++-----
>>>> drivers/i2c/busses/i2c-rk3x.c | 14 +++----------
>>>> drivers/i2c/busses/i2c-sirf.c | 10 ++-------
>>>> drivers/i2c/busses/i2c-stu300.c | 4 ++--
>>>> drivers/i2c/busses/i2c-synquacer.c | 12 +++--------
>>>> include/linux/platform_device.h | 4 ++++
>>>> 15 files changed, 72 insertions(+), 91 deletions(-)
>
> If you look at all driver except for cadence one it doesn't do any
> change and I can't see any issue with it because sequences are the same
> as were before.
>
> Regarding Cadence and Grygorii's comments:
> We are not checking that id->irq is valid that's why even if that fails
> driver continues to work. Which means that this change doesn't increase
> boot time or change code flow.
> On Xilinx devices cadence i2c is connected to ARM GIC which is
> initialized very early and IRC controller should be up and running all
> the time.
> That's why I can't see any issue which this change on Cadence driver too.
My main point was to pay attention on changes, which may be risky
especially when they are part of bulk changes (such optimization tend to spread
fast and all over the kernel without proper review).
Sry, if i introduced some misunderstanding, but it seems worked and this patch has got more attention.
There are no objection from my side to use devm_platform_get_and_ioremap_resource() if driver
owners find it acceptable.
--
Best regards,
grygorii
On Tue, May 26, 2020 at 08:13:25PM +0300, Grygorii Strashko wrote:
>
>
> On 25/05/2020 10:05, Michal Simek wrote:
> > On 23. 05. 20 19:09, Dejin Zheng wrote:
> > > On Sat, May 23, 2020 at 06:08:29PM +0200, Wolfram Sang wrote:
> > > > On Sat, May 23, 2020 at 10:51:55PM +0800, Dejin Zheng wrote:
> > > > > It will call devm_request_irq() after platform_get_irq() function
> > > > > in many drivers, sometimes, it is not right for the error handling
> > > > > of these two functions in some drivers. so provide this function
> > > > > to simplify the driver.
> > > > >
> > > > > the first patch will provide devm_platform_request_irq(), and the
> > > > > other patch will convert to devm_platform_request_irq() in some
> > > > > i2c bus dirver.
> > > > >
> > > > > v1 -> v2:
> > > > > - I give up this series of patches in v1 version. I resend this
> > > > > patches v2 by that discussion:
> > > > > https://patchwork.ozlabs.org/project/linux-i2c/patch/[email protected]/
> > > > > The patch content has not changed.
> > > >
> > > > I don't understand. v1 has been nacked because of technical reasons. How
> > > > did the discussion above change the situation? Am I missing something?
> > > >
> > > No, you are not missing something. Maybe I did not explain clearly.
> > >
> > > The v1 has been nacked because Grygorii told me that the
> > > function platform_get_irq() should be done as early as possible to avoid
> > > unnecessary initialization steps, and the function devm_request_irq()
> > > should be done late in probe when driver and HW are actually ready to
> > > handle IRQs. It can do the other things between the two funtions. I agree
> > > with him that it may be necessary in some complex drives. So abandon the
> > > patch v1.
> > >
> > > Base on the discussion of you and Michal, I think maybe this patch is also
> > > needed for the simple driver or the driver of already use it like that:
> > >
> > > irq = platform_get_irq(pdev, 0);
> > > if (irq < 0)
> > > return irq;
> > > ret = devm_request_irq()
> > >
> > > It provides a common error handling and reduce one function call for each
> > > drivers, more easier to use and simplify code. So resend it.
> > >
> > > BR,
> > > Dejin
> > >
> > > > >
> > > > > Dejin Zheng (2):
> > > > > drivers: provide devm_platform_request_irq()
> > > > > i2c: busses: convert to devm_platform_request_irq()
> > > > >
> > > > > drivers/base/platform.c | 33 ++++++++++++++++++++++++++++++
> > > > > drivers/i2c/busses/i2c-bcm-kona.c | 16 +++------------
> > > > > drivers/i2c/busses/i2c-cadence.c | 10 +++------
> > > > > drivers/i2c/busses/i2c-digicolor.c | 10 +++------
> > > > > drivers/i2c/busses/i2c-emev2.c | 5 ++---
> > > > > drivers/i2c/busses/i2c-jz4780.c | 5 ++---
> > > > > drivers/i2c/busses/i2c-meson.c | 13 ++++--------
> > > > > drivers/i2c/busses/i2c-mxs.c | 9 +++-----
> > > > > drivers/i2c/busses/i2c-pnx.c | 9 ++------
> > > > > drivers/i2c/busses/i2c-rcar.c | 9 +++-----
> > > > > drivers/i2c/busses/i2c-rk3x.c | 14 +++----------
> > > > > drivers/i2c/busses/i2c-sirf.c | 10 ++-------
> > > > > drivers/i2c/busses/i2c-stu300.c | 4 ++--
> > > > > drivers/i2c/busses/i2c-synquacer.c | 12 +++--------
> > > > > include/linux/platform_device.h | 4 ++++
> > > > > 15 files changed, 72 insertions(+), 91 deletions(-)
> >
> > If you look at all driver except for cadence one it doesn't do any
> > change and I can't see any issue with it because sequences are the same
> > as were before.
> >
> > Regarding Cadence and Grygorii's comments:
> > We are not checking that id->irq is valid that's why even if that fails
> > driver continues to work. Which means that this change doesn't increase
> > boot time or change code flow.
> > On Xilinx devices cadence i2c is connected to ARM GIC which is
> > initialized very early and IRC controller should be up and running all
> > the time.
> > That's why I can't see any issue which this change on Cadence driver too.
>
>
> My main point was to pay attention on changes, which may be risky
> especially when they are part of bulk changes (such optimization tend to spread
> fast and all over the kernel without proper review).
>
> Sry, if i introduced some misunderstanding, but it seems worked and this patch has got more attention.
> There are no objection from my side to use devm_platform_get_and_ioremap_resource() if driver
> owners find it acceptable.
>
This should be my misunderstanding regarding your comment in patch v1,
Anyway, thanks everyone for using your precious time to review my patch.
And also I very sorry for the Gmail will prevent me sending messages to
a large number of recipient, I had to reduce the number of recipients to
send this email. so sorry!
BR,
Dejin
> --
> Best regards,
> grygorii
On Tue, May 26, 2020 at 08:11:22PM +0300, Grygorii Strashko wrote:
>
>
> On 23/05/2020 17:51, Dejin Zheng wrote:
> > It will call devm_request_irq() after platform_get_irq() function
> > in many drivers, sometimes, it is not right for the error handling
> > of these two functions in some drivers. so provide this function
> > to simplify the driver.
> >
> > Cc: Michal Simek <[email protected]>
> > Cc: Wolfram Sang <[email protected]>
> > Signed-off-by: Dejin Zheng <[email protected]>
> > ---
> > v1 -> v2:
> > - The patch content has not changed. just resend it by this discussion:
> > https://patchwork.ozlabs.org/project/linux-i2c/patch/[email protected]/
> >
> > drivers/base/platform.c | 33 +++++++++++++++++++++++++++++++++
> > include/linux/platform_device.h | 4 ++++
> > 2 files changed, 37 insertions(+)
> >
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index c0d0a5490ac6..1d2fd1ea3bc5 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -275,6 +275,39 @@ int platform_irq_count(struct platform_device *dev)
> > }
> > EXPORT_SYMBOL_GPL(platform_irq_count);
> > +/**
> > + * devm_platform_request_irq - get an irq and allocate an interrupt
> > + * line for a managed device
> > + * @pdev: platform device
> > + * @num: IRQ number index
> > + * @irq: get an IRQ for a device if irq != NULL
> > + * @handler: function to be called when the IRQ occurs
> > + * @irqflags: interrupt type flags
> > + * @devname: an ascii name for the claiming device, dev_name(dev) if NULL
> > + * @dev_id: a cookie passed back to the handler function
> > + *
> > + * Return: zero on success, negative error number on failure.
> > + */
> > +int devm_platform_request_irq(struct platform_device *pdev, unsigned int num,
> > + unsigned int *irq, irq_handler_t handler,
> > + unsigned long irqflags, const char *devname, void *dev_id)
> > +{
> > + int tmp_irq, ret;
> > +
> > + tmp_irq = platform_get_irq(pdev, num);
> > + if (tmp_irq < 0)
> > + return tmp_irq;
> > +
> > + ret = devm_request_irq(&pdev->dev, tmp_irq, handler, irqflags,
> > + devname, dev_id);
> > + if (ret < 0)
> > + dev_err(&pdev->dev, "can't request IRQ\n");
> > + else if (irq != NULL)
> > + *irq = tmp_irq;
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(devm_platform_request_irq);
> > +
> > /**
> > * platform_get_resource_byname - get a resource for a device by name
> > * @dev: platform device
> > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> > index 77a2aada106d..d94652deea5c 100644
> > --- a/include/linux/platform_device.h
> > +++ b/include/linux/platform_device.h
> > @@ -11,6 +11,7 @@
> > #define _PLATFORM_DEVICE_H_
> > #include <linux/device.h>
> > +#include <linux/interrupt.h>
> > #define PLATFORM_DEVID_NONE (-1)
> > #define PLATFORM_DEVID_AUTO (-2)
> > @@ -70,6 +71,9 @@ devm_platform_ioremap_resource_byname(struct platform_device *pdev,
> > extern int platform_get_irq(struct platform_device *, unsigned int);
> > extern int platform_get_irq_optional(struct platform_device *, unsigned int);
> > extern int platform_irq_count(struct platform_device *);
> > +extern int devm_platform_request_irq(struct platform_device *pdev,
> > + unsigned int num, unsigned int *irq, irq_handler_t handler,
> > + unsigned long irqflags, const char *devname, void *dev_id);
>
>
> it has to be documented in devres.rst
>
Grygorii, Thnaks! I will add it in patch v3.
BTW, the Gmail will prevent me sending messages to a large number of
recipients, So I reduced some recipients, but still retained i2c and
linux kernel mail list. sorry!
BR,
Dejin
> > extern struct resource *platform_get_resource_byname(struct platform_device *,
> > unsigned int,
> > const char *);
> >
>
>
>
> --
> Best regards,
> grygorii