[PATCHv8 00/10] I2C fixes

[PATCHv8 00/10] I2C fixes

[Shubhrajyoti D]

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

-- 
1.7.5.4

[PATCHv8 01/10] I2C: OMAP: make omap_i2c_unidle/idle functions depend on CONFIG_PM_RUNTIME

[Shubhrajyoti D]

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 <shubhrajyoti@ti.com>
---
 drivers/i2c/busses/i2c-omap.c |   75 +++++++++++++++++-----------------------
 1 files changed, 32 insertions(+), 43 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 801df60..4f4188d 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -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;
 }
-- 
1.7.5.4

[PATCHv8 02/10] I2C: OMAP: Fix the mismatch of pm_runtime enable and disable

[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 <khilman-l0cyMroinI0@public.gmane.org>
Cc: Rajendra Nayak <rnayak-l0cyMroinI0@public.gmane.org>
Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 4f4188d..c851672 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -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);
-- 
1.7.5.4

Re: [PATCHv8 02/10] I2C: OMAP: Fix the mismatch of pm_runtime enable and disable

[Wolfram Sang]

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

On Thu, Apr 19, 2012 at 06:58:13PM +0530, Shubhrajyoti D wrote:
> 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"?

> 
> Cc: Kevin Hilman <khilman@ti.com>
> Cc: Rajendra Nayak <rnayak@ti.com>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
>  drivers/i2c/busses/i2c-omap.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 4f4188d..c851672 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -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);
> -- 
> 1.7.5.4
> 

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCHv8 02/10] I2C: OMAP: Fix the mismatch of pm_runtime enable and disable

[Datta, Shubhrajyoti]

Thanks for review,

On Mon, Apr 23, 2012 at 9:13 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> On Thu, Apr 19, 2012 at 06:58:13PM +0530, Shubhrajyoti D wrote:
>> 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.

>From 1f6d8671d15b16de8840f0ff23d77d613a03e876 Mon Sep 17 00:00:00 2001
From: Shubhrajyoti D <shubhrajyoti@ti.com>
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 <khilman@ti.com>
Cc: Rajendra Nayak <rnayak@ti.com>
Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
---
 drivers/i2c/busses/i2c-omap.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 4f4188d..c851672 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -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);
-- 
1.7.5.4

[PATCHv8 03/10] I2C: OMAP: Fix the interrupt clearing in OMAP4

