[PATCH 1/7] i2c-designware: move to managed functions (devm_*)

[PATCH 1/7] i2c-designware: move to managed functions (devm_*)

[Mika Westerberg]

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

This makes the error handling much more simpler than open-coding everything
and in addition makes the probe function smaller and tidier.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c |   73 ++++++++-------------------
 1 file changed, 20 insertions(+), 53 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 0ceb6e1..c19c4ce 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -92,7 +92,7 @@ static int dw_i2c_probe(struct platform_device *pdev)
 {
 	struct dw_i2c_dev *dev;
 	struct i2c_adapter *adap;
-	struct resource *mem, *ioarea;
+	struct resource *mem;
 	int irq, r;
 
 	/* NOTE: driver uses the static register mapping */
@@ -108,32 +108,27 @@ static int dw_i2c_probe(struct platform_device *pdev)
 		return irq; /* -ENXIO */
 	}
 
-	ioarea = request_mem_region(mem->start, resource_size(mem),
-			pdev->name);
-	if (!ioarea) {
-		dev_err(&pdev->dev, "I2C region already claimed\n");
-		return -EBUSY;
-	}
+	dev = devm_kzalloc(&pdev->dev, sizeof(struct dw_i2c_dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
 
-	dev = kzalloc(sizeof(struct dw_i2c_dev), GFP_KERNEL);
-	if (!dev) {
-		r = -ENOMEM;
-		goto err_release_region;
+	dev->base = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(dev->base)) {
+		dev_err(&pdev->dev, "I2C region already claimed\n");
+		return PTR_ERR(dev->base);
 	}
 
 	init_completion(&dev->cmd_complete);
 	mutex_init(&dev->lock);
-	dev->dev = get_device(&pdev->dev);
+	dev->dev = &pdev->dev;
 	dev->irq = irq;
 	platform_set_drvdata(pdev, dev);
 
-	dev->clk = clk_get(&pdev->dev, NULL);
+	dev->clk = devm_clk_get(&pdev->dev, NULL);
 	dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz;
 
-	if (IS_ERR(dev->clk)) {
-		r = -ENODEV;
-		goto err_free_mem;
-	}
+	if (IS_ERR(dev->clk))
+		return PTR_ERR(dev->clk);
 	clk_prepare_enable(dev->clk);
 
 	dev->functionality =
@@ -146,13 +141,6 @@ static int dw_i2c_probe(struct platform_device *pdev)
 	dev->master_cfg =  DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
 		DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
 
-	dev->base = ioremap(mem->start, resource_size(mem));
-	if (dev->base == NULL) {
-		dev_err(&pdev->dev, "failure mapping io resources\n");
-		r = -EBUSY;
-		goto err_unuse_clocks;
-	}
-
 	/* Try first if we can configure the device from ACPI */
 	r = dw_i2c_acpi_configure(pdev);
 	if (r) {
@@ -164,13 +152,14 @@ static int dw_i2c_probe(struct platform_device *pdev)
 	}
 	r = i2c_dw_init(dev);
 	if (r)
-		goto err_iounmap;
+		return r;
 
 	i2c_dw_disable_int(dev);
-	r = request_irq(dev->irq, i2c_dw_isr, IRQF_SHARED, pdev->name, dev);
+	r = devm_request_irq(&pdev->dev, dev->irq, i2c_dw_isr, IRQF_SHARED,
+			pdev->name, dev);
 	if (r) {
 		dev_err(&pdev->dev, "failure requesting irq %i\n", dev->irq);
-		goto err_iounmap;
+		return r;
 	}
 
 	adap = &dev->adapter;
@@ -187,57 +176,35 @@ static int dw_i2c_probe(struct platform_device *pdev)
 	r = i2c_add_numbered_adapter(adap);
 	if (r) {
 		dev_err(&pdev->dev, "failure adding adapter\n");
-		goto err_free_irq;
+		return r;
 	}
 	of_i2c_register_devices(adap);
 	acpi_i2c_register_devices(adap);
 
+	/* Increase reference counter */
+	get_device(&pdev->dev);
+
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_put(&pdev->dev);
 
 	return 0;
-
-err_free_irq:
-	free_irq(dev->irq, dev);
-err_iounmap:
-	iounmap(dev->base);
-err_unuse_clocks:
-	clk_disable_unprepare(dev->clk);
-	clk_put(dev->clk);
-	dev->clk = NULL;
-err_free_mem:
-	put_device(&pdev->dev);
-	kfree(dev);
-err_release_region:
-	release_mem_region(mem->start, resource_size(mem));
-
-	return r;
 }
 
 static int dw_i2c_remove(struct platform_device *pdev)
 {
 	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
-	struct resource *mem;
 
 	pm_runtime_get_sync(&pdev->dev);
 
 	i2c_del_adapter(&dev->adapter);
 	put_device(&pdev->dev);
 
-	clk_disable_unprepare(dev->clk);
-	clk_put(dev->clk);
-	dev->clk = NULL;
-
 	i2c_dw_disable(dev);
-	free_irq(dev->irq, dev);
-	kfree(dev);
 
 	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
-	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	release_mem_region(mem->start, resource_size(mem));
 	return 0;
 }
 
-- 
1.7.10.4

[PATCH 2/7] i2c-designware-pci: use dev_err() instead of printk()

[Mika Westerberg]

From: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

With dev_err() we can get the device instance printed as well and is pretty
much standard to use dev_* macros in drivers anyway. In addition correct
indentation of probe() arguments.

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/i2c/busses/i2c-designware-pcidrv.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 7c5e383..eed149d 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -208,7 +208,7 @@ static u32 i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
 }
 
 static int i2c_dw_pci_probe(struct pci_dev *pdev,
-const struct pci_device_id *id)
+			    const struct pci_device_id *id)
 {
 	struct dw_i2c_dev *dev;
 	struct i2c_adapter *adap;
@@ -218,7 +218,7 @@ const struct pci_device_id *id)
 	struct  dw_pci_controller *controller;
 
 	if (id->driver_data >= ARRAY_SIZE(dw_pci_controllers)) {
-		printk(KERN_ERR "dw_i2c_pci_probe: invalid driver data %ld\n",
+		dev_err(&pdev->dev, "%s: invalid driver data %ld\n", __func__,
 			id->driver_data);
 		return -EINVAL;
 	}
-- 
1.7.10.4

[PATCH 3/7] i2c-designware-pci: use managed functions pcim_* and devm_*

[Mika Westerberg]

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

This makes the error handling much more simpler than open-coding everything
and in addition makes the probe function smaller an tidier.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-pcidrv.c |   68 +++++++---------------------
 1 file changed, 17 insertions(+), 51 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index eed149d..aacb64e 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -212,8 +212,6 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 {
 	struct dw_i2c_dev *dev;
 	struct i2c_adapter *adap;
-	unsigned long start, len;
-	void __iomem *base;
 	int r;
 	struct  dw_pci_controller *controller;
 
@@ -225,51 +223,30 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 
 	controller = &dw_pci_controllers[id->driver_data];
 
-	r = pci_enable_device(pdev);
+	r = pcim_enable_device(pdev);
 	if (r) {
 		dev_err(&pdev->dev, "Failed to enable I2C PCI device (%d)\n",
 			r);
-		goto exit;
+		return r;
 	}
 
-	/* Determine the address of the I2C area */
-	start = pci_resource_start(pdev, 0);
-	len = pci_resource_len(pdev, 0);
-	if (!start || len == 0) {
-		dev_err(&pdev->dev, "base address not set\n");
-		r = -ENODEV;
-		goto exit;
-	}
-
-	r = pci_request_region(pdev, 0, DRIVER_NAME);
+	r = pcim_iomap_regions(pdev, 1 << 0, pci_name(pdev));
 	if (r) {
-		dev_err(&pdev->dev, "failed to request I2C region "
-			"0x%lx-0x%lx\n", start,
-			(unsigned long)pci_resource_end(pdev, 0));
-		goto exit;
-	}
-
-	base = ioremap_nocache(start, len);
-	if (!base) {
 		dev_err(&pdev->dev, "I/O memory remapping failed\n");
-		r = -ENOMEM;
-		goto err_release_region;
+		return r;
 	}
 
-
-	dev = kzalloc(sizeof(struct dw_i2c_dev), GFP_KERNEL);
-	if (!dev) {
-		r = -ENOMEM;
-		goto err_release_region;
-	}
+	dev = devm_kzalloc(&pdev->dev, sizeof(struct dw_i2c_dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
 
 	init_completion(&dev->cmd_complete);
 	mutex_init(&dev->lock);
 	dev->clk = NULL;
 	dev->controller = controller;
 	dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz;
-	dev->base = base;
-	dev->dev = get_device(&pdev->dev);
+	dev->base = pcim_iomap_table(pdev)[0];
+	dev->dev = &pdev->dev;
 	dev->functionality =
 		I2C_FUNC_I2C |
 		I2C_FUNC_SMBUS_BYTE |
@@ -284,7 +261,7 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 	dev->rx_fifo_depth = controller->rx_fifo_depth;
 	r = i2c_dw_init(dev);
 	if (r)
-		goto err_iounmap;
+		return r;
 
 	adap = &dev->adapter;
 	i2c_set_adapdata(adap, dev);
@@ -296,10 +273,11 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 	snprintf(adap->name, sizeof(adap->name), "i2c-designware-pci-%d",
 		adap->nr);
 
-	r = request_irq(pdev->irq, i2c_dw_isr, IRQF_SHARED, adap->name, dev);
+	r = devm_request_irq(&pdev->dev, pdev->irq, i2c_dw_isr, IRQF_SHARED,
+			adap->name, dev);
 	if (r) {
 		dev_err(&pdev->dev, "failure requesting irq %i\n", dev->irq);
-		goto err_iounmap;
+		return r;
 	}
 
 	i2c_dw_disable_int(dev);
@@ -307,24 +285,16 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 	r = i2c_add_numbered_adapter(adap);
 	if (r) {
 		dev_err(&pdev->dev, "failure adding adapter\n");
-		goto err_free_irq;
+		return r;
 	}
 
+	/* Increase reference counter */
+	get_device(&pdev->dev);
+
 	pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_allow(&pdev->dev);
 
 	return 0;
-
-err_free_irq:
-	free_irq(pdev->irq, dev);
-err_iounmap:
-	iounmap(dev->base);
-	put_device(&pdev->dev);
-	kfree(dev);
-err_release_region:
-	pci_release_region(pdev, 0);
-exit:
-	return r;
 }
 
 static void i2c_dw_pci_remove(struct pci_dev *pdev)
@@ -337,10 +307,6 @@ static void i2c_dw_pci_remove(struct pci_dev *pdev)
 
 	i2c_del_adapter(&dev->adapter);
 	put_device(&pdev->dev);
-
-	free_irq(dev->irq, dev);
-	kfree(dev);
-	pci_release_region(pdev, 0);
 }
 
 /* work with hotplug and coldplug */
-- 
1.7.10.4

[PATCH 4/7] i2c-designware: use dynamic adapter numbering on Lynxpoint

[Mika Westerberg]

It is not good idea to mix static and dynamic I2C adapter numbering. In
this particular case on Lynxpoint we had graphics I2C adapter which took
the first numbers preventing the designware I2C driver from using the
adapter numbers it preferred.

Fix this by switching to use dynamic adapter numbering on Intel Lynxpoint.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c |    9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index c19c4ce..a22a852 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -56,20 +56,11 @@ static u32 i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
 static int dw_i2c_acpi_configure(struct platform_device *pdev)
 {
 	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
-	struct acpi_device *adev;
-	int busno, ret;
 
 	if (!ACPI_HANDLE(&pdev->dev))
 		return -ENODEV;
 
-	ret = acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev);
-	if (ret)
-		return -ENODEV;
-
 	dev->adapter.nr = -1;
-	if (adev->pnp.unique_id && !kstrtoint(adev->pnp.unique_id, 0, &busno))
-		dev->adapter.nr = busno;
-
 	dev->tx_fifo_depth = 32;
 	dev->rx_fifo_depth = 32;
 	return 0;
-- 
1.7.10.4

[PATCH 5/7] i2c-designware: enable/disable the controller properly

[Mika Westerberg]

The correct way to disable or enable the controller is to wait until
DW_IC_ENABLE_STATUS register bit matches the bit we program to the
DW_IC_ENABLE register. This procedure is described in the DesignWare I2C
handbook.

By doing this we can be sure that the controller is in correct state once
the function returns.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-core.c |   29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 94fd818..0e4f531 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -68,6 +68,7 @@
 #define DW_IC_TXFLR		0x74
 #define DW_IC_RXFLR		0x78
 #define DW_IC_TX_ABRT_SOURCE	0x80
+#define DW_IC_ENABLE_STATUS	0x9c
 #define DW_IC_COMP_PARAM_1	0xf4
 #define DW_IC_COMP_TYPE		0xfc
 #define DW_IC_COMP_TYPE_VALUE	0x44570140
@@ -248,6 +249,22 @@ static u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset)
 	return ((ic_clk * (tLOW + tf) + 5000) / 10000) - 1 + offset;
 }
 
