I2C fixes

PATCH Superseded Shubhrajyoti D 2 participants 28 messages Apr 19, 2012 – Apr 26, 2012

Subsystems (4)

I2C subsystem host drivers OMAP2+ support OMAP1 support OMAP I2C driver

lore

Share & tools

lore

Shubhrajyoti D

author

2012-04-19 13:28 UTC #0 lore raw

v8 cover I2C fixes

This patch series seperates the fixes from other
changes from [2]

[2] http://www.spinics.net/lists/linux-omap/msg68056.html

The patch series does the following

- Warn fixes if CONFIG_PM_RUNTIME is not selected.
- In case of i2c remove register access was done without any get_sync fix the same.
- Folds a patch from Tasslehoff to prevent any merge conflicts.
- Prevents the XDUF flag to be set if the underflow condition is not met.
- As per discussion in [1] .Adds a patch to rename the 1p153 errata and use the unique id instead as the section number in the recent errata docs has changed.

[1] http://www.spinics.net/lists/linux-i2c/msg07607.html

Tested on omap4sdp and omap3sdp.

The following changes since commit 592fe8980688e7cba46897685d014c7fb3018a67:

Merge tag 'ext4_for_linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4 (2012-04-17 13:30:34 -0700)

are available in the git repository at:

git://gitorious.org/linus-tree/linus-tree.git i2c_omap-fixes

Shubhrajyoti D (9):
I2C: OMAP: make omap_i2c_unidle/idle functions depend on
CONFIG_PM_RUNTIME
I2C: OMAP: Fix the mismatch of pm_runtime enable and disable
I2C: OMAP: Fix the interrupt clearing in OMAP4
I2C: OMAP: Fix the error handling
I2C: OMAP: Don't check if wait_for_completion_timeout() returns less
than zero
I2C: OMAP: Fix the crash in i2c remove
I2C: OMAP: Handle error check for pm runtime
I2C: OMAP: Do not set the XUDF if the underflow is not reached
I2C: OMAP: Rename the 1p153 to the erratum id i462

Tasslehoff Kjappfot (1):
I2C: OMAP: fix missing handling of errata I2C_OMAP3_1P153

 drivers/i2c/busses/i2c-omap.c |  132 +++++++++++++++++++++--------------------
 1 files changed, 67 insertions(+), 65 deletions(-)

Signature · 1 line

1.7.5.4

Shubhrajyoti D

author

2012-04-19 13:28 UTC #1 lore raw

PATCH v8 1/10 I2C: OMAP: make omap_i2c_unidle/idle functions depend on CONFIG_PM_RUNTIME

The functions omap_i2c_unidle/idle are called from omap_i2c_runtime_resume and omap_i2c_runtime_suspend which is compiled for CONFIG_PM_RUNTIME. This patch removes the omap_i2c_unidle/idle functions and folds them into the runtime callbacks.

This fixes the below warn when CONFIG_PM_RUNTIME is not defined

CC arch/arm/mach-omap2/board-ti8168evm.o
drivers/i2c/busses/i2c-omap.c:272: warning: 'omap_i2c_unidle' defined but not used drivers/i2c/busses/i2c-omap.c:293: warning: 'omap_i2c_idle' defined but not used
CC net/ipv4/ip_forward.o

Signed-off-by Shubhrajyoti D <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c |   75 +++++++++++++++++-----------------------
 1 files changed, 32 insertions(+), 43 deletions(-)

drivers/i2c/busses/i2c-omap.c +32 ~~-43~~

index 801df60..4f4188d 100644
@@ -269,47 +269,6 @@ static inline u16 omap_i2c_read_reg(struct omap_i2c_dev *i2c_dev, int reg)
 				(i2c_dev->regs[reg] << i2c_dev->reg_shift));
 }
 
-static void omap_i2c_unidle(struct omap_i2c_dev *dev)
-{
-	if (dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
-		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
-		omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev->pscstate);
-		omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, dev->scllstate);
-		omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, dev->sclhstate);
-		omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG, dev->bufstate);
-		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, dev->syscstate);
-		omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate);
-		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
-	}
-
-	/*
-	 * Don't write to this register if the IE state is 0 as it can
-	 * cause deadlock.
-	 */
-	if (dev->iestate)
-		omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
-}
-
-static void omap_i2c_idle(struct omap_i2c_dev *dev)
-{
-	u16 iv;
-
-	dev->iestate = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
-	if (dev->dtrev == OMAP_I2C_IP_VERSION_2)
-		omap_i2c_write_reg(dev, OMAP_I2C_IP_V2_IRQENABLE_CLR, 1);
-	else
-		omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, 0);
-
-	if (dev->rev < OMAP_I2C_OMAP1_REV_2) {
-		iv = omap_i2c_read_reg(dev, OMAP_I2C_IV_REG); /* Read clears */
-	} else {
-		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, dev->iestate);
-
-		/* Flush posted write */
-		omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
-	}
-}
-
 static int omap_i2c_init(struct omap_i2c_dev *dev)
 {
 	u16 psc = 0, scll = 0, sclh = 0, buf = 0;

@@ -1163,8 +1122,22 @@ static int omap_i2c_runtime_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+	u16 iv;
+
+	_dev->iestate = omap_i2c_read_reg(_dev, OMAP_I2C_IE_REG);
+	if (_dev->dtrev == OMAP_I2C_IP_VERSION_2)
+		omap_i2c_write_reg(_dev, OMAP_I2C_IP_V2_IRQENABLE_CLR, 1);
+	else
+		omap_i2c_write_reg(_dev, OMAP_I2C_IE_REG, 0);
+
+	if (_dev->rev < OMAP_I2C_OMAP1_REV_2) {
+		iv = omap_i2c_read_reg(_dev, OMAP_I2C_IV_REG); /* Read clears */
+	} else {
+		omap_i2c_write_reg(_dev, OMAP_I2C_STAT_REG, _dev->iestate);
 
-	omap_i2c_idle(_dev);
+		/* Flush posted write */
+		omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG);
+	}
 
 	return 0;
 }

@@ -1174,7 +1147,23 @@ static int omap_i2c_runtime_resume(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
 
-	omap_i2c_unidle(_dev);
+	if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
+		omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, 0);
+		omap_i2c_write_reg(_dev, OMAP_I2C_PSC_REG, _dev->pscstate);
+		omap_i2c_write_reg(_dev, OMAP_I2C_SCLL_REG, _dev->scllstate);
+		omap_i2c_write_reg(_dev, OMAP_I2C_SCLH_REG, _dev->sclhstate);
+		omap_i2c_write_reg(_dev, OMAP_I2C_BUF_REG, _dev->bufstate);
+		omap_i2c_write_reg(_dev, OMAP_I2C_SYSC_REG, _dev->syscstate);
+		omap_i2c_write_reg(_dev, OMAP_I2C_WE_REG, _dev->westate);
+		omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
+	}
+
+	/*
+	 * Don't write to this register if the IE state is 0 as it can
+	 * cause deadlock.
+	 */
+	if (_dev->iestate)
+		omap_i2c_write_reg(_dev, OMAP_I2C_IE_REG, _dev->iestate);
 
 	return 0;
 }

Signature · 1 line

