LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/2] drivers: provide devm_platform_request_irq()
@ 2020-05-23 14:51 Dejin Zheng
  2020-05-23 14:51 ` [PATCH v2 1/2] " Dejin Zheng
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Dejin Zheng @ 2020-05-23 14:51 UTC (permalink / raw)
  To: gregkh, rafael, f.fainelli, rjui, sbranden, michal.simek, baruch,
	paul, khilman, shawnguo, festevam, vz, heiko, linus.walleij,
	baohua, ardb, linux-i2c
  Cc: linux-kernel, Dejin Zheng

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/20200520144821.8069-1-zhengdejin5@gmail.com/
	  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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 1/2] drivers: provide devm_platform_request_irq()
  2020-05-23 14:51 [PATCH v2 0/2] drivers: provide devm_platform_request_irq() Dejin Zheng
@ 2020-05-23 14:51 ` Dejin Zheng
  2020-05-26 17:11   ` Grygorii Strashko
  2020-05-23 14:51 ` [PATCH v2 2/2] i2c: busses: convert to devm_platform_request_irq() Dejin Zheng
  2020-05-23 16:08 ` [PATCH v2 0/2] drivers: provide devm_platform_request_irq() Wolfram Sang
  2 siblings, 1 reply; 12+ messages in thread
From: Dejin Zheng @ 2020-05-23 14:51 UTC (permalink / raw)
  To: gregkh, rafael, f.fainelli, rjui, sbranden, michal.simek, baruch,
	paul, khilman, shawnguo, festevam, vz, heiko, linus.walleij,
	baohua, ardb, linux-i2c
  Cc: linux-kernel, Dejin Zheng, Wolfram Sang

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 <michal.simek@xilinx.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
v1 -> v2:
	- The patch content has not changed. just resend it by this discussion:
	  https://patchwork.ozlabs.org/project/linux-i2c/patch/20200520144821.8069-1-zhengdejin5@gmail.com/

 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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 2/2] i2c: busses: convert to devm_platform_request_irq()
  2020-05-23 14:51 [PATCH v2 0/2] drivers: provide devm_platform_request_irq() Dejin Zheng
  2020-05-23 14:51 ` [PATCH v2 1/2] " Dejin Zheng
@ 2020-05-23 14:51 ` Dejin Zheng
  2020-05-25 11:42   ` Linus Walleij
  2020-05-23 16:08 ` [PATCH v2 0/2] drivers: provide devm_platform_request_irq() Wolfram Sang
  2 siblings, 1 reply; 12+ messages in thread
From: Dejin Zheng @ 2020-05-23 14:51 UTC (permalink / raw)
  To: gregkh, rafael, f.fainelli, rjui, sbranden, michal.simek, baruch,
	paul, khilman, shawnguo, festevam, vz, heiko, linus.walleij,
	baohua, ardb, linux-i2c
  Cc: linux-kernel, Dejin Zheng, Wolfram Sang

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/20200520144821.8069-1-zhengdejin5@gmail.com/

Cc: Michal Simek <michal.simek@xilinx.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
v1 -> v2:
	- The patch content has not changed. just resend it by this discussion:
	  https://patchwork.ozlabs.org/project/linux-i2c/patch/20200520144821.8069-1-zhengdejin5@gmail.com/

 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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/2] drivers: provide devm_platform_request_irq()
  2020-05-23 14:51 [PATCH v2 0/2] drivers: provide devm_platform_request_irq() Dejin Zheng
  2020-05-23 14:51 ` [PATCH v2 1/2] " Dejin Zheng
  2020-05-23 14:51 ` [PATCH v2 2/2] i2c: busses: convert to devm_platform_request_irq() Dejin Zheng
@ 2020-05-23 16:08 ` Wolfram Sang
  2020-05-23 17:09   ` Dejin Zheng
  2 siblings, 1 reply; 12+ messages in thread
From: Wolfram Sang @ 2020-05-23 16:08 UTC (permalink / raw)
  To: Dejin Zheng
  Cc: gregkh, rafael, f.fainelli, rjui, sbranden, michal.simek, baruch,
	paul, khilman, shawnguo, festevam, vz, heiko, linus.walleij,
	baohua, ardb, linux-i2c, linux-kernel


[-- Attachment #1: Type: text/plain, Size: 1913 bytes --]

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/20200520144821.8069-1-zhengdejin5@gmail.com/
> 	  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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/2] drivers: provide devm_platform_request_irq()
  2020-05-23 16:08 ` [PATCH v2 0/2] drivers: provide devm_platform_request_irq() Wolfram Sang