+static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
+{
+	int timeout = 100;
+
+	do {
+		dw_writel(dev, enable, DW_IC_ENABLE);
+		if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
+			return;
+
+		usleep_range(25, 250);
+	} while (timeout-- > 0);
+
+	dev_warn(dev->dev, "timeout in %sabling adapter\n",
+		 enable ? "en" : "dis");
+}
+
 /**
  * i2c_dw_init() - initialize the designware i2c master hardware
  * @dev: device private data
@@ -278,7 +295,7 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	}
 
 	/* Disable the adapter */
-	dw_writel(dev, 0, DW_IC_ENABLE);
+	__i2c_dw_enable(dev, false);
 
 	/* set standard and fast speed deviders for high/low periods */
 
@@ -345,7 +362,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 	u32 ic_con;
 
 	/* Disable the adapter */
-	dw_writel(dev, 0, DW_IC_ENABLE);
+	__i2c_dw_enable(dev, false);
 
 	/* set the slave (target) address */
 	dw_writel(dev, msgs[dev->msg_write_idx].addr, DW_IC_TAR);
@@ -359,7 +376,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 	dw_writel(dev, ic_con, DW_IC_CON);
 
 	/* Enable the adapter */
-	dw_writel(dev, 1, DW_IC_ENABLE);
+	__i2c_dw_enable(dev, true);
 
 	/* Enable interrupts */
 	dw_writel(dev, DW_IC_INTR_DEFAULT_MASK, DW_IC_INTR_MASK);