1.7.5.4

Shubhrajyoti D

author

2012-04-19 13:28 UTC #2 lore raw

PATCH v8 2/10 I2C: OMAP: Fix the mismatch of pm_runtime enable and disable

In reply to Shubhrajyoti D

Currently the i2c driver calls the pm_runtime_enable and never the disable. This may cause a warning when pm_runtime_enable checks for the count match.Attempting to fix the same by calling pm_runtime_disable in the error and the remove path.

Cc Kevin Hilman <[email protected]>

Cc Rajendra Nayak <[email protected]>

Signed-off-by Shubhrajyoti D <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

drivers/i2c/busses/i2c-omap.c +2 -0

index 4f4188d..c851672 100644
@@ -1090,6 +1090,7 @@ err_unuse_clocks:
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
 	pm_runtime_put(dev->dev);
 	iounmap(dev->base);
+	pm_runtime_disable(&pdev->dev);
 err_free_mem:
 	platform_set_drvdata(pdev, NULL);
 	kfree(dev);

@@ -1110,6 +1111,7 @@ omap_i2c_remove(struct platform_device *pdev)
 	free_irq(dev->irq, dev);
 	i2c_del_adapter(&dev->adapter);
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
+	pm_runtime_disable(&pdev->dev);
 	iounmap(dev->base);
 	kfree(dev);
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);

Signature · 1 line

1.7.5.4

Wolfram Sang

2012-04-23 15:43 UTC #3 lore raw

PATCH v8 2/10 I2C: OMAP: Fix the mismatch of pm_runtime enable and disable

Quoted 4 lines · Shubhrajyoti D

Currently the i2c driver calls the pm_runtime_enable and never the disable. This may cause a warning when pm_runtime_enable checks for the count match.Attempting to fix the same by calling pm_runtime_disable in the error and the remove path.

Why "attempting"?

Quoted 31 lines

Cc Kevin Hilman <[email protected]>

Cc Rajendra Nayak <[email protected]>

Signed-off-by Shubhrajyoti D <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

drivers/i2c/busses/i2c-omap.c +2 -0

index 4f4188d..c851672 100644
@@ -1090,6 +1090,7 @@ err_unuse_clocks:
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
 	pm_runtime_put(dev->dev);
 	iounmap(dev->base);
+	pm_runtime_disable(&pdev->dev);
 err_free_mem:
 	platform_set_drvdata(pdev, NULL);
 	kfree(dev);

@@ -1110,6 +1111,7 @@ omap_i2c_remove(struct platform_device *pdev)
 	free_irq(dev->irq, dev);
 	i2c_del_adapter(&dev->adapter);
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
+	pm_runtime_disable(&pdev->dev);
 	iounmap(dev->base);
 	kfree(dev);
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);

Signature · 1 line

1.7.5.4

Signature · 9 lines

Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120423/31f10c4f/attachment.sig>

Datta, Shubhrajyoti

author

2012-04-23 15:57 UTC #4 lore raw

PATCH v8 2/10 I2C: OMAP: Fix the mismatch of pm_runtime enable and disable

Thanks for review,

Quoted 7 lines · Wolfram Sang

Quoted 4 lines · Shubhrajyoti D
Currently the i2c driver calls the pm_runtime_enable and never the disable. This may cause a warning when pm_runtime_enable checks for the count match.Attempting to fix the same by calling pm_runtime_disable in the error and the remove path.
Why "attempting"?

yes will correct it. The updated patch below.

Quoted 1 line

From 1f6d8671d15b16de8840f0ff23d77d613a03e876 Mon Sep 17 00:00:00 2001

From: Shubhrajyoti D <[email protected]>
Date: Fri, 16 Dec 2011 19:43:16 +0530
Subject: [PATCHv8 02/10] I2C: OMAP: Fix the mismatch of pm_runtime enable and
disable

Currently the i2c driver calls the pm_runtime_enable and never the disable. This may cause a warning when pm_runtime_enable checks for the count match. Fix the same by calling pm_runtime_disable in the error and the remove path.

Cc Kevin Hilman <[email protected]>

Cc Rajendra Nayak <[email protected]>

Signed-off-by Shubhrajyoti D <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

drivers/i2c/busses/i2c-omap.c +2 -0

index 4f4188d..c851672 100644
@@ -1090,6 +1090,7 @@ err_unuse_clocks:
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
 	pm_runtime_put(dev->dev);
 	iounmap(dev->base);
+	pm_runtime_disable(&pdev->dev);
 err_free_mem:
 	platform_set_drvdata(pdev, NULL);
 	kfree(dev);

@@ -1110,6 +1111,7 @@ omap_i2c_remove(struct platform_device *pdev)
 	free_irq(dev->irq, dev);
 	i2c_del_adapter(&dev->adapter);
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
+	pm_runtime_disable(&pdev->dev);
 	iounmap(dev->base);
 	kfree(dev);
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);

Signature · 1 line

1.7.5.4

Shubhrajyoti D

author

2012-04-19 13:28 UTC #5 lore raw

PATCH v8 3/10 I2C: OMAP: Fix the interrupt clearing in OMAP4

In reply to Shubhrajyoti D

On OMAP4 we were writing 1 to IRQENABLE_CLR which cleared only the arbitration lost interrupt. The patch intends to fix the same by writing 0 to the IE register clearing all interrupts.

This is based on the work done by Vikram Pandita <[email protected]>.

The changes from the original patch ...
- Does not use the IRQENABLE_CLR register to clear as it is not mentioned to be legacy register IRQENABLE_CLR helps in atomically setting/clearing specific interrupts, instead use the OMAP_I2C_IE_REG as we are clearing all interrupts.

Cc Vikram Pandita <[email protected]>

Signed-off-by Shubhrajyoti D <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

drivers/i2c/busses/i2c-omap.c +2 -4

index c851672..bf07ffd 100644
@@ -1127,10 +1127,8 @@ static int omap_i2c_runtime_suspend(struct device *dev)
 	u16 iv;
 
 	_dev->iestate = omap_i2c_read_reg(_dev, OMAP_I2C_IE_REG);
