[PATCH] OMAP4: I2C: Enable the wakeup in I2C_WE

[PATCH] OMAP4: I2C: Enable the wakeup in I2C_WE

[Shubhrajyoti D]

Currently for OMAP4 the I2C_WE is not programmed.
This patch enables the programming for OMAP4.

Reported-by: Santosh Shilimkar <santosh.shilimkar-l0cyMroinI0@public.gmane.org>
Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
---
TODO:
Currently all the wakeup sources are enabled.
There is scope of optimising the same. Will revisit it.
Rebased on Kevin's wip/i2c branch
Tested on OMAP4430.

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

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index d05efe7..18cc0af 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -313,9 +313,8 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 		 * REVISIT: Some wakeup sources might not be needed.
 		 */
 		dev->westate = OMAP_I2C_WE_ALL;
-		if (dev->rev < OMAP_I2C_REV_ON_3530_4430)
-			omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
-							dev->westate);
+		omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
+						dev->westate);
 	}
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
 
-- 
1.7.1

Re: [PATCH] OMAP4: I2C: Enable the wakeup in I2C_WE

[Felipe Balbi]

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

Hi,

On Fri, Jul 29, 2011 at 05:18:42PM +0530, Shubhrajyoti D wrote:
> Currently for OMAP4 the I2C_WE is not programmed.
> This patch enables the programming for OMAP4.
> 
> Reported-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
> TODO:
> Currently all the wakeup sources are enabled.
> There is scope of optimising the same. Will revisit it.
> Rebased on Kevin's wip/i2c branch
> Tested on OMAP4430.
> 
>  drivers/i2c/busses/i2c-omap.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index d05efe7..18cc0af 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -313,9 +313,8 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
>  		 * REVISIT: Some wakeup sources might not be needed.
>  		 */
>  		dev->westate = OMAP_I2C_WE_ALL;
> -		if (dev->rev < OMAP_I2C_REV_ON_3530_4430)
> -			omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
> -							dev->westate);
> +		omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
> +						dev->westate);

this is also enabling for 3530, have you tested there too ?? On the
other hand, this looks like it's fixing a bogus change on commit
a3a7acbcc3df4e9ecc12aa1fc435534d74ebbdf4 (I2C: OMAP2+: address confused
probed version naming) specifically on the hunk below [1]:

@@ -379,7 +379,9 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
                         * REVISIT: Some wkup sources might not be needed.
                         */
                        dev->westate = OMAP_I2C_WE_ALL;
-                       omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate);
+                       if (dev->rev < OMAP_I2C_REV_ON_3530_4430)
+                               omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
+                                                               dev->westate);
                }
        }
        omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);

if that's the case, you should either update your commit log stating
that this is a fix to a bug introduced by that commit, or fold this
patch in the same. You should also Cc the patch author to clarify why
the dev->rev check was added.

Andy, can you clarify why you added the revision check which didn't
exist before ?

[1] http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-pm.git;a=commitdiff;h=a3a7acbcc3df4e9ecc12aa1fc435534d74ebbdf4

-- 
balbi

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

Re: [PATCH] OMAP4: I2C: Enable the wakeup in I2C_WE

["Andy Green (林安廸)"]

On 07/29/2011 01:07 PM, Somebody in the thread at some point said:

Hi -

> -                       omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate);
> +                       if (dev->rev<  OMAP_I2C_REV_ON_3530_4430)
> +                               omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
> +                                                               dev->westate);

> Andy, can you clarify why you added the revision check which didn't
> exist before ?
>
> [1] http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-pm.git;a=commitdiff;h=a3a7acbcc3df4e9ecc12aa1fc435534d74ebbdf4
>

At the time I wrote the patches back in March, the code there was 
different: there was a pre-extant test avoiding that line on 4430, and 
the patch is simply converting it to the new scheme.  You can see it here:

http://permalink.gmane.org/gmane.linux.ports.arm.omap/54940

@@ -379,7 +379,7 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
  			 * REVISIT: Some wkup sources might not be needed.
  			 */
  			dev->westate = OMAP_I2C_WE_ALL;
-			if (dev->rev < OMAP_I2C_REV_ON_4430)
+			if (dev->rev < OMAP_I2C_REV_ON_3530_4430)
  				omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
  								dev->westate);
  		}