@@ -565,7 +582,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	/* no error */
 	if (likely(!dev->cmd_err)) {
 		/* Disable the adapter */
-		dw_writel(dev, 0, DW_IC_ENABLE);
+		__i2c_dw_enable(dev, false);
 		ret = num;
 		goto done;
 	}
@@ -700,7 +717,7 @@ EXPORT_SYMBOL_GPL(i2c_dw_isr);
 void i2c_dw_enable(struct dw_i2c_dev *dev)
 {
        /* Enable the adapter */
-	dw_writel(dev, 1, DW_IC_ENABLE);
+	__i2c_dw_enable(dev, true);
 }
 EXPORT_SYMBOL_GPL(i2c_dw_enable);
 
@@ -713,7 +730,7 @@ EXPORT_SYMBOL_GPL(i2c_dw_is_enabled);
 void i2c_dw_disable(struct dw_i2c_dev *dev)
 {
 	/* Disable controller */
-	dw_writel(dev, 0, DW_IC_ENABLE);
+	__i2c_dw_enable(dev, false);
 
 	/* Disable all interupts */
 	dw_writel(dev, 0, DW_IC_INTR_MASK);
-- 
1.7.10.4

[PATCH 6/7] i2c-designware: use usleep_range() in the busy-loop

[Mika Westerberg]

This is not an atomic context so there is no need to use mdelay() but
instead use usleep_range().

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 0e4f531..0849648 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -350,7 +350,7 @@ static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
 			return -ETIMEDOUT;
 		}
 		timeout--;