[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 <vikram.pandita-l0cyMroinI0@public.gmane.org>.

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 <vikram.pandita-l0cyMroinI0@public.gmane.org>
Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index c851672..bf07ffd 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -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 */
-- 
1.7.5.4

[PATCHv8 04/10] I2C: OMAP: Fix the error handling

[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 <shubhrajyoti-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index bf07ffd..1777d79 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -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:
-- 
1.7.5.4

Re: [PATCHv8 04/10] I2C: OMAP: Fix the error handling

[Wolfram Sang]

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


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

On Thu, Apr 19, 2012 at 06:58:15PM +0530, Shubhrajyoti D wrote:
> Currently in probe
>       pm_runtime_put(dev->dev);

Indentation wrong.

> 
> ...
>         /* 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.

Drop "Attempting".

> 
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
>  drivers/i2c/busses/i2c-omap.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index bf07ffd..1777d79 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -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:
> -- 
> 1.7.5.4
> 

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCHv8 06/10] I2C: OMAP: Fix the crash in i2c remove

[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: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index fec8d5c..44e8cfa 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -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);
-- 
1.7.5.4

Re: [PATCHv8 06/10] I2C: OMAP: Fix the crash in i2c remove

[Wolfram Sang]

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

On Thu, Apr 19, 2012 at 06:58:17PM +0530, Shubhrajyoti D wrote:
>     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: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>

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?

> ---
>  drivers/i2c/busses/i2c-omap.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index fec8d5c..44e8cfa 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -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);
> -- 
> 1.7.5.4
> 

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCHv8 06/10] I2C: OMAP: Fix the crash in i2c remove

[Shubhrajyoti]

On Monday 23 April 2012 10:19 PM, Wolfram Sang wrote:
>>     [  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: <stable@vger.kernel.org>
>> > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> 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?
> 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.

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

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

Re: [PATCHv8 06/10] I2C: OMAP: Fix the crash in i2c remove

[Wolfram Sang]

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

On Tue, Apr 24, 2012 at 11:44:15PM +0530, Shubhrajyoti wrote:
> On Monday 23 April 2012 10:19 PM, Wolfram Sang wrote:
> >>     [  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: <stable@vger.kernel.org>
> >> > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> > 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.

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

> Will wait for the pm experts to comment.

Yup.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCHv8 06/10] I2C: OMAP: Fix the crash in i2c remove

[Shubhrajyoti]

On Tuesday 24 April 2012 11:48 PM, Wolfram Sang wrote:
> On Tue, Apr 24, 2012 at 11:44:15PM +0530, Shubhrajyoti wrote:
>> On Monday 23 April 2012 10:19 PM, Wolfram Sang wrote:
>>>>     [  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: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
>>>>> Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
>>> 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.
>
>>> 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".
>
>> 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.

Re: [PATCHv8 06/10] I2C: OMAP: Fix the crash in i2c remove

[Shubhrajyoti]

On Thursday 26 April 2012 11:58 AM, Shubhrajyoti wrote:
> On Tuesday 24 April 2012 11:48 PM, Wolfram Sang wrote:
>> On Tue, Apr 24, 2012 at 11:44:15PM +0530, Shubhrajyoti wrote:
>>> On Monday 23 April 2012 10:19 PM, Wolfram Sang wrote:
>>>>>     [  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: <stable@vger.kernel.org>
>>>>>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
>>>> 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.
>>
>>>> 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".
>>
>>> 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,
>
>

[PATCHv8 07/10] I2C: OMAP: Handle error check for pm runtime

[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 <shubhrajyoti-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 44e8cfa..d555dcd 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -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);
-- 
1.7.5.4

Re: [PATCHv8 07/10] I2C: OMAP: Handle error check for pm runtime

[Wolfram Sang]

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

On Thu, Apr 19, 2012 at 06:58:18PM +0530, Shubhrajyoti D wrote:
> 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).

> 
> Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-omap.c |   19 ++++++++++++++++---
>  1 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 44e8cfa..d555dcd 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -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;
> +	}

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.

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

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?

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

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCHv8 07/10] I2C: OMAP: Handle error check for pm runtime

[Shubhrajyoti]

On Monday 23 April 2012 09:50 PM, Wolfram Sang wrote:
> On Thu, Apr 19, 2012 at 06:58:18PM +0530, Shubhrajyoti D wrote:
>> 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
>
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
>> ---
>>  drivers/i2c/busses/i2c-omap.c |   19 ++++++++++++++++---
>>  1 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>> index 44e8cfa..d555dcd 100644
>> --- a/drivers/i2c/busses/i2c-omap.c
>> +++ b/drivers/i2c/busses/i2c-omap.c
>> @@ -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;
>> +	}
> 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.
Ops  will correct, thanks.
>
>> @@ -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;
>> +	}
> 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.
>
>>  	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
>>  	pm_runtime_put(&pdev->dev);
>>  	pm_runtime_disable(&pdev->dev);
>> -- 
>> 1.7.5.4
>>
> Thanks,
>
>    Wolfram
>

[PATCHv8 05/10] I2C: OMAP: Don't check if wait_for_completion_timeout() returns less than zero

[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 <jon-hunter@ti.com>
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 <jon-hunter@ti.com>
Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
---
 drivers/i2c/busses/i2c-omap.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 1777d79..fec8d5c 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -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;
-- 
1.7.5.4

Re: [PATCHv8 05/10] I2C: OMAP: Don't check if wait_for_completion_timeout() returns less than zero

[Wolfram Sang]

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

On Thu, Apr 19, 2012 at 06:58:16PM +0530, Shubhrajyoti D wrote:
> 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 <jon-hunter-l0cyMroinI0@public.gmane.org>
> 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.

> 
> Cc : Jon Hunter <jon-hunter-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-omap.c |   10 ++++------
>  1 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 1777d79..fec8d5c 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -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;
> -- 
> 1.7.5.4
> 

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCHv8 08/10] I2C: OMAP: fix missing handling of errata I2C_OMAP3_1P153

[Shubhrajyoti D]

From: Tasslehoff Kjappfot <tasskjapp@gmail.com>

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 <tasskjapp@gmail.com>
Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
---
 drivers/i2c/busses/i2c-omap.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index d555dcd..9323201 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -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;
 
-- 
1.7.5.4

Re: [PATCHv8 08/10] I2C: OMAP: fix missing handling of errata I2C_OMAP3_1P153

[Wolfram Sang]

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

On Thu, Apr 19, 2012 at 06:58:19PM +0530, Shubhrajyoti D wrote:
> From: Tasslehoff Kjappfot <tasskjapp@gmail.com>
> 
> 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 <tasskjapp@gmail.com>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>

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

> ---
>  drivers/i2c/busses/i2c-omap.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index d555dcd..9323201 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -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;
>  
> -- 
> 1.7.5.4
> 

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCHv8 08/10] I2C: OMAP: fix missing handling of errata I2C_OMAP3_1P153

[Shubhrajyoti]

On Monday 23 April 2012 10:13 PM, Wolfram Sang wrote:
> On Thu, Apr 19, 2012 at 06:58:19PM +0530, Shubhrajyoti D wrote:
>> From: Tasslehoff Kjappfot <tasskjapp@gmail.com>
>>
>> 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 <tasskjapp@gmail.com>
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> 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
>
>> ---
>>  drivers/i2c/busses/i2c-omap.c |   10 +++++-----
>>  1 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>> index d555dcd..9323201 100644
>> --- a/drivers/i2c/busses/i2c-omap.c
>> +++ b/drivers/i2c/busses/i2c-omap.c
>> @@ -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;
>>  
>> -- 
>> 1.7.5.4
>>

[PATCHv8 09/10] I2C: OMAP: Do not set the XUDF if the underflow is not reached

[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 <virtuoso@slind.org>
Acked-by: Moiz Sonasath <m-sonasath@ti.com>
Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
---
 drivers/i2c/busses/i2c-omap.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 9323201..d12b319 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -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;
 }
 
-- 
1.7.5.4

Re: [PATCHv8 09/10] I2C: OMAP: Do not set the XUDF if the underflow is not reached

[Wolfram Sang]

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

On Thu, Apr 19, 2012 at 06:58:20PM +0530, Shubhrajyoti D wrote:
> 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.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCHv8 09/10] I2C: OMAP: Do not set the XUDF if the underflow is not reached

[Shubhrajyoti]

On Monday 23 April 2012 10:12 PM, Wolfram Sang wrote:
> On Thu, Apr 19, 2012 at 06:58:20PM +0530, Shubhrajyoti D wrote:
>> 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.

[PATCHv8 10/10] I2C: OMAP: Rename the 1p153 to the erratum id i462

[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 <jon-hunter@ti.com>
Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
---
 drivers/i2c/busses/i2c-omap.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index d12b319..f5094b1 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -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;
-- 
1.7.5.4

Re: [PATCHv8 00/10] I2C fixes

[Wolfram Sang]

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

On Thu, Apr 19, 2012 at 06:58:11PM +0530, Shubhrajyoti D wrote:
> 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).

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCHv8 00/10] I2C fixes

[Shubhrajyoti]

On Monday 23 April 2012 10:35 PM, Wolfram Sang wrote:
> On Thu, Apr 19, 2012 at 06:58:11PM +0530, Shubhrajyoti D wrote:
>> 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,