I guess since March and before this got committed for 3.1, someone got a 
patch in first removing the test, so when my patchset was uplevelled for 
commit against 3.1-rc this conflict was dealt with by re-introducing the 
test.

Long story short, it's there from me as a mechanical 1:1 renaming action 
as part of the fix that 3530 and 4430 (different) IPs return the same 
rev number.  Despite how it now looks I didn't add it, so if 
Shubhrajyoti has reasons to think it should be gone again I have nothing 
against that at all.

-Andy

Re: [PATCH] OMAP4: I2C: Enable the wakeup in I2C_WE

[Felipe Balbi]

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

Hi,

On Fri, Jul 29, 2011 at 01:28:12PM +0100, "Andy Green (林安廸)" wrote:
> On 07/29/2011 01:07 PM, Somebody in the thread at some point said:
> 
> Hi -
> 
> >-                       omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate);
> >+                       if (dev->rev<  OMAP_I2C_REV_ON_3530_4430)
> >+                               omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
> >+                                                               dev->westate);
> 
> >Andy, can you clarify why you added the revision check which didn't
> >exist before ?
> >
> >[1] http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-pm.git;a=commitdiff;h=a3a7acbcc3df4e9ecc12aa1fc435534d74ebbdf4
> >
> 
> At the time I wrote the patches back in March, the code there was
> different: there was a pre-extant test avoiding that line on 4430,
> and the patch is simply converting it to the new scheme.  You can see
> it here:
> 
> http://permalink.gmane.org/gmane.linux.ports.arm.omap/54940
> 
> @@ -379,7 +379,7 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
>  			 * REVISIT: Some wkup sources might not be needed.
>  			 */
>  			dev->westate = OMAP_I2C_WE_ALL;
> -			if (dev->rev < OMAP_I2C_REV_ON_4430)
> +			if (dev->rev < OMAP_I2C_REV_ON_3530_4430)
>  				omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
>  								dev->westate);
>  		}
> 
> I guess since March and before this got committed for 3.1, someone
> got a patch in first removing the test, so when my patchset was
> uplevelled for commit against 3.1-rc this conflict was dealt with by
> re-introducing the test.
> 
> Long story short, it's there from me as a mechanical 1:1 renaming
> action as part of the fix that 3530 and 4430 (different) IPs return
> the same rev number.  Despite how it now looks I didn't add it, so if
> Shubhrajyoti has reasons to think it should be gone again I have
> nothing against that at all.

yeah, looks like a bad conflict resolution. Shubhrajyoti, care to respin
the patch and update commit log stating that it is fixing a bad conflict
resolution or something ?

-- 
balbi

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

Re: [PATCH] OMAP4: I2C: Enable the wakeup in I2C_WE

[Shubhrajyoti]

On Friday 29 July 2011 06:07 PM, Felipe Balbi wrote:
> Hi,
>
> On Fri, Jul 29, 2011 at 01:28:12PM +0100, "Andy Green (林安廸)" wrote:
>> On 07/29/2011 01:07 PM, Somebody in the thread at some point said:
>>
>> Hi -
>>
>>> -                       omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate);
>>> +                       if (dev->rev<   OMAP_I2C_REV_ON_3530_4430)
>>> +                               omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
>>> +                                                               dev->westate);
>>> Andy, can you clarify why you added the revision check which didn't
>>> exist before ?
>>>
>>> [1] http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-pm.git;a=commitdiff;h=a3a7acbcc3df4e9ecc12aa1fc435534d74ebbdf4
>>>
>> At the time I wrote the patches back in March, the code there was
>> different: there was a pre-extant test avoiding that line on 4430,
>> and the patch is simply converting it to the new scheme.  You can see
>> it here:
>>
>> http://permalink.gmane.org/gmane.linux.ports.arm.omap/54940
>>
>> @@ -379,7 +379,7 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
>>   			 * REVISIT: Some wkup sources might not be needed.
>>   			 */
>>   			dev->westate = OMAP_I2C_WE_ALL;
>> -			if (dev->rev<  OMAP_I2C_REV_ON_4430)
>> +			if (dev->rev<  OMAP_I2C_REV_ON_3530_4430)
>>   				omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
>>   								dev->westate);
>>   		}
>>
>> I guess since March and before this got committed for 3.1, someone
>> got a patch in first removing the test, so when my patchset was
>> uplevelled for commit against 3.1-rc this conflict was dealt with by
>> re-introducing the test.
>>
>> Long story short, it's there from me as a mechanical 1:1 renaming
>> action as part of the fix that 3530 and 4430 (different) IPs return
>> the same rev number.  Despite how it now looks I didn't add it, so if
>> Shubhrajyoti has reasons to think it should be gone again I have
>> nothing against that at all.
> yeah, looks like a bad conflict resolution. Shubhrajyoti, care to respin
> the patch and update commit log stating that it is fixing a bad conflict
> resolution or something ?
I wasn't aware of the conflict resolution part. Actually came across this
piece of code as per the discussion on the reset implementation patch 
will update
the changelogs.
How about,