-		mdelay(1);
+		usleep_range(1000, 1100);
 	}
 
 	return 0;
-- 
1.7.10.4

[PATCH 7/7] i2c-designware: switch to use runtime PM autosuspend

[Mika Westerberg]

Using autosuspend helps to reduce the resume latency in situations where
another I2C message is going to be started soon. For example with HID over
I2C touch panels we get several messages in a short period of time while
the touch panel is in use.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-core.c    |    3 ++-
 drivers/i2c/busses/i2c-designware-pcidrv.c  |    3 ++-
 drivers/i2c/busses/i2c-designware-platdrv.c |    3 ++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 0849648..71d33e6 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -595,7 +595,8 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	ret = -EIO;
 
 done:
-	pm_runtime_put(dev->dev);
+	pm_runtime_mark_last_busy(dev->dev);
+	pm_runtime_put_autosuspend(dev->dev);
 	mutex_unlock(&dev->lock);
 
 	return ret;
diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index aacb64e..c8797e2 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -291,7 +291,8 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 	/* Increase reference counter */
 	get_device(&pdev->dev);
 
-	pm_runtime_put_noidle(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_allow(&pdev->dev);
 
 	return 0;
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index a22a852..46a25ca 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -175,9 +175,10 @@ static int dw_i2c_probe(struct platform_device *pdev)
 	/* Increase reference counter */
 	get_device(&pdev->dev);
 
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
-	pm_runtime_put(&pdev->dev);
 
 	return 0;
 }
-- 
1.7.10.4

Re: [PATCH 1/7] i2c-designware: move to managed functions (devm_*)

[Mika Westerberg]

Hi Wolfram,

Any comments on this series? Could you consider merging these for 3.10?

Thanks.

