Patchwork [PATCHv8,00/10] I2C fixes

login
register
mail settings
Submitter Datta, Shubhrajyoti
Date April 19, 2012, 1:28 p.m.
Message ID <1334842101-20670-1-git-send-email-shubhrajyoti@ti.com>
Download mbox
Permalink /patch/153753/
State New
Headers show

Pull-request

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

Comments

Datta, Shubhrajyoti - April 19, 2012, 1:28 p.m.
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(-)
Wolfram Sang - April 23, 2012, 3:43 p.m.
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
>
Wolfram Sang - April 23, 2012, 4:20 p.m.
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@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.

> @@ -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
Wolfram Sang - April 23, 2012, 4:42 p.m.
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.
Wolfram Sang - April 23, 2012, 4:43 p.m.
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
>
Wolfram Sang - April 23, 2012, 4:49 p.m.
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@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, 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
>
Wolfram Sang - April 23, 2012, 5:05 p.m.
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).
Wolfram Sang - April 23, 2012, 5:10 p.m.
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
>
Wolfram Sang - April 23, 2012, 5:11 p.m.
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@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

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

> 
> 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
>
Datta, Shubhrajyoti - April 23, 2012, 5:26 p.m.
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
>
Datta, Shubhrajyoti - April 23, 2012, 5:29 p.m.
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.
Datta, Shubhrajyoti - April 23, 2012, 5:35 p.m.
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
>>
Datta, Shubhrajyoti - April 23, 2012, 5:41 p.m.
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,
Datta, Shubhrajyoti - April 24, 2012, 6:14 p.m.
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(-)
>> >
Wolfram Sang - April 24, 2012, 6:18 p.m.
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.
Datta, Shubhrajyoti - April 26, 2012, 6:28 a.m.
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.
Datta, Shubhrajyoti - April 26, 2012, 6:43 a.m.
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,
>
>