@ 2020-05-23 17:09   ` Dejin Zheng
  2020-05-25  7:05     ` Michal Simek
  0 siblings, 1 reply; 12+ messages in thread
From: Dejin Zheng @ 2020-05-23 17:09 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: gregkh, rafael, f.fainelli, rjui, sbranden, michal.simek, baruch,
	paul, khilman, shawnguo, festevam, vz, heiko, linus.walleij,
	baohua, ardb, linux-i2c, linux-kernel

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/20200520144821.8069-1-zhengdejin5@gmail.com/
> > 	  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
> > 



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/2] drivers: provide devm_platform_request_irq()
  2020-05-23 17:09   ` Dejin Zheng
@ 2020-05-25  7:05     ` Michal Simek
  2020-05-26 17:13       ` Grygorii Strashko
  0 siblings, 1 reply; 12+ messages in thread
From: Michal Simek @ 2020-05-25  7:05 UTC (permalink / raw)
  To: Dejin Zheng, Wolfram Sang
  Cc: gregkh, rafael, f.fainelli, rjui, sbranden, michal.simek, baruch,
	paul, khilman, shawnguo, festevam, vz, heiko, linus.walleij,
	baohua, ardb, linux-i2c, linux-kernel

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/20200520144821.8069-1-zhengdejin5@gmail.com/
>>> 	  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





^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/2] i2c: busses: convert to devm_platform_request_irq()
  2020-05-23 14:51 ` [PATCH v2 2/2] i2c: busses: convert to devm_platform_request_irq() Dejin Zheng
@ 2020-05-25 11:42   ` Linus Walleij
  0 siblings, 0 replies; 12+ messages in thread
From: Linus Walleij @ 2020-05-25 11:42 UTC (permalink / raw)
  To: Dejin Zheng
  Cc: Greg KH, Rafael J. Wysocki, Florian Fainelli, Ray Jui,
	Scott Branden, Michal Simek, Baruch Siach, Paul Cercueil,
	Kevin Hilman, Shawn Guo, Fabio Estevam, Vladimir Zapolskiy,
	Heiko Stübner, Barry Song, Ard Biesheuvel, linux-i2c,
	linux-kernel, Wolfram Sang

On Sat, May 23, 2020 at 4:52 PM Dejin Zheng <zhengdejin5@gmail.com> 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/20200520144821.8069-1-zhengdejin5@gmail.com/
>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] drivers: provide devm_platform_request_irq()
  2020-05-23 14:51 ` [PATCH v2 1/2] " Dejin Zheng
@ 2020-05-26 17:11   ` Grygorii Strashko
  2020-05-27 13:31     ` Dejin Zheng
  0 siblings, 1 reply; 12+ messages in thread