On Thu, Mar 21, 2013 at 02:09:54PM +0200, Mika Westerberg wrote:
> From: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> 
> This makes the error handling much more simpler than open-coding everything
> and in addition makes the probe function smaller and tidier.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c |   73 ++++++++-------------------
>  1 file changed, 20 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 0ceb6e1..c19c4ce 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -92,7 +92,7 @@ static int dw_i2c_probe(struct platform_device *pdev)
>  {
>  	struct dw_i2c_dev *dev;
>  	struct i2c_adapter *adap;
> -	struct resource *mem, *ioarea;
> +	struct resource *mem;
>  	int irq, r;
>  
>  	/* NOTE: driver uses the static register mapping */
> @@ -108,32 +108,27 @@ static int dw_i2c_probe(struct platform_device *pdev)
>  		return irq; /* -ENXIO */
>  	}
>  
> -	ioarea = request_mem_region(mem->start, resource_size(mem),
> -			pdev->name);
> -	if (!ioarea) {
> -		dev_err(&pdev->dev, "I2C region already claimed\n");
> -		return -EBUSY;
> -	}
> +	dev = devm_kzalloc(&pdev->dev, sizeof(struct dw_i2c_dev), GFP_KERNEL);
> +	if (!dev)
> +		return -ENOMEM;
>  
> -	dev = kzalloc(sizeof(struct dw_i2c_dev), GFP_KERNEL);
> -	if (!dev) {
> -		r = -ENOMEM;
> -		goto err_release_region;
> +	dev->base = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(dev->base)) {
> +		dev_err(&pdev->dev, "I2C region already claimed\n");
> +		return PTR_ERR(dev->base);
>  	}
>  
>  	init_completion(&dev->cmd_complete);
>  	mutex_init(&dev->lock);
> -	dev->dev = get_device(&pdev->dev);
> +	dev->dev = &pdev->dev;
>  	dev->irq = irq;
>  	platform_set_drvdata(pdev, dev);
>  
> -	dev->clk = clk_get(&pdev->dev, NULL);
> +	dev->clk = devm_clk_get(&pdev->dev, NULL);
>  	dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz;
>  
> -	if (IS_ERR(dev->clk)) {
> -		r = -ENODEV;
> -		goto err_free_mem;
> -	}
> +	if (IS_ERR(dev->clk))
> +		return PTR_ERR(dev->clk);
>  	clk_prepare_enable(dev->clk);
>  
>  	dev->functionality =
> @@ -146,13 +141,6 @@ static int dw_i2c_probe(struct platform_device *pdev)
>  	dev->master_cfg =  DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
>  		DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
>  
> -	dev->base = ioremap(mem->start, resource_size(mem));
> -	if (dev->base == NULL) {
> -		dev_err(&pdev->dev, "failure mapping io resources\n");
> -		r = -EBUSY;
> -		goto err_unuse_clocks;
> -	}
> -
>  	/* Try first if we can configure the device from ACPI */
>  	r = dw_i2c_acpi_configure(pdev);
>  	if (r) {
> @@ -164,13 +152,14 @@ static int dw_i2c_probe(struct platform_device *pdev)
>  	}
>  	r = i2c_dw_init(dev);
>  	if (r)
> -		goto err_iounmap;
> +		return r;
>  
>  	i2c_dw_disable_int(dev);
> -	r = request_irq(dev->irq, i2c_dw_isr, IRQF_SHARED, pdev->name, dev);
> +	r = devm_request_irq(&pdev->dev, dev->irq, i2c_dw_isr, IRQF_SHARED,
> +			pdev->name, dev);
>  	if (r) {
>  		dev_err(&pdev->dev, "failure requesting irq %i\n", dev->irq);
> -		goto err_iounmap;
> +		return r;
>  	}
>  
>  	adap = &dev->adapter;
> @@ -187,57 +176,35 @@ static int dw_i2c_probe(struct platform_device *pdev)
>  	r = i2c_add_numbered_adapter(adap);
>  	if (r) {
>  		dev_err(&pdev->dev, "failure adding adapter\n");
> -		goto err_free_irq;
> +		return r;
>  	}
>  	of_i2c_register_devices(adap);
>  	acpi_i2c_register_devices(adap);
>  
> +	/* Increase reference counter */
> +	get_device(&pdev->dev);
> +
>  	pm_runtime_set_active(&pdev->dev);
>  	pm_runtime_enable(&pdev->dev);
>  	pm_runtime_put(&pdev->dev);
>  
>  	return 0;
> -
> -err_free_irq:
> -	free_irq(dev->irq, dev);
> -err_iounmap:
> -	iounmap(dev->base);
> -err_unuse_clocks:
> -	clk_disable_unprepare(dev->clk);
> -	clk_put(dev->clk);
> -	dev->clk = NULL;
> -err_free_mem:
> -	put_device(&pdev->dev);
> -	kfree(dev);
> -err_release_region:
> -	release_mem_region(mem->start, resource_size(mem));
> -
> -	return r;
>  }
>  
>  static int dw_i2c_remove(struct platform_device *pdev)
>  {
>  	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
> -	struct resource *mem;
>  
>  	pm_runtime_get_sync(&pdev->dev);
>  
>  	i2c_del_adapter(&dev->adapter);
>  	put_device(&pdev->dev);
>  
> -	clk_disable_unprepare(dev->clk);
> -	clk_put(dev->clk);
> -	dev->clk = NULL;
> -
>  	i2c_dw_disable(dev);
> -	free_irq(dev->irq, dev);
> -	kfree(dev);
>  
>  	pm_runtime_put(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  
> -	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	release_mem_region(mem->start, resource_size(mem));
>  	return 0;
>  }
>  
> -- 
> 1.7.10.4

Re: [1/7] i2c-designware: move to managed functions (devm_*)

[Wolfram Sang]

> @@ -108,32 +108,27 @@ static int dw_i2c_probe(struct platform_device *pdev)
>  		return irq; /* -ENXIO */
>  	}
>  
> -	ioarea = request_mem_region(mem->start, resource_size(mem),
> -			pdev->name);
> -	if (!ioarea) {
> -		dev_err(&pdev->dev, "I2C region already claimed\n");
> -		return -EBUSY;
> -	}
> +	dev = devm_kzalloc(&pdev->dev, sizeof(struct dw_i2c_dev), GFP_KERNEL);
> +	if (!dev)
> +		return -ENOMEM;
>  
> -	dev = kzalloc(sizeof(struct dw_i2c_dev), GFP_KERNEL);
> -	if (!dev) {
> -		r = -ENOMEM;
> -		goto err_release_region;
> +	dev->base = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(dev->base)) {
> +		dev_err(&pdev->dev, "I2C region already claimed\n");

No dev_err here. The devm function will print out errors already.

> @@ -164,13 +152,14 @@ static int dw_i2c_probe(struct platform_device *pdev)
>  	}
>  	r = i2c_dw_init(dev);
>  	if (r)
> -		goto err_iounmap;
> +		return r;
>  
>  	i2c_dw_disable_int(dev);
> -	r = request_irq(dev->irq, i2c_dw_isr, IRQF_SHARED, pdev->name, dev);
> +	r = devm_request_irq(&pdev->dev, dev->irq, i2c_dw_isr, IRQF_SHARED,
> +			pdev->name, dev);

