[PATCH 0/3] i2c: ibm_iic: use devm and get irq in probe

[PATCH 0/3] i2c: ibm_iic: use devm and get irq in probe

[Rosen Penev]

Simplifies probe slightly and handles potential -EPROBE_DEFER a bit better.

Rosen Penev (3):
  i2c: ibm_iic: ioremap with platform pointer
  i2c: ibm_iic: get the irq early in probe
  i2c: ibm_iic: use devm for main allocation

 drivers/i2c/busses/i2c-ibm_iic.c | 44 ++++++++++++--------------------
 1 file changed, 17 insertions(+), 27 deletions(-)

-- 
2.55.0

[PATCH 1/3] i2c: ibm_iic: ioremap with platform pointer

[Rosen Penev]

devm_platform_ioremap_resource only needs a platform_device pointer and
calling it early allows handling potential -EPROBE_DEFER.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/i2c/busses/i2c-ibm_iic.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c
index 7c70e8bda24e..043cb64a2544 100644
--- a/drivers/i2c/busses/i2c-ibm_iic.c
+++ b/drivers/i2c/busses/i2c-ibm_iic.c
@@ -684,21 +684,21 @@ static int iic_probe(struct platform_device *ofdev)
 	struct device_node *np = ofdev->dev.of_node;
 	struct ibm_iic_private *dev;
 	struct i2c_adapter *adap;
+	void __iomem *vaddr;
 	const u32 *freq;
 	int ret;
 
+	vaddr = devm_platform_ioremap_resource(ofdev, 0);
+	if (IS_ERR(vaddr))
+		return PTR_ERR(vaddr);
+
 	dev = kzalloc_obj(*dev);
 	if (!dev)
 		return -ENOMEM;
 
 	platform_set_drvdata(ofdev, dev);
 
-	dev->vaddr = of_iomap(np, 0);
-	if (dev->vaddr == NULL) {
-		dev_err(&ofdev->dev, "failed to iomap device\n");
-		ret = -ENXIO;
-		goto error_cleanup;
-	}
+	dev->vaddr = vaddr;
 
 	init_waitqueue_head(&dev->wq);
 
@@ -751,9 +751,6 @@ static int iic_probe(struct platform_device *ofdev)
 		free_irq(dev->irq, dev);
 	}
 
-	if (dev->vaddr)
-		iounmap(dev->vaddr);
-
 	kfree(dev);
 	return ret;
 }
@@ -772,7 +769,6 @@ static void iic_remove(struct platform_device *ofdev)
 		free_irq(dev->irq, dev);
 	}
 
-	iounmap(dev->vaddr);
 	kfree(dev);
 }
 
-- 
2.55.0

[PATCH 2/3] i2c: ibm_iic: get the irq early in probe

[Rosen Penev]

platform_get_irq() can return -EPROBE_DEFER, unlike
irq_of_map_and_parse(), which helps reduce work done during probe.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/i2c/busses/i2c-ibm_iic.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c
index 043cb64a2544..2730c8410167 100644
--- a/drivers/i2c/busses/i2c-ibm_iic.c
+++ b/drivers/i2c/busses/i2c-ibm_iic.c
@@ -649,20 +649,11 @@ static inline u8 iic_clckdiv(unsigned int opb)
 }
 
 static int iic_request_irq(struct platform_device *ofdev,
-				     struct ibm_iic_private *dev)
+			     struct ibm_iic_private *dev, int irq)
 {
-	struct device_node *np = ofdev->dev.of_node;
-	int irq;
-
 	if (iic_force_poll)
 		return 0;
 
-	irq = irq_of_parse_and_map(np, 0);
-	if (!irq) {
-		dev_err(&ofdev->dev, "irq_of_parse_and_map failed\n");
-		return 0;
-	}
-
 	/* Disable interrupts until we finish initialization, assumes
 	 *  level-sensitive IRQ setup...
 	 */
@@ -686,12 +677,19 @@ static int iic_probe(struct platform_device *ofdev)
 	struct i2c_adapter *adap;
 	void __iomem *vaddr;
 	const u32 *freq;
+	int irq;
 	int ret;
 
 	vaddr = devm_platform_ioremap_resource(ofdev, 0);
 	if (IS_ERR(vaddr))
 		return PTR_ERR(vaddr);
 
+	irq = platform_get_irq(ofdev, 0);
+	if (irq == -EPROBE_DEFER)
+		return irq;
+	if (irq < 0)
+		dev_warn(&ofdev->dev, "using polling mode\n");
+
 	dev = kzalloc_obj(*dev);
 	if (!dev)
 		return -ENOMEM;
@@ -702,9 +700,8 @@ static int iic_probe(struct platform_device *ofdev)
 
 	init_waitqueue_head(&dev->wq);
 
-	dev->irq = iic_request_irq(ofdev, dev);
-	if (!dev->irq)
-		dev_warn(&ofdev->dev, "using polling mode\n");
+	if (irq > 0)
+		dev->irq = iic_request_irq(ofdev, dev, irq);
 
 	/* Board specific settings */
 	if (iic_force_fast || of_get_property(np, "fast-mode", NULL))
-- 
2.55.0

[PATCH 3/3] i2c: ibm_iic: use devm for main allocation

[Rosen Penev]

Allows getting rid of kfree in failure and remove paths.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/i2c/busses/i2c-ibm_iic.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c
index 2730c8410167..8fc5f314742f 100644
--- a/drivers/i2c/busses/i2c-ibm_iic.c
+++ b/drivers/i2c/busses/i2c-ibm_iic.c
@@ -690,7 +690,7 @@ static int iic_probe(struct platform_device *ofdev)
 	if (irq < 0)
 		dev_warn(&ofdev->dev, "using polling mode\n");
 
-	dev = kzalloc_obj(*dev);
+	dev = devm_kzalloc(&ofdev->dev, sizeof(*dev), GFP_KERNEL);
 	if (!dev)
 		return -ENOMEM;
 
@@ -748,7 +748,6 @@ static int iic_probe(struct platform_device *ofdev)
 		free_irq(dev->irq, dev);
 	}
 
-	kfree(dev);
 	return ret;
 }
 
@@ -765,8 +764,6 @@ static void iic_remove(struct platform_device *ofdev)
 		iic_interrupt_mode(dev, 0);
 		free_irq(dev->irq, dev);
 	}
-
-	kfree(dev);
 }
 
 static const struct of_device_id ibm_iic_match[] = {
-- 
2.55.0

Re: [PATCH 1/3] i2c: ibm_iic: ioremap with platform pointer

[Markus Elfring]

> devm_platform_ioremap_resource only needs a platform_device pointer and
> calling it early allows handling potential -EPROBE_DEFER.

I find the change description improvable.

See also once more:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2#n94

Regards,
Markus

Re: [PATCH 3/3] i2c: ibm_iic: use devm for main allocation

[Markus Elfring]

> Allows getting rid of kfree in failure and remove paths.

Will another improved change description become more desirable?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2#n94

Regards,
Markus