From: Grygorii Strashko @ 2020-05-26 17:11 UTC (permalink / raw)
  To: Dejin Zheng, gregkh, rafael, f.fainelli, rjui, sbranden,
	michal.simek, baruch, paul, khilman, shawnguo, festevam, vz,
	heiko, linus.walleij, baohua, ardb, linux-i2c
  Cc: linux-kernel, Wolfram Sang



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 <michal.simek@xilinx.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
> ---
> v1 -> v2:
> 	- The patch content has not changed. just resend it by this discussion:
> 	  https://patchwork.ozlabs.org/project/linux-i2c/patch/20200520144821.8069-1-zhengdejin5@gmail.com/
> 
>   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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/2] drivers: provide devm_platform_request_irq()
  2020-05-25  7:05     ` Michal Simek
@ 2020-05-26 17:13       ` Grygorii Strashko
  2020-05-27 13:47         ` Dejin Zheng
  0 siblings, 1 reply; 12+ messages in thread
From: Grygorii Strashko @ 2020-05-26 17:13 UTC (permalink / raw)
  To: Michal Simek, Dejin Zheng, Wolfram Sang
  Cc: gregkh, rafael, f.fainelli, rjui, sbranden, baruch, paul,
	khilman, shawnguo, festevam, vz, heiko, linus.walleij, baohua,
	ardb, linux-i2c, linux-kernel



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/20200520144821.8069-1-zhengdejin5@gmail.com/
>>>> 	  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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] drivers: provide devm_platform_request_irq()
  2020-05-26 17:11   ` Grygorii Strashko
@ 2020-05-27 13:31     ` Dejin Zheng
  0 siblings, 0 replies; 12+ messages in thread
From: Dejin Zheng @ 2020-05-27 13:31 UTC (permalink / raw)
  To: Grygorii Strashko; +Cc: gregkh, linux-i2c, linux-kernel, Wolfram Sang

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 <michal.simek@xilinx.com>
> > Cc: Wolfram Sang <wsa@the-dreams.de>
> > Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
> > ---
> > v1 -> v2:
> > 	- The patch content has not changed. just resend it by this discussion:
> > 	  https://patchwork.ozlabs.org/project/linux-i2c/patch/20200520144821.8069-1-zhengdejin5@gmail.com/
> > 
> >   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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/2] drivers: provide devm_platform_request_irq()
  2020-05-26 17:13       ` Grygorii Strashko
@ 2020-05-27 13:47         ` Dejin Zheng
  0 siblings, 0 replies; 12+ messages in thread
From: Dejin Zheng @ 2020-05-27 13:47 UTC (permalink / raw)
  To: Grygorii Strashko
  Cc: Michal Simek, Wolfram Sang, gregkh, linus.walleij, linux-i2c,
	linux-kernel

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/20200520144821.8069-1-zhengdejin5@gmail.com/
> > > > > 	  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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] drivers: provide devm_platform_request_irq()
@ 2020-05-24  7:00 Markus Elfring
  0 siblings, 0 replies; 12+ messages in thread
From: Markus Elfring @ 2020-05-24  7:00 UTC (permalink / raw)
  To: Dejin Zheng, linux-i2c, kernel-janitors
  Cc: Ard Biesheuvel, Barry Song, Baruch Siach, Fabio Estevam,
	Florian Fainelli, Greg Kroah-Hartman, Heiko Stübner,
	Kevin Hilman, Linus Walleij, Michal Simek, Paul Cercueil,
	Rafael J. Wysocki, Ray Jui, Scott Branden, Shawn Guo,
	Vladimir Zapolskiy, Wolfram Sang, linux-kernel, Coccinelle

> 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, back to index

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-23 14:51 [PATCH v2 0/2] drivers: provide devm_platform_request_irq() Dejin Zheng
2020-05-23 14:51 ` [PATCH v2 1/2] " Dejin Zheng
2020-05-26 17:11   ` Grygorii Strashko
2020-05-27 13:31     ` Dejin Zheng
2020-05-23 14:51 ` [PATCH v2 2/2] i2c: busses: convert to devm_platform_request_irq() Dejin Zheng
2020-05-25 11:42   ` Linus Walleij
2020-05-23 16:08 ` [PATCH v2 0/2] drivers: provide devm_platform_request_irq() Wolfram Sang
2020-05-23 17:09   ` Dejin Zheng
2020-05-25  7:05     ` Michal Simek
2020-05-26 17:13       ` Grygorii Strashko
2020-05-27 13:47         ` Dejin Zheng
2020-05-24  7:00 [PATCH v2 1/2] " Markus Elfring

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git