Is it ensured that no interrupts will happen during remove? Because the
adapter will be deleted before devm will free the interrupt.

Thanks,

   Wolfram

Re: [3/7] i2c-designware-pci: use managed functions pcim_* and devm_*

[Wolfram Sang]

> @@ -296,10 +273,11 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
>  	snprintf(adap->name, sizeof(adap->name), "i2c-designware-pci-%d",
>  		adap->nr);
>  
> -	r = request_irq(pdev->irq, i2c_dw_isr, IRQF_SHARED, adap->name, dev);
> +	r = devm_request_irq(&pdev->dev, pdev->irq, i2c_dw_isr, IRQF_SHARED,
> +			adap->name, dev);

IRQ question from patch 1 fits here, too.

Re: [4/7] i2c-designware: use dynamic adapter numbering on Lynxpoint

[Wolfram Sang]

On Thu, Mar 21, 2013 at 02:09:57AM -0000, Mika Westerberg wrote:
> It is not good idea to mix static and dynamic I2C adapter numbering. In
> this particular case on Lynxpoint we had graphics I2C adapter which took
> the first numbers preventing the designware I2C driver from using the
> adapter numbers it preferred.
> 
> Fix this by switching to use dynamic adapter numbering on Intel Lynxpoint.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

I am fearing regressions here if the bus numbering changes.

Re: [5/7] i2c-designware: enable/disable the controller properly

[Wolfram Sang]

