i2c-designware: move to managed functions (devm_*)

PATCH Superseded Mika Westerberg 2 participants 20 messages Mar 21, 2013 – Apr 9, 2013

Subsystems (3)

I2C subsystem host drivers Intel LPSS (Low Power SubSystem) drivers Synopsys DesignWare I2C driver

lore

Share & tools

lore

Mika Westerberg

author maintainer

2013-03-21 12:09 UTC #0 lore raw

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

From: Andy Shevchenko <[email protected]>

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 <[email protected]>

Signed-off-by Mika Westerberg <[email protected]>

---
 drivers/i2c/busses/i2c-designware-platdrv.c |   73 ++++++++-------------------
 1 file changed, 20 insertions(+), 53 deletions(-)

drivers/i2c/busses/i2c-designware-platdrv.c +20 ~~-53~~

index 0ceb6e1..c19c4ce 100644
@@ -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;
 }

Signature · 1 line

1.7.10.4

Mika Westerberg

author maintainer

2013-03-21 12:08 UTC #1 lore raw

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

From: Andy Shevchenko <[email protected]>

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 <[email protected]>

Signed-off-by Mika Westerberg <[email protected]>

---
 drivers/i2c/busses/i2c-designware-pcidrv.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/i2c/busses/i2c-designware-pcidrv.c +2 -2

index 7c5e383..eed149d 100644
@@ -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;
 	}

Signature · 1 line

1.7.10.4

Mika Westerberg

author maintainer

2013-03-21 12:08 UTC #2 lore raw

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

In reply to Mika Westerberg

From: Andy Shevchenko <[email protected]>

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 <[email protected]>

Signed-off-by Mika Westerberg <[email protected]>

---
 drivers/i2c/busses/i2c-designware-pcidrv.c |   68 +++++++---------------------
 1 file changed, 17 insertions(+), 51 deletions(-)

drivers/i2c/busses/i2c-designware-pcidrv.c +17 ~~-51~~

index eed149d..aacb64e 100644
@@ -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 */

Signature · 1 line

1.7.10.4

Wolfram Sang

2013-04-09 09:03 UTC #3 lore raw

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

drivers/i2c/busses/i2c-designware-pcidrv.c static int i2c_dw_pci_probe(struct pci_dev *pdev, · line 277

Quoted 7 lines

drivers/i2c/busses/i2c-designware-pcidrv.c +2 -1

@@ -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.

Mika Westerberg

author maintainer

2013-03-21 12:08 UTC #4 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/i2c/busses/i2c-designware-platdrv.c |    9 ---------
 1 file changed, 9 deletions(-)

drivers/i2c/busses/i2c-designware-platdrv.c +0 -9

index c19c4ce..a22a852 100644
@@ -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;

Signature · 1 line

1.7.10.4

Wolfram Sang

2013-04-09 09:06 UTC #5 lore raw

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

Quoted 8 lines · 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-VuQAYsv1563Yd54FQh9/[email protected]>

I am fearing regressions here if the bus numbering changes.

Mika Westerberg

author maintainer

2013-04-09 09:19 UTC #6 lore raw

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

Quoted 11 lines · Wolfram Sang

Quoted 8 lines · 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 <[email protected]>
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.

Wolfram Sang

2013-04-09 09:29 UTC #7 lore raw

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

Quoted 7 lines · Mika Westerberg

Quoted 6 lines · Wolfram Sang
Quoted 5 lines · 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.

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
Fix this by switching to use dynamic adapter numbering on Intel Lynxpoint.
Signed-off-by Mika Westerberg <[email protected]>
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.

Mika Westerberg

author maintainer

2013-04-09 09:33 UTC #8 lore raw

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

Quoted 10 lines · Wolfram Sang

Quoted 9 lines · Mika Westerberg
Quoted 8 lines · Wolfram Sang
Quoted 7 lines · 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.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Mika Westerberg <[email protected]>
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.

Mika Westerberg

author maintainer

2013-03-21 12:08 UTC #9 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/i2c/busses/i2c-designware-core.c |   29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

drivers/i2c/busses/i2c-designware-core.c +23 -6

index 94fd818..0e4f531 100644
@@ -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);

Signature · 1 line

1.7.10.4

Wolfram Sang

2013-04-09 09:09 UTC #10 lore raw

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

drivers/i2c/busses/i2c-designware-core.c static u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset) · line 261

Quoted 11 lines

drivers/i2c/busses/i2c-designware-core.c +10 -0

+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?

drivers/i2c/busses/i2c-designware-core.c static u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset) · line 262

Quoted 1 line

drivers/i2c/busses/i2c-designware-core.c +1 -0
+	} while (timeout-- > 0);

while (timeout--)?

Mika Westerberg

author maintainer

2013-04-09 09:24 UTC #11 lore raw

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

Quoted 13 lines · Wolfram Sang

drivers/i2c/busses/i2c-designware-core.c static u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset) · line 261
Quoted 11 lines
drivers/i2c/busses/i2c-designware-core.c +10 -0
+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

Quoted 3 lines

drivers/i2c/busses/i2c-designware-core.c static u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset) · line 262
Quoted 1 line
drivers/i2c/busses/i2c-designware-core.c +1 -0
+	} while (timeout-- > 0);
while (timeout--)?

OK, thanks.

Wolfram Sang

2013-04-09 09:28 UTC #12 lore raw

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

Quoted 20 lines · Mika Westerberg

Quoted 13 lines · Wolfram Sang
drivers/i2c/busses/i2c-designware-core.c static u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset) · line 261
Quoted 11 lines
drivers/i2c/busses/i2c-designware-core.c +10 -0
+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.

Quoted 7 lines

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.

Mika Westerberg

author maintainer

2013-04-09 09:34 UTC #13 lore raw

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

Quoted 24 lines · Wolfram Sang

Quoted 23 lines · Mika Westerberg
Quoted 13 lines · Wolfram Sang
drivers/i2c/busses/i2c-designware-core.c static u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset) · line 261
Quoted 11 lines
drivers/i2c/busses/i2c-designware-core.c +10 -0
+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.

Quoted 6 lines

Quoted 4 lines
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.

Quoted 1 line

Other than that, the series looks fine to me.

Thanks!

Mika Westerberg

author maintainer

2013-03-21 12:09 UTC #14 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/i2c/busses/i2c-designware-core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/i2c/busses/i2c-designware-core.c +1 -1

index 0e4f531..0849648 100644
@@ -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;

Signature · 1 line

1.7.10.4

Mika Westerberg

author maintainer

2013-03-21 12:08 UTC #15 lore raw

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

In reply to 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 <[email protected]>

---
 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(-)

drivers/i2c/busses/i2c-designware-core.c +2 -1

index 0849648..71d33e6 100644
@@ -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;

drivers/i2c/busses/i2c-designware-pcidrv.c +2 -1

index aacb64e..c8797e2 100644
@@ -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;

drivers/i2c/busses/i2c-designware-platdrv.c +2 -1

index a22a852..46a25ca 100644
@@ -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;
 }

Signature · 1 line

1.7.10.4

Mika Westerberg

author maintainer

2013-04-08 11:00 UTC #16 lore raw

In reply to Mika Westerberg

Hi Wolfram,

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

Thanks.

Quoted 164 lines · Mika Westerberg

From: Andy Shevchenko <[email protected]>

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 <[email protected]>

Signed-off-by Mika Westerberg <[email protected]>

---
 drivers/i2c/busses/i2c-designware-platdrv.c |   73 ++++++++-------------------
 1 file changed, 20 insertions(+), 53 deletions(-)

drivers/i2c/busses/i2c-designware-platdrv.c +20 ~~-53~~

index 0ceb6e1..c19c4ce 100644
@@ -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;
 }

Signature · 1 line

1.7.10.4

Wolfram Sang

2013-04-09 09:00 UTC #17 lore raw

In reply to Mika Westerberg

Quoted 12 lines

drivers/i2c/busses/i2c-designware-platdrv.c +1 -6

@@ -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);

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_probe(struct platform_device *pdev) · line 117

Quoted 10 lines

drivers/i2c/busses/i2c-designware-platdrv.c +5 -4

+	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.

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_probe(struct platform_device *pdev) · line 159

Quoted 11 lines

drivers/i2c/busses/i2c-designware-platdrv.c +3 -2

@@ -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

Mika Westerberg

author maintainer

2013-04-09 09:18 UTC #18 lore raw

Quoted 15 lines · Wolfram Sang

Quoted 15 lines

drivers/i2c/busses/i2c-designware-platdrv.c +3 -6

@@ -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;

Quoted 9 lines

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_probe(struct platform_device *pdev) · line 117
Quoted 7 lines
drivers/i2c/busses/i2c-designware-platdrv.c +3 -4
-	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.

Quoted 7 lines

drivers/i2c/busses/i2c-designware-platdrv.c +1 -1

@@ -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;

Quoted 7 lines

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_probe(struct platform_device *pdev) · line 159
Quoted 4 lines
drivers/i2c/busses/i2c-designware-platdrv.c +2 -1
 	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?

Wolfram Sang

2013-04-09 09:20 UTC #19 lore raw

Quoted 9 lines

Quoted 8 lines
drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_probe(struct platform_device *pdev) · line 159
Quoted 5 lines
drivers/i2c/busses/i2c-designware-platdrv.c +2 -1
 	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.

Quoted 3 lines

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!