From: Shubhrajyoti D<shubhrajyoti@ti.com>

Currently for OMAP4 the I2C_WE is not programmed.
This patch enables the programming for OMAP4.

Fixes a conflict resolution of Andy's patches.

Reported-by: Santosh Shilimkar<santosh.shilimkar@ti.com>
Signed-off-by: Shubhrajyoti D<shubhrajyoti@ti.com>
---
TODO:
Currently all the wakeup sources are enabled.
There is scope of optimising the same. Will revisit it.
Rebased on Kevin's wip/i2c branch
Tested on OMAP4430.

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

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index d05efe7..18cc0af 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -313,9 +313,8 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
  		 * REVISIT: Some wakeup sources might not be needed.
  		 */
  		dev->westate = OMAP_I2C_WE_ALL;
-		if (dev->rev<  OMAP_I2C_REV_ON_3530_4430)
-			omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
-							dev->westate);
+		omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
+						dev->westate);
  	}
  	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);

-- 1.7.1


--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] OMAP4: I2C: Enable the wakeup in I2C_WE

[Felipe Balbi]

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

Hi,

On Fri, Jul 29, 2011 at 07:11:34PM +0530, Shubhrajyoti wrote:
> On Friday 29 July 2011 06:07 PM, Felipe Balbi wrote:
> >Hi,
> >
> >On Fri, Jul 29, 2011 at 01:28:12PM +0100, "Andy Green (林安廸)" wrote:
> >>On 07/29/2011 01:07 PM, Somebody in the thread at some point said:
> >>
> >>Hi -
> >>
> >>>-                       omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate);
> >>>+                       if (dev->rev<   OMAP_I2C_REV_ON_3530_4430)
> >>>+                               omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
> >>>+                                                               dev->westate);
> >>>Andy, can you clarify why you added the revision check which didn't
> >>>exist before ?
> >>>
> >>>[1] http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-pm.git;a=commitdiff;h=a3a7acbcc3df4e9ecc12aa1fc435534d74ebbdf4
> >>>
> >>At the time I wrote the patches back in March, the code there was
> >>different: there was a pre-extant test avoiding that line on 4430,
> >>and the patch is simply converting it to the new scheme.  You can see
> >>it here:
> >>
> >>http://permalink.gmane.org/gmane.linux.ports.arm.omap/54940
> >>
> >>@@ -379,7 +379,7 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
> >>  			 * REVISIT: Some wkup sources might not be needed.
> >>  			 */
> >>  			dev->westate = OMAP_I2C_WE_ALL;
> >>-			if (dev->rev<  OMAP_I2C_REV_ON_4430)
> >>+			if (dev->rev<  OMAP_I2C_REV_ON_3530_4430)
> >>  				omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
> >>  								dev->westate);
> >>  		}
> >>
> >>I guess since March and before this got committed for 3.1, someone
> >>got a patch in first removing the test, so when my patchset was
> >>uplevelled for commit against 3.1-rc this conflict was dealt with by
> >>re-introducing the test.
> >>
> >>Long story short, it's there from me as a mechanical 1:1 renaming
> >>action as part of the fix that 3530 and 4430 (different) IPs return
> >>the same rev number.  Despite how it now looks I didn't add it, so if
> >>Shubhrajyoti has reasons to think it should be gone again I have
> >>nothing against that at all.
> >yeah, looks like a bad conflict resolution. Shubhrajyoti, care to respin
> >the patch and update commit log stating that it is fixing a bad conflict
> >resolution or something ?
> I wasn't aware of the conflict resolution part. Actually came across this
> piece of code as per the discussion on the reset implementation patch
> will update
> the changelogs.
> How about,
> 
> From: Shubhrajyoti D<shubhrajyoti-l0cyMroinI0@public.gmane.org>
> 
> Currently for OMAP4 the I2C_WE is not programmed.
> This patch enables the programming for OMAP4.
> 
> Fixes a conflict resolution of Andy's patches.