> +static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
> +{
> +	int timeout = 100;
> +
> +	do {
> +		dw_writel(dev, enable, DW_IC_ENABLE);
> +		if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
> +			return;
> +
> +		usleep_range(25, 250);

This would wait 25ms max. Is there a timeout value specified in the docs?

> +	} while (timeout-- > 0);

while (timeout--)?

Re: [1/7] i2c-designware: move to managed functions (devm_*)

[Wolfram Sang]

> > >  	i2c_dw_disable_int(dev);
> > > -	r = request_irq(dev->irq, i2c_dw_isr, IRQF_SHARED, pdev->name, dev);
> > > +	r = devm_request_irq(&pdev->dev, dev->irq, i2c_dw_isr, IRQF_SHARED,
> > > +			pdev->name, dev);
> > 
> > Is it ensured that no interrupts will happen during remove? Because the
> > adapter will be deleted before devm will free the interrupt.
> 
> Both platform and PCI driver disable the controller in their remove
> function, and interrupts will be disabled as well. Is this enough or should
> we handle this differently?

That's fine. Thanks!

Re: [1/7] i2c-designware: move to managed functions (devm_*)

[Mika Westerberg]

On Tue, Apr 09, 2013 at 11:00:32AM +0200, Wolfram Sang wrote:
> 
> > @@ -108,32 +108,27 @@ static int dw_i2c_probe(struct platform_device *pdev)
> >  		return irq; /* -ENXIO */
> >  	}
> >  
> > -	ioarea = request_mem_region(mem->start, resource_size(mem),
> > -			pdev->name);
> > -	if (!ioarea) {
> > -		dev_err(&pdev->dev, "I2C region already claimed\n");
> > -		return -EBUSY;
> > -	}
> > +	dev = devm_kzalloc(&pdev->dev, sizeof(struct dw_i2c_dev), GFP_KERNEL);
> > +	if (!dev)
> > +		return -ENOMEM;
> >  
> > -	dev = kzalloc(sizeof(struct dw_i2c_dev), GFP_KERNEL);
> > -	if (!dev) {
> > -		r = -ENOMEM;
> > -		goto err_release_region;
> > +	dev->base = devm_ioremap_resource(&pdev->dev, mem);
> > +	if (IS_ERR(dev->base)) {
> > +		dev_err(&pdev->dev, "I2C region already claimed\n");
> 
> No dev_err here. The devm function will print out errors already.

OK

> > @@ -164,13 +152,14 @@ static int dw_i2c_probe(struct platform_device *pdev)
> >  	}
> >  	r = i2c_dw_init(dev);
> >  	if (r)
> > -		goto err_iounmap;
> > +		return r;
> >  
> >  	i2c_dw_disable_int(dev);
> > -	r = request_irq(dev->irq, i2c_dw_isr, IRQF_SHARED, pdev->name, dev);
> > +	r = devm_request_irq(&pdev->dev, dev->irq, i2c_dw_isr, IRQF_SHARED,
> > +			pdev->name, dev);
> 
> Is it ensured that no interrupts will happen during remove? Because the
> adapter will be deleted before devm will free the interrupt.

Both platform and PCI driver disable the controller in their remove
function, and interrupts will be disabled as well. Is this enough or should
we handle this differently?

Re: [4/7] i2c-designware: use dynamic adapter numbering on Lynxpoint

[Mika Westerberg]

On Tue, Apr 09, 2013 at 11:06:05AM +0200, Wolfram Sang wrote:
> On Thu, Mar 21, 2013 at 02:09:57AM -0000, Mika Westerberg wrote:
> > It is not good idea to mix static and dynamic I2C adapter numbering. In
> > this particular case on Lynxpoint we had graphics I2C adapter which took
> > the first numbers preventing the designware I2C driver from using the
> > adapter numbers it preferred.
> > 
> > Fix this by switching to use dynamic adapter numbering on Intel Lynxpoint.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> I am fearing regressions here if the bus numbering changes.

There are no users for this dynamic numbering yet since it was introduced
with the Lynxpoint support.

Re: [5/7] i2c-designware: enable/disable the controller properly

[Mika Westerberg]

On Tue, Apr 09, 2013 at 11:09:14AM +0200, Wolfram Sang wrote:
> 
> > +static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
> > +{
> > +	int timeout = 100;
> > +
> > +	do {
> > +		dw_writel(dev, enable, DW_IC_ENABLE);
> > +		if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
> > +			return;
> > +
> > +		usleep_range(25, 250);
> 
> This would wait 25ms max. Is there a timeout value specified in the docs?

The datasheet says something like:

	1. Define a timer interval (t_i2c_poll) equal 10 times the highest
	signaling period. For 400kHz this is 25us.

	2. Define max timeout parameter, MAX_T_POLL_COUNT, such that if any
	repeated operation exeeds this maximum, an error is reported.

In this case I have:

	t_i2c_poll = 25 (to 250 us)
	MAX_T_POLL_COUNT = 100

> > +	} while (timeout-- > 0);
> 
> while (timeout--)?

OK, thanks.

Re: [5/7] i2c-designware: enable/disable the controller properly

[Wolfram Sang]

On Tue, Apr 09, 2013 at 12:28:36PM +0300, Mika Westerberg wrote:
> On Tue, Apr 09, 2013 at 11:09:14AM +0200, Wolfram Sang wrote:
> > 
> > > +static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
> > > +{
> > > +	int timeout = 100;
> > > +
> > > +	do {
> > > +		dw_writel(dev, enable, DW_IC_ENABLE);
> > > +		if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
> > > +			return;
> > > +
> > > +		usleep_range(25, 250);
> > 
> > This would wait 25ms max. Is there a timeout value specified in the docs?
> 
> The datasheet says something like:
> 
> 	1. Define a timer interval (t_i2c_poll) equal 10 times the highest
> 	signaling period. For 400kHz this is 25us.
> 
> 	2. Define max timeout parameter, MAX_T_POLL_COUNT, such that if any
> 	repeated operation exeeds this maximum, an error is reported.
> 
> In this case I have:
> 
> 	t_i2c_poll = 25 (to 250 us)
> 	MAX_T_POLL_COUNT = 100

Maybe worth a comment?

Other than that, the series looks fine to me.

Re: [4/7] i2c-designware: use dynamic adapter numbering on Lynxpoint

[Wolfram Sang]

On Tue, Apr 09, 2013 at 12:23:19PM +0300, Mika Westerberg wrote:
> On Tue, Apr 09, 2013 at 11:06:05AM +0200, Wolfram Sang wrote:
> > On Thu, Mar 21, 2013 at 02:09:57AM -0000, Mika Westerberg wrote:
> > > It is not good idea to mix static and dynamic I2C adapter numbering. In
> > > this particular case on Lynxpoint we had graphics I2C adapter which took
> > > the first numbers preventing the designware I2C driver from using the
> > > adapter numbers it preferred.
> > > 
> > > Fix this by switching to use dynamic adapter numbering on Intel Lynxpoint.
> > > 
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > 
> > I am fearing regressions here if the bus numbering changes.
> 
> There are no users for this dynamic numbering yet since it was introduced
> with the Lynxpoint support.

Please add this to the commit msg.

Re: [4/7] i2c-designware: use dynamic adapter numbering on Lynxpoint

[Mika Westerberg]

On Tue, Apr 09, 2013 at 11:29:24AM +0200, Wolfram Sang wrote:
> On Tue, Apr 09, 2013 at 12:23:19PM +0300, Mika Westerberg wrote:
> > On Tue, Apr 09, 2013 at 11:06:05AM +0200, Wolfram Sang wrote:
> > > On Thu, Mar 21, 2013 at 02:09:57AM -0000, Mika Westerberg wrote:
> > > > It is not good idea to mix static and dynamic I2C adapter numbering. In
> > > > this particular case on Lynxpoint we had graphics I2C adapter which took
> > > > the first numbers preventing the designware I2C driver from using the
> > > > adapter numbers it preferred.
> > > > 
> > > > Fix this by switching to use dynamic adapter numbering on Intel Lynxpoint.
> > > > 
> > > > Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > > 
> > > I am fearing regressions here if the bus numbering changes.
> > 
> > There are no users for this dynamic numbering yet since it was introduced
> > with the Lynxpoint support.
> 
> Please add this to the commit msg.

Sure, thanks.

Re: [5/7] i2c-designware: enable/disable the controller properly

[Mika Westerberg]

On Tue, Apr 09, 2013 at 11:28:57AM +0200, Wolfram Sang wrote:
> On Tue, Apr 09, 2013 at 12:28:36PM +0300, Mika Westerberg wrote:
> > On Tue, Apr 09, 2013 at 11:09:14AM +0200, Wolfram Sang wrote:
> > > 
> > > > +static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
> > > > +{
> > > > +	int timeout = 100;
> > > > +
> > > > +	do {
> > > > +		dw_writel(dev, enable, DW_IC_ENABLE);
> > > > +		if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
> > > > +			return;
> > > > +
> > > > +		usleep_range(25, 250);
> > > 
> > > This would wait 25ms max. Is there a timeout value specified in the docs?
> > 
> > The datasheet says something like:
> > 
> > 	1. Define a timer interval (t_i2c_poll) equal 10 times the highest
> > 	signaling period. For 400kHz this is 25us.
> > 
> > 	2. Define max timeout parameter, MAX_T_POLL_COUNT, such that if any
> > 	repeated operation exeeds this maximum, an error is reported.
> > 
> > In this case I have:
> > 
> > 	t_i2c_poll = 25 (to 250 us)
> > 	MAX_T_POLL_COUNT = 100
> 
> Maybe worth a comment?

I'll add it in the next revision.

> Other than that, the series looks fine to me.

Thanks!