-	if (_dev->dtrev == OMAP_I2C_IP_VERSION_2)
-		omap_i2c_write_reg(_dev, OMAP_I2C_IP_V2_IRQENABLE_CLR, 1);
-	else
-		omap_i2c_write_reg(_dev, OMAP_I2C_IE_REG, 0);
+
+	omap_i2c_write_reg(_dev, OMAP_I2C_IE_REG, 0);
 
 	if (_dev->rev < OMAP_I2C_OMAP1_REV_2) {
 		iv = omap_i2c_read_reg(_dev, OMAP_I2C_IV_REG); /* Read clears */

Signature · 1 line

1.7.5.4

Shubhrajyoti D

author

2012-04-19 13:28 UTC #6 lore raw

PATCH v8 4/10 I2C: OMAP: Fix the error handling

In reply to Shubhrajyoti D

Currently in probe

      pm_runtime_put(dev->dev);

...

        /* i2c device drivers may be active on return from add_adapter() */
        adap->nr = pdev->id;
        r = i2c_add_numbered_adapter(adap);
        if (r) {
                dev_err(dev->dev, "failure adding adapter\n");
                goto err_free_irq;
        }

...

        return 0;

err_free_irq:

        free_irq(dev->irq, dev);
err_unuse_clocks:
        omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
        pm_runtime_put(dev->dev);

This may access the i2c registers without the clocks.
Attempting to fix the same by moving the pm_rintime_put after the error check.

Signed-off-by Shubhrajyoti D <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/i2c/busses/i2c-omap.c +2 -2

index bf07ffd..1777d79 100644
@@ -1061,8 +1061,6 @@ omap_i2c_probe(struct platform_device *pdev)
 	dev_info(dev->dev, "bus %d rev%d.%d.%d at %d kHz\n", pdev->id,
 		 dev->dtrev, dev->rev >> 4, dev->rev & 0xf, dev->speed);
 
-	pm_runtime_put(dev->dev);
-
 	adap = &dev->adapter;
 	i2c_set_adapdata(adap, dev);
 	adap->owner = THIS_MODULE;

@@ -1082,6 +1080,8 @@ omap_i2c_probe(struct platform_device *pdev)
 
 	of_i2c_register_devices(adap);
 
+	pm_runtime_put(dev->dev);
+
 	return 0;
 
 err_free_irq:

Signature · 1 line

1.7.5.4

Wolfram Sang

2012-04-23 17:10 UTC #7 lore raw

PATCH v8 4/10 I2C: OMAP: Fix the error handling

The subject is too generic. (I don't want to be negative always: The following description of the problem is good :))

Quoted 2 lines · Shubhrajyoti D

Currently in probe

      pm_runtime_put(dev->dev);

Indentation wrong.

Quoted 13 lines

...

        /* i2c device drivers may be active on return from add_adapter() */
        adap->nr = pdev->id;
        r = i2c_add_numbered_adapter(adap);
        if (r) {
                dev_err(dev->dev, "failure adding adapter\n");
                goto err_free_irq;
        }

...

        return 0;

Quoted 8 lines

err_free_irq:
        free_irq(dev->irq, dev);
err_unuse_clocks:
        omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
        pm_runtime_put(dev->dev);
This may access the i2c registers without the clocks.
Attempting to fix the same by moving the pm_rintime_put after the error check.

Drop "Attempting".

Quoted 31 lines

Signed-off-by Shubhrajyoti D <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/i2c/busses/i2c-omap.c +2 -2

index bf07ffd..1777d79 100644
@@ -1061,8 +1061,6 @@ omap_i2c_probe(struct platform_device *pdev)
 	dev_info(dev->dev, "bus %d rev%d.%d.%d at %d kHz\n", pdev->id,
 		 dev->dtrev, dev->rev >> 4, dev->rev & 0xf, dev->speed);
 
-	pm_runtime_put(dev->dev);
-
 	adap = &dev->adapter;
 	i2c_set_adapdata(adap, dev);
 	adap->owner = THIS_MODULE;

@@ -1082,6 +1080,8 @@ omap_i2c_probe(struct platform_device *pdev)
 
 	of_i2c_register_devices(adap);
 
+	pm_runtime_put(dev->dev);
+
 	return 0;
 
 err_free_irq:

Signature · 1 line

1.7.5.4

Signature · 9 lines

Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120423/31f1c682/attachment.sig>

Shubhrajyoti D

author

2012-04-19 13:28 UTC #8 lore raw

PATCH v8 5/10 I2C: OMAP: Don't check if wait_for_completion_timeout() returns less than zero

In reply to Shubhrajyoti D

By definition, wait_for_completion_timeout() returns an unsigned value and therefore, it is not necessary to check if the return value is less than zero as this is not possible.

This is based on a patch from Jon Hunter <[email protected]> Changes from his patch
- Declare a long as the wait_for_completion_timeout returns long.

Original patch is
http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=ea02cece7b0000bc736e60c4188a11aaa74bc6e6

Cc Jon Hunter <[email protected]>

Signed-off-by Shubhrajyoti D <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

drivers/i2c/busses/i2c-omap.c +4 -6

index 1777d79..fec8d5c 100644
@@ -473,7 +473,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 			     struct i2c_msg *msg, int stop)
 {
 	struct omap_i2c_dev *dev = i2c_get_adapdata(adap);
-	int r;
+	unsigned long timeout;
 	u16 w;
 
 	dev_dbg(dev->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",

@@ -541,12 +541,10 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 	 * REVISIT: We should abort the transfer on signals, but the bus goes
 	 * into arbitration and we're currently unable to recover from it.
 	 */
-	r = wait_for_completion_timeout(&dev->cmd_complete,
-					OMAP_I2C_TIMEOUT);
+	timeout = wait_for_completion_timeout(&dev->cmd_complete,
+						OMAP_I2C_TIMEOUT);
 	dev->buf_len = 0;
-	if (r < 0)
-		return r;
-	if (r == 0) {
+	if (timeout == 0) {
 		dev_err(dev->dev, "controller timed out\n");
 		omap_i2c_init(dev);
 		return -ETIMEDOUT;

Signature · 1 line

1.7.5.4

Wolfram Sang

2012-04-23 17:11 UTC #9 lore raw

PATCH v8 5/10 I2C: OMAP: Don't check if wait_for_completion_timeout() returns less than zero

Quoted 10 lines · Shubhrajyoti D

By definition, wait_for_completion_timeout() returns an unsigned value and therefore, it is not necessary to check if the return value is less than zero as this is not possible.
This is based on a patch from Jon Hunter <[email protected]> Changes from his patch
- Declare a long as the wait_for_completion_timeout returns long.
Original patch is
http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=ea02cece7b0000bc736e60c4188a11aaa74bc6e6

The original patch has a signed-off, so you need to include it here as well.

Quoted 39 lines

Cc Jon Hunter <[email protected]>

Signed-off-by Shubhrajyoti D <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

drivers/i2c/busses/i2c-omap.c +4 -6

index 1777d79..fec8d5c 100644
@@ -473,7 +473,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 			     struct i2c_msg *msg, int stop)
 {
 	struct omap_i2c_dev *dev = i2c_get_adapdata(adap);
-	int r;
+	unsigned long timeout;
 	u16 w;
 
 	dev_dbg(dev->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",

@@ -541,12 +541,10 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 	 * REVISIT: We should abort the transfer on signals, but the bus goes
 	 * into arbitration and we're currently unable to recover from it.
 	 */
-	r = wait_for_completion_timeout(&dev->cmd_complete,
-					OMAP_I2C_TIMEOUT);
+	timeout = wait_for_completion_timeout(&dev->cmd_complete,
+						OMAP_I2C_TIMEOUT);
 	dev->buf_len = 0;
-	if (r < 0)
-		return r;
-	if (r == 0) {
+	if (timeout == 0) {
 		dev_err(dev->dev, "controller timed out\n");
 		omap_i2c_init(dev);
 		return -ETIMEDOUT;

Signature · 1 line

1.7.5.4

Signature · 9 lines

Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120423/08dc2423/attachment-0001.sig>

Shubhrajyoti D

author

2012-04-19 13:28 UTC #10 lore raw

PATCH v8 6/10 I2C: OMAP: Fix the crash in i2c remove

In reply to Shubhrajyoti D

    In omap_i2c_remove we are accessing the I2C_CON register without
    enabling the clocks. Fix the same by enabling the clocks and disabling
    it.
    This fixes the following crash.
    [  154.723022] ------------[ cut here ]------------
    [  154.725677] WARNING: at arch/arm/mach-omap2/omap_l3_noc.c:112 l3_interrupt_handler+0x1b4/0x1c4()
    [  154.725677] L3 custom error: MASTER:MPU TARGET:L4 PER2
    [  154.742614] Modules linked in: i2c_omap(-)
    [  154.746948] Backtrace:
    [  154.746948] [<c0013078>] (dump_backtrace+0x0/0x110) from [<c026c158>] (dump_stack+0x18/0x1c)
    [  154.752716]  r6:00000070 r5:c002c43c r4:df9b9e98 r3:df9b8000
    [  154.764465] [<c026c140>] (dump_stack+0x0/0x1c) from [<c0041a2c>] (warn_slowpath_common+0x5c/0x6c)
    [  154.768341] [<c00419d0>] (warn_slowpath_common+0x0/0x6c) from [<c0041ae0>] (warn_slowpath_fmt+0x38/0x40)
    [  154.776153]  r8:00000180 r7:c0361594 r6:c0379b48 r5:00080003 r4:e0838b00
    [  154.790771] r3:00000009
    [  154.791778] [<c0041aa8>] (warn_slowpath_fmt+0x0/0x40) from [<c002c43c>] (l3_interrupt_handler+0x1b4/0x1c4)
    [  154.803710]  r3:c0361598 r2:c02ef74c
    [  154.807403] [<c002c288>] (l3_interrupt_handler+0x0/0x1c4) from [<c0085f44>] (handle_irq_event_percpu+0x58/0
    [  154.818237]  r8:0000002a r7:00000000 r6:00000000 r5:df808054 r4:df8893c0
    [  154.825378] [<c0085eec>] (handle_irq_event_percpu+0x0/0x188) from [<c00860b8>] (handle_irq_event+0x44/0x64)
    [  154.835662] [<c0086074>] (handle_irq_event+0x0/0x64) from [<c0088ec0>] (handle_fasteoi_irq+0xa4/0x10c)
    [  154.845458]  r6:0000002a r5:df808054 r4:df808000 r3:c034a150
    [  154.846466] [<c0088e1c>] (handle_fasteoi_irq+0x0/0x10c) from [<c0085ed0>] (generic_handle_irq+0x30/0x38)
    [  154.854278]  r5:c034aa48 r4:0000002a
    [  154.862091] [<c0085ea0>] (generic_handle_irq+0x0/0x38) from [<c000fd38>] (handle_IRQ+0x60/0xc0)
    [  154.874450]  r4:c034ea70 r3:000001f8
    [  154.878234] [<c000fcd8>] (handle_IRQ+0x0/0xc0) from [<c0008478>] (gic_handle_irq+0x20/0x5c)
    [  154.887023]  r7:ffffff40 r6:df9b9fb0 r5:c034e2b4 r4:0000001a
    [  154.887054] [<c0008458>] (gic_handle_irq+0x0/0x5c) from [<c000ea80>] (__irq_usr+0x40/0x60)
    [  154.901153] Exception stack(0xdf9b9fb0 to 0xdf9b9ff8)
    [  154.907104] 9fa0:                                     beaf1f04 4006be00 0000000f 0000000c
    [  154.915710] 9fc0: 4006c000 00000000 00008034 ffffff40 00000007 00000000 00000000 0007b8d7
    [  154.916778] 9fe0: 00000000 beaf1b68 0000d23c 4005baf0 80000010 ffffffff
    [  154.931335]  r6:ffffffff r5:80000010 r4:4005baf0 r3:beaf1f04
    [  154.937316] ---[ end trace 1b75b31a2719ed21 ]--

Cc <[email protected]>

Signed-off-by Shubhrajyoti D <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

drivers/i2c/busses/i2c-omap.c +2 -0

index fec8d5c..44e8cfa 100644
@@ -1108,7 +1108,9 @@ omap_i2c_remove(struct platform_device *pdev)
 
 	free_irq(dev->irq, dev);
 	i2c_del_adapter(&dev->adapter);
+	pm_runtime_get_sync(&pdev->dev);
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
+	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	iounmap(dev->base);
 	kfree(dev);

Signature · 1 line

1.7.5.4

Wolfram Sang

2012-04-23 16:49 UTC #11 lore raw

PATCH v8 6/10 I2C: OMAP: Fix the crash in i2c remove

Quoted 36 lines · Shubhrajyoti D

    In omap_i2c_remove we are accessing the I2C_CON register without
    enabling the clocks. Fix the same by enabling the clocks and disabling
    it.
    This fixes the following crash.
    [  154.723022] ------------[ cut here ]------------
    [  154.725677] WARNING: at arch/arm/mach-omap2/omap_l3_noc.c:112 l3_interrupt_handler+0x1b4/0x1c4()
    [  154.725677] L3 custom error: MASTER:MPU TARGET:L4 PER2
    [  154.742614] Modules linked in: i2c_omap(-)
    [  154.746948] Backtrace:
    [  154.746948] [<c0013078>] (dump_backtrace+0x0/0x110) from [<c026c158>] (dump_stack+0x18/0x1c)
    [  154.752716]  r6:00000070 r5:c002c43c r4:df9b9e98 r3:df9b8000
    [  154.764465] [<c026c140>] (dump_stack+0x0/0x1c) from [<c0041a2c>] (warn_slowpath_common+0x5c/0x6c)
    [  154.768341] [<c00419d0>] (warn_slowpath_common+0x0/0x6c) from [<c0041ae0>] (warn_slowpath_fmt+0x38/0x40)
    [  154.776153]  r8:00000180 r7:c0361594 r6:c0379b48 r5:00080003 r4:e0838b00
    [  154.790771] r3:00000009
    [  154.791778] [<c0041aa8>] (warn_slowpath_fmt+0x0/0x40) from [<c002c43c>] (l3_interrupt_handler+0x1b4/0x1c4)
    [  154.803710]  r3:c0361598 r2:c02ef74c
    [  154.807403] [<c002c288>] (l3_interrupt_handler+0x0/0x1c4) from [<c0085f44>] (handle_irq_event_percpu+0x58/0
    [  154.818237]  r8:0000002a r7:00000000 r6:00000000 r5:df808054 r4:df8893c0
    [  154.825378] [<c0085eec>] (handle_irq_event_percpu+0x0/0x188) from [<c00860b8>] (handle_irq_event+0x44/0x64)
    [  154.835662] [<c0086074>] (handle_irq_event+0x0/0x64) from [<c0088ec0>] (handle_fasteoi_irq+0xa4/0x10c)
    [  154.845458]  r6:0000002a r5:df808054 r4:df808000 r3:c034a150
    [  154.846466] [<c0088e1c>] (handle_fasteoi_irq+0x0/0x10c) from [<c0085ed0>] (generic_handle_irq+0x30/0x38)
    [  154.854278]  r5:c034aa48 r4:0000002a
    [  154.862091] [<c0085ea0>] (generic_handle_irq+0x0/0x38) from [<c000fd38>] (handle_IRQ+0x60/0xc0)
    [  154.874450]  r4:c034ea70 r3:000001f8
    [  154.878234] [<c000fcd8>] (handle_IRQ+0x0/0xc0) from [<c0008478>] (gic_handle_irq+0x20/0x5c)
    [  154.887023]  r7:ffffff40 r6:df9b9fb0 r5:c034e2b4 r4:0000001a
    [  154.887054] [<c0008458>] (gic_handle_irq+0x0/0x5c) from [<c000ea80>] (__irq_usr+0x40/0x60)
    [  154.901153] Exception stack(0xdf9b9fb0 to 0xdf9b9ff8)
    [  154.907104] 9fa0:                                     beaf1f04 4006be00 0000000f 0000000c
    [  154.915710] 9fc0: 4006c000 00000000 00008034 ffffff40 00000007 00000000 00000000 0007b8d7
    [  154.916778] 9fe0: 00000000 beaf1b68 0000d23c 4005baf0 80000010 ffffffff
    [  154.931335]  r6:ffffffff r5:80000010 r4:4005baf0 r3:beaf1f04
    [  154.937316] ---[ end trace 1b75b31a2719ed21 ]--

Quoted 2 lines

Cc <[email protected]>
Signed-off-by Shubhrajyoti D <[email protected]>

Is this really the correct solution? I do wonder that every driver using runtime PM should enable the clocks on their own. That should be done by the core, I'd say; it is not unusual that drivers need to write to registers in remove(). If it is correct, can I get some acks?

Quoted 21 lines

---
 drivers/i2c/busses/i2c-omap.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

drivers/i2c/busses/i2c-omap.c +2 -0

index fec8d5c..44e8cfa 100644
@@ -1108,7 +1108,9 @@ omap_i2c_remove(struct platform_device *pdev)
 
 	free_irq(dev->irq, dev);
 	i2c_del_adapter(&dev->adapter);
+	pm_runtime_get_sync(&pdev->dev);
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
+	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	iounmap(dev->base);
 	kfree(dev);

Signature · 1 line

1.7.5.4

Signature · 9 lines

Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120423/aded4a0d/attachment.sig>

Shubhrajyoti

author

2012-04-24 18:14 UTC #12 lore raw

PATCH v8 6/10 I2C: OMAP: Fix the crash in i2c remove

Quoted 7 lines · Wolfram Sang

Quoted 7 lines

    [  154.901153] Exception stack(0xdf9b9fb0 to 0xdf9b9ff8)

Quoted 6 lines

    [  154.907104] 9fa0:                                     beaf1f04 4006be00 0000000f 0000000c
    [  154.915710] 9fc0: 4006c000 00000000 00008034 ffffff40 00000007 00000000 00000000 0007b8d7
    [  154.916778] 9fe0: 00000000 beaf1b68 0000d23c 4005baf0 80000010 ffffffff
    [  154.931335]  r6:ffffffff r5:80000010 r4:4005baf0 r3:beaf1f04
    [  154.937316] ---[ end trace 1b75b31a2719ed21 ]--

Quoted 5 lines

Quoted 2 lines
Quoted 2 lines
Cc <[email protected]>
Signed-off-by Shubhrajyoti D <[email protected]>
Is this really the correct solution? I do wonder that every driver using runtime PM should enable the clocks on their own. That should be done by the core,

By core you don't mean the i2c core but the pm layer right?

Quoted 2 lines

I'd say; it is not unusual that drivers need to write to registers in remove(). If it is correct, can I get some acks?

I did see the crash.

Will wait for the pm experts to comment.

Quoted 5 lines

---
 drivers/i2c/busses/i2c-omap.c |    2 ++

 1 files changed, 2 insertions(+), 0 deletions(-)

Wolfram Sang

2012-04-24 18:18 UTC #13 lore raw

PATCH v8 6/10 I2C: OMAP: Fix the crash in i2c remove

Quoted 8 lines · Shubhrajyoti

Quoted 7 lines · Wolfram Sang

Quoted 7 lines

    [  154.901153] Exception stack(0xdf9b9fb0 to 0xdf9b9ff8)

Quoted 6 lines

    [  154.907104] 9fa0:                                     beaf1f04 4006be00 0000000f 0000000c
    [  154.915710] 9fc0: 4006c000 00000000 00008034 ffffff40 00000007 00000000 00000000 0007b8d7
    [  154.916778] 9fe0: 00000000 beaf1b68 0000d23c 4005baf0 80000010 ffffffff
    [  154.931335]  r6:ffffffff r5:80000010 r4:4005baf0 r3:beaf1f04
    [  154.937316] ---[ end trace 1b75b31a2719ed21 ]--

Quoted 6 lines

Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
Cc <[email protected]>
Signed-off-by Shubhrajyoti D <[email protected]>
Is this really the correct solution? I do wonder that every driver using runtime PM should enable the clocks on their own. That should be done by the core,
By core you don't mean the i2c core but the pm layer right?

Yes.

Quoted 3 lines

Quoted 2 lines
I'd say; it is not unusual that drivers need to write to registers in remove(). If it is correct, can I get some acks?
I did see the crash.

That was never a doubt. With "correct" I meant "correct solution".

Quoted 1 line

Will wait for the pm experts to comment.

Yup.

Signature · 9 lines

Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120424/683e5f8d/attachment.sig>

Shubhrajyoti

author

2012-04-26 06:28 UTC #14 lore raw

PATCH v8 6/10 I2C: OMAP: Fix the crash in i2c remove

Quoted 14 lines · Wolfram Sang

Quoted 13 lines · Shubhrajyoti
Quoted 12 lines · Wolfram Sang
Quoted 9 lines
    [  154.901153] Exception stack(0xdf9b9fb0 to 0xdf9b9ff8)
Quoted 8 lines
    [  154.907104] 9fa0:                                     beaf1f04 4006be00 0000000f 0000000c
    [  154.915710] 9fc0: 4006c000 00000000 00008034 ffffff40 00000007 00000000 00000000 0007b8d7
    [  154.916778] 9fe0: 00000000 beaf1b68 0000d23c 4005baf0 80000010 ffffffff
    [  154.931335]  r6:ffffffff r5:80000010 r4:4005baf0 r3:beaf1f04
    [  154.937316] ---[ end trace 1b75b31a2719ed21 ]--
Cc <[email protected]>
Signed-off-by Shubhrajyoti D <[email protected]>
Is this really the correct solution? I do wonder that every driver using runtime PM should enable the clocks on their own. That should be done by the core,

Quoted 9 lines

Quoted 1 line
By core you don't mean the i2c core but the pm layer right?
Yes.
Quoted 3 lines
Quoted 2 lines
I'd say; it is not unusual that drivers need to write to registers in remove(). If it is correct, can I get some acks?
I did see the crash.
That was never a doubt. With "correct" I meant "correct solution".
Quoted 1 line
Will wait for the pm experts to comment.

As far as I know I don?t think that the pm layer doesn't enable the clocks for the drivers on remove. Maybe Kevin or Rajendra can comment better.

Shubhrajyoti

author

2012-04-26 06:43 UTC #15 lore raw

PATCH v8 6/10 I2C: OMAP: Fix the crash in i2c remove

Quoted 18 lines · Shubhrajyoti

Quoted 17 lines · Wolfram Sang
Quoted 8 lines · Shubhrajyoti
Quoted 7 lines · Wolfram Sang
Quoted 7 lines
    [  154.901153] Exception stack(0xdf9b9fb0 to 0xdf9b9ff8)
Quoted 6 lines
    [  154.907104] 9fa0:                                     beaf1f04 4006be00 0000000f 0000000c
    [  154.915710] 9fc0: 4006c000 00000000 00008034 ffffff40 00000007 00000000 00000000 0007b8d7
    [  154.916778] 9fe0: 00000000 beaf1b68 0000d23c 4005baf0 80000010 ffffffff
    [  154.931335]  r6:ffffffff r5:80000010 r4:4005baf0 r3:beaf1f04
    [  154.937316] ---[ end trace 1b75b31a2719ed21 ]--
Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
Cc <[email protected]>
Signed-off-by Shubhrajyoti D <[email protected]>
Is this really the correct solution? I do wonder that every driver using runtime PM should enable the clocks on their own. That should be done by the core,
By core you don't mean the i2c core but the pm layer right?
Yes.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Quoted 2 lines
I'd say; it is not unusual that drivers need to write to registers in remove(). If it is correct, can I get some acks?
I did see the crash.
That was never a doubt. With "correct" I meant "correct solution".
Quoted 1 line
Will wait for the pm experts to comment.
As far as I know I don?t think that the pm layer doesn't enable the clocks for the drivers on remove. Maybe Kevin or Rajendra can comment better.

typo what i meant was

As far as I know the pm layer doesn't enable the clocks
for the drivers on remove. Maybe Kevin or Rajendra can comment better.

thanks,

Quoted 2 lines

Shubhrajyoti D

author

2012-04-19 13:28 UTC #16 lore raw

PATCH v8 7/10 I2C: OMAP: Handle error check for pm runtime

In reply to Shubhrajyoti D

If PM runtime get_sync fails return with the error so that no further reads/writes goes through the interface. This will avoid possible abort. Add a error message in case of failure with the cause of the failure.

Signed-off-by Shubhrajyoti D <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

drivers/i2c/busses/i2c-omap.c +16 -3

index 44e8cfa..d555dcd 100644
@@ -585,7 +585,11 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	int i;
 	int r;
 
-	pm_runtime_get_sync(dev->dev);
+	r = pm_runtime_get_sync(dev->dev);
+	if (r < 0) {
+		dev_err(dev->dev, "pm_runtime_get_sync failed %d\n", r);
+		return r;
+	}
 
 	r = omap_i2c_wait_for_bb(dev);
 	if (r < 0)

@@ -1011,7 +1015,11 @@ omap_i2c_probe(struct platform_device *pdev)
 		dev->regs = (u8 *)reg_map_ip_v1;
 
 	pm_runtime_enable(dev->dev);
-	pm_runtime_get_sync(dev->dev);
+	r = pm_runtime_get_sync(dev->dev);
+	if (r < 0) {
+		dev_err(dev->dev, "pm_runtime_get_sync failed:%d\n", r);
+		return r;
+	}
 
 	dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff;

@@ -1103,12 +1111,17 @@ omap_i2c_remove(struct platform_device *pdev)
 {
 	struct omap_i2c_dev	*dev = platform_get_drvdata(pdev);
 	struct resource		*mem;
+	int ret;
 
 	platform_set_drvdata(pdev, NULL);
 
 	free_irq(dev->irq, dev);
 	i2c_del_adapter(&dev->adapter);
-	pm_runtime_get_sync(&pdev->dev);
+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (ret < 0) {
+		dev_err(dev->dev, "pm_runtime_get_sync failed %d\n", ret);
+		return ret;
+	}
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
 	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);

Signature · 1 line

1.7.5.4

Wolfram Sang

2012-04-23 16:20 UTC #17 lore raw

PATCH v8 7/10 I2C: OMAP: Handle error check for pm runtime

Quoted 4 lines · Shubhrajyoti D

If PM runtime get_sync fails return with the error so that no further reads/writes goes through the interface. This will avoid possible abort. Add a error message in case of failure with the cause of the failure.

I don't think the error message is especially helpful. You also use different string (probably typo).

Quoted 24 lines

Signed-off-by Shubhrajyoti D <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

drivers/i2c/busses/i2c-omap.c +5 -1

index 44e8cfa..d555dcd 100644
@@ -585,7 +585,11 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	int i;
 	int r;
 
-	pm_runtime_get_sync(dev->dev);
+	r = pm_runtime_get_sync(dev->dev);
+	if (r < 0) {
+		dev_err(dev->dev, "pm_runtime_get_sync failed %d\n", r);
+		return r;
+	}
 
 	r = omap_i2c_wait_for_bb(dev);
 	if (r < 0)

drivers/i2c/busses/i2c-omap.c omap_i2c_probe(struct platform_device *pdev) · line 1022

Quoted 9 lines

drivers/i2c/busses/i2c-omap.c +5 -1

@@ -1011,7 +1015,11 @@ omap_i2c_probe(struct platform_device *pdev)
 		dev->regs = (u8 *)reg_map_ip_v1;
 
 	pm_runtime_enable(dev->dev);
-	pm_runtime_get_sync(dev->dev);
+	r = pm_runtime_get_sync(dev->dev);
+	if (r < 0) {
+		dev_err(dev->dev, "pm_runtime_get_sync failed:%d\n", r);
+		return r;
+	}

Smatch says:

drivers/i2c/busses/i2c-omap.c:1021 omap_i2c_probe() warn: 'mem->start' was not released on error

In fact, you are leaking quite more.

Quoted 6 lines

drivers/i2c/busses/i2c-omap.c +1 -0

@@ -1103,12 +1111,17 @@ omap_i2c_remove(struct platform_device *pdev)
 {
 	struct omap_i2c_dev	*dev = platform_get_drvdata(pdev);
 	struct resource		*mem;
+	int ret;

drivers/i2c/busses/i2c-omap.c omap_i2c_remove(struct platform_device *pdev) · line 1124

Quoted 10 lines

drivers/i2c/busses/i2c-omap.c +5 -1

 	platform_set_drvdata(pdev, NULL);
 
 	free_irq(dev->irq, dev);
 	i2c_del_adapter(&dev->adapter);
-	pm_runtime_get_sync(&pdev->dev);
+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (ret < 0) {
+		dev_err(dev->dev, "pm_runtime_get_sync failed %d\n", ret);
+		return ret;
+	}

I am no too familiar with runtime_pm. Is it really so bad to fail remove, when get_sync has an error? Why is it checked and e.g. pm_runtime_put later is not? Any pointers?

Quoted 6 lines

drivers/i2c/busses/i2c-omap.c +0 -0

 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
 	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);

Signature · 1 line

1.7.5.4

Thanks,

Wolfram

Signature · 9 lines

Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120423/e62475bd/attachment.sig>

Shubhrajyoti

author

2012-04-23 17:26 UTC #18 lore raw

PATCH v8 7/10 I2C: OMAP: Handle error check for pm runtime

Quoted 7 lines · Wolfram Sang

Quoted 4 lines · Shubhrajyoti D
If PM runtime get_sync fails return with the error so that no further reads/writes goes through the interface. This will avoid possible abort. Add a error message in case of failure with the cause of the failure.
I don't think the error message is especially helpful. You also use different string (probably typo).

Will correct and resend

Quoted 35 lines

Quoted 24 lines

Signed-off-by Shubhrajyoti D <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

drivers/i2c/busses/i2c-omap.c +5 -1

index 44e8cfa..d555dcd 100644
@@ -585,7 +585,11 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	int i;
 	int r;
 
-	pm_runtime_get_sync(dev->dev);
+	r = pm_runtime_get_sync(dev->dev);
+	if (r < 0) {
+		dev_err(dev->dev, "pm_runtime_get_sync failed %d\n", r);
+		return r;
+	}
 
 	r = omap_i2c_wait_for_bb(dev);
 	if (r < 0)

drivers/i2c/busses/i2c-omap.c omap_i2c_probe(struct platform_device *pdev) · line 1022

Quoted 9 lines

drivers/i2c/busses/i2c-omap.c +5 -1

@@ -1011,7 +1015,11 @@ omap_i2c_probe(struct platform_device *pdev)
 		dev->regs = (u8 *)reg_map_ip_v1;
 
 	pm_runtime_enable(dev->dev);
-	pm_runtime_get_sync(dev->dev);
+	r = pm_runtime_get_sync(dev->dev);
+	if (r < 0) {
+		dev_err(dev->dev, "pm_runtime_get_sync failed:%d\n", r);
+		return r;
+	}

Smatch says:

Quoted 3 lines

drivers/i2c/busses/i2c-omap.c:1021 omap_i2c_probe() warn: 'mem->start' was not released on error
In fact, you are leaking quite more.

Ops will correct, thanks.

Quoted 9 lines

drivers/i2c/busses/i2c-omap.c +1 -0

@@ -1103,12 +1111,17 @@ omap_i2c_remove(struct platform_device *pdev)
 {
 	struct omap_i2c_dev	*dev = platform_get_drvdata(pdev);
 	struct resource		*mem;
+	int ret;
 
 	platform_set_drvdata(pdev, NULL);

Quoted 11 lines

drivers/i2c/busses/i2c-omap.c omap_i2c_remove(struct platform_device *pdev) · line 1124
Quoted 8 lines
drivers/i2c/busses/i2c-omap.c +5 -1
 	free_irq(dev->irq, dev);
 	i2c_del_adapter(&dev->adapter);
-	pm_runtime_get_sync(&pdev->dev);
+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (ret < 0) {
+		dev_err(dev->dev, "pm_runtime_get_sync failed %d\n", ret);
+		return ret;
+	}
I am no too familiar with runtime_pm. Is it really so bad to fail remove, when get_sync has an error? Why is it checked and e.g. pm_runtime_put later is not? Any pointers?

If the get_sync fails the clock were not enabled so checking to prevent register access.

Quoted 11 lines

Quoted 7 lines
drivers/i2c/busses/i2c-omap.c +0 -0
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
 	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
Signature · 1 line
1.7.5.4
Thanks,
Wolfram

Shubhrajyoti D

author

2012-04-19 13:28 UTC #19 lore raw

PATCH v8 8/10 I2C: OMAP: fix missing handling of errata I2C_OMAP3_1P153

In reply to Shubhrajyoti D

From: Tasslehoff Kjappfot <[email protected]>

i2c_probe set the dev->errata flag, but omap_i2c_init cleared the flag again. Move the errata handling to i2c_probe.

Signed-off-by Tasslehoff Kjappfot <[email protected]>

Signed-off-by Shubhrajyoti D <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

drivers/i2c/busses/i2c-omap.c +5 -5

index d555dcd..9323201 100644
@@ -427,11 +427,6 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 	/* Take the I2C module out of reset: */
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
 
-	dev->errata = 0;
-
-	if (dev->flags & OMAP_I2C_FLAG_APPLY_ERRATA_I207)
-		dev->errata |= I2C_OMAP_ERRATA_I207;
-
 	/* Enable interrupts */
 	dev->iestate = (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
 			OMAP_I2C_IE_ARDY | OMAP_I2C_IE_NACK |

@@ -1023,6 +1018,11 @@ omap_i2c_probe(struct platform_device *pdev)
 
 	dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff;
 
+	dev->errata = 0;
+
+	if (dev->flags & OMAP_I2C_FLAG_APPLY_ERRATA_I207)
+		dev->errata |= I2C_OMAP_ERRATA_I207;
+
 	if (dev->rev <= OMAP_I2C_REV_ON_3430)
 		dev->errata |= I2C_OMAP3_1P153;

Signature · 1 line

1.7.5.4

Wolfram Sang

2012-04-23 16:43 UTC #20 lore raw

PATCH v8 8/10 I2C: OMAP: fix missing handling of errata I2C_OMAP3_1P153

Quoted 7 lines · Shubhrajyoti D

From: Tasslehoff Kjappfot <[email protected]>
i2c_probe set the dev->errata flag, but omap_i2c_init cleared the flag again. Move the errata handling to i2c_probe.
Signed-off-by Tasslehoff Kjappfot <[email protected]>
Signed-off-by Shubhrajyoti D <[email protected]>

Subject looks bogus to me. You are handling I207 here.

Quoted 35 lines

---
 drivers/i2c/busses/i2c-omap.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

drivers/i2c/busses/i2c-omap.c +5 -5

index d555dcd..9323201 100644
@@ -427,11 +427,6 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 	/* Take the I2C module out of reset: */
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
 
-	dev->errata = 0;
-
-	if (dev->flags & OMAP_I2C_FLAG_APPLY_ERRATA_I207)
-		dev->errata |= I2C_OMAP_ERRATA_I207;
-
 	/* Enable interrupts */
 	dev->iestate = (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
 			OMAP_I2C_IE_ARDY | OMAP_I2C_IE_NACK |

@@ -1023,6 +1018,11 @@ omap_i2c_probe(struct platform_device *pdev)
 
 	dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff;
 
+	dev->errata = 0;
+
+	if (dev->flags & OMAP_I2C_FLAG_APPLY_ERRATA_I207)
+		dev->errata |= I2C_OMAP_ERRATA_I207;
+
 	if (dev->rev <= OMAP_I2C_REV_ON_3430)
 		dev->errata |= I2C_OMAP3_1P153;

Signature · 1 line

1.7.5.4

Signature · 9 lines

Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120423/ac813654/attachment.sig>

Shubhrajyoti

author

2012-04-23 17:35 UTC #21 lore raw

PATCH v8 8/10 I2C: OMAP: fix missing handling of errata I2C_OMAP3_1P153

Quoted 9 lines · Wolfram Sang

Quoted 7 lines · Shubhrajyoti D
From: Tasslehoff Kjappfot <[email protected]>
i2c_probe set the dev->errata flag, but omap_i2c_init cleared the flag again. Move the errata handling to i2c_probe.
Signed-off-by Tasslehoff Kjappfot <[email protected]>
Signed-off-by Shubhrajyoti D <[email protected]>
Subject looks bogus to me. You are handling I207 here.

Actually probe set ip153 then i2c init cleared so moved all the errata handling
in one place so that such errors are prevented.

WIll update the changelogs and resend

Quoted 36 lines

---
 drivers/i2c/busses/i2c-omap.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

drivers/i2c/busses/i2c-omap.c +5 -5

index d555dcd..9323201 100644
@@ -427,11 +427,6 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 	/* Take the I2C module out of reset: */
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
 
-	dev->errata = 0;
-
-	if (dev->flags & OMAP_I2C_FLAG_APPLY_ERRATA_I207)
-		dev->errata |= I2C_OMAP_ERRATA_I207;
-
 	/* Enable interrupts */
 	dev->iestate = (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
 			OMAP_I2C_IE_ARDY | OMAP_I2C_IE_NACK |

@@ -1023,6 +1018,11 @@ omap_i2c_probe(struct platform_device *pdev)
 
 	dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff;
 
+	dev->errata = 0;
+
+	if (dev->flags & OMAP_I2C_FLAG_APPLY_ERRATA_I207)
+		dev->errata |= I2C_OMAP_ERRATA_I207;
+
 	if (dev->rev <= OMAP_I2C_REV_ON_3430)
 		dev->errata |= I2C_OMAP3_1P153;

Signature · 1 line

1.7.5.4

Shubhrajyoti D

author

2012-04-19 13:28 UTC #22 lore raw

PATCH v8 9/10 I2C: OMAP: Do not set the XUDF if the underflow is not reached

In reply to Shubhrajyoti D

Currently in the 1.153 errata handling while waiting for transmitter underflow if NACK is got the XUDF flag is also set.
The flag is set after wait for the condition is over.

Cc Alexander Shishkin <[email protected]>

Acked-by Moiz Sonasath <[email protected]>

Signed-off-by Shubhrajyoti D <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/i2c/busses/i2c-omap.c +1 -1

index 9323201..d12b319 100644
@@ -732,7 +732,6 @@ static int errata_omap3_1p153(struct omap_i2c_dev *dev, u16 *stat, int *err)
 		if (*stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
 			omap_i2c_ack_stat(dev, *stat & (OMAP_I2C_STAT_XRDY |
 							OMAP_I2C_STAT_XDR));
-			*err |= OMAP_I2C_STAT_XUDF;
 			return -ETIMEDOUT;
 		}

@@ -745,6 +744,7 @@ static int errata_omap3_1p153(struct omap_i2c_dev *dev, u16 *stat, int *err)
 		return 0;
 	}
 
+	*err |= OMAP_I2C_STAT_XUDF;
 	return 0;
 }

Signature · 1 line

1.7.5.4

Wolfram Sang

2012-04-23 16:42 UTC #23 lore raw

PATCH v8 9/10 I2C: OMAP: Do not set the XUDF if the underflow is not reached

Quoted 3 lines · Shubhrajyoti D

Currently in the 1.153 errata handling while waiting for transmitter underflow if NACK is got the XUDF flag is also set.
The flag is set after wait for the condition is over.

What does XUDF mean? Please update the commit description, I can't know all terminology for all architectures.

Signature · 9 lines

Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120423/a25575d6/attachment-0001.sig>

Shubhrajyoti

author

2012-04-23 17:29 UTC #24 lore raw

PATCH v8 9/10 I2C: OMAP: Do not set the XUDF if the underflow is not reached

Quoted 7 lines · Wolfram Sang

Quoted 3 lines · Shubhrajyoti D
Currently in the 1.153 errata handling while waiting for transmitter underflow if NACK is got the XUDF flag is also set.
The flag is set after wait for the condition is over.
What does XUDF mean? Please update the commit description, I can't know all terminology for all architectures.

Transmitter underflow ,will update the commit description and resend.

Shubhrajyoti D

author

2012-04-19 13:28 UTC #25 lore raw

PATCH v8 10/10 I2C: OMAP: Rename the 1p153 to the erratum id i462

In reply to Shubhrajyoti D

The section number in the recent errata document has changed. Rename the erratum 1p153 to the unique id i462 instead, so that it is easier to reference. Also change the function name and comments to reflect the same.

Cc Jon Hunter <[email protected]>

Signed-off-by Shubhrajyoti D <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

drivers/i2c/busses/i2c-omap.c +6 -6

index d12b319..f5094b1 100644
@@ -173,7 +173,7 @@ enum {
 
 /* Errata definitions */
 #define I2C_OMAP_ERRATA_I207		(1 << 0)
-#define I2C_OMAP3_1P153			(1 << 1)
+#define I2C_OMAP_ERRATA_I462		(1 << 1)
 
 struct omap_i2c_dev {
 	struct device		*dev;

@@ -720,11 +720,11 @@ omap_i2c_omap1_isr(int this_irq, void *dev_id)
 #endif
 
 /*
- * OMAP3430 Errata 1.153: When an XRDY/XDR is hit, wait for XUDF before writing
+ * OMAP3430 Errata i462: When an XRDY/XDR is hit, wait for XUDF before writing
  * data to DATA_REG. Otherwise some data bytes can be lost while transferring
  * them from the memory to the I2C interface.
  */
-static int errata_omap3_1p153(struct omap_i2c_dev *dev, u16 *stat, int *err)
+static int errata_omap3_i462(struct omap_i2c_dev *dev, u16 *stat, int *err)
 {
 	unsigned long timeout = 10000;

@@ -883,8 +883,8 @@ complete:
 					break;
 				}
 
-				if ((dev->errata & I2C_OMAP3_1P153) &&
-				    errata_omap3_1p153(dev, &stat, &err))
+				if ((dev->errata & I2C_OMAP_ERRATA_I462) &&
+				    errata_omap3_i462(dev, &stat, &err))
 					goto complete;
 
 				omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);

@@ -1024,7 +1024,7 @@ omap_i2c_probe(struct platform_device *pdev)
 		dev->errata |= I2C_OMAP_ERRATA_I207;
 
 	if (dev->rev <= OMAP_I2C_REV_ON_3430)
-		dev->errata |= I2C_OMAP3_1P153;
+		dev->errata |= I2C_OMAP_ERRATA_I462;
 
 	if (!(dev->flags & OMAP_I2C_FLAG_NO_FIFO)) {
 		u16 s;

Signature · 1 line

1.7.5.4

Wolfram Sang

2012-04-23 17:05 UTC #26 lore raw

v8 cover I2C fixes

In reply to Shubhrajyoti D

Quoted 2 lines · Shubhrajyoti D

This patch series seperates the fixes from other
changes from [2]

I also noticed this one in my build. Can you check?

WARNING: modpost: Found 1 section mismatch(es).

Signature · 9 lines

Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120423/71648018/attachment.sig>

Shubhrajyoti

author

2012-04-23 17:41 UTC #27 lore raw

v8 cover I2C fixes

Quoted 6 lines · Wolfram Sang

Quoted 2 lines · Shubhrajyoti D
This patch series seperates the fixes from other
changes from [2]
I also noticed this one in my build. Can you check?
WARNING: modpost: Found 1 section mismatch(es).

I checked on the old branch didnt see the error anyways will rebase to the latest head
and recheck .

Thanks,