I think you need to be a bit more verbose here ;-) Describe what
happened and point to the commit number and mailing list archives for
references. Imagine someone else reads this commit half a year from now,
will s/he have enough information to understand the background of this
patch ?

-- 
balbi

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

Re: [PATCH] OMAP4: I2C: Enable the wakeup in I2C_WE

[Felipe Balbi]

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

Hi,

On Fri, Jul 29, 2011 at 07:33:39PM +0530, Datta, Shubhrajyoti wrote:
>    On Fri, Jul 29, 2011 at 7:11 PM, Shubhrajyoti <[1]shubhrajyoti-l0cyMroinI0@public.gmane.org>
>    wrote:
> 
>      On Friday 29 July 2011 06:07 PM, Felipe Balbi wrote:
> 
>        Hi,
> 
>        On Fri, Jul 29, 2011 at 01:28:12PM +0100, "Andy Green (林安廸)" wrote:
> 
>          On 07/29/2011 01:07 PM, Somebody in the thread at some point said:
> 
>          Hi -
> 
>            -                       omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
>            dev->westate);
>            +                       if (dev->rev<   OMAP_I2C_REV_ON_3530_4430)
>            +                               omap_i2c_write_reg(dev,
>            OMAP_I2C_WE_REG,
>            +                                                              
>            dev->westate);
>            Andy, can you clarify why you added the revision check which
>            didn't
>            exist before ?
> 
>            [1]
>            [2]http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-pm.git;a=commitdiff;h=a3a7acbcc3df4e9ecc12aa1fc435534d74ebbdf4
> 
>          At the time I wrote the patches back in March, the code there was
>          different: there was a pre-extant test avoiding that line on 4430,
>          and the patch is simply converting it to the new scheme.  You can
>          see
>          it here:
> 
>          [3]http://permalink.gmane.org/gmane.linux.ports.arm.omap/54940
> 
>          @@ -379,7 +379,7 @@ static int omap_i2c_init(struct omap_i2c_dev
>          *dev)
>                                  * REVISIT: Some wkup sources might not be
>          needed.
>                                  */
>                                 dev->westate = OMAP_I2C_WE_ALL;
>          -                       if (dev->rev<  OMAP_I2C_REV_ON_4430)
>          +                       if (dev->rev<  OMAP_I2C_REV_ON_3530_4430)
>                                         omap_i2c_write_reg(dev,
>          OMAP_I2C_WE_REG,
>                                                                       
>           dev->westate);
>                         }
> 
>          I guess since March and before this got committed for 3.1, someone
>          got a patch in first removing the test, so when my patchset was
>          uplevelled for commit against 3.1-rc this conflict was dealt with by
>          re-introducing the test.
> 
>          Long story short, it's there from me as a mechanical 1:1 renaming
>          action as part of the fix that 3530 and 4430 (different) IPs return
>          the same rev number.  Despite how it now looks I didn't add it, so
>          if
>          Shubhrajyoti has reasons to think it should be gone again I have
>          nothing against that at all.
> 
>        yeah, looks like a bad conflict resolution. Shubhrajyoti, care to
>        respin
>        the patch and update commit log stating that it is fixing a bad
>        conflict
>        resolution or something ?
> 
>      I wasn't aware of the conflict resolution part. Actually came across
>      this
>      piece of code as per the discussion on the reset implementation patch
>      will update
>      the changelogs.
>      How about,
> 
>    Earlier mail got corrupted resending

this is much worse. What mail client are you using ? Maybe there are
some tips on Documentation/email-clients.txt

-- 
balbi

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