Patchwork [PATCHv8,00/13] I2C cleanups

login
register
mail settings
Submitter Datta, Shubhrajyoti
Date June 18, 2012, 2:30 p.m.
Message ID <1340029828-20751-1-git-send-email-shubhrajyoti@ti.com>
Download mbox
Permalink /patch/165473/
State New
Headers show

Pull-request

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

Comments

Datta, Shubhrajyoti - June 18, 2012, 2:30 p.m.
The patch series does the following

- I2C register restore only if context if the context is lost
- Bus busy recovery mechanism.
- the reset is not done in init.
- Adds a patch to use devm_* functions
- Adds a pdata function pointer to do context save restore
- Split the omap_i2c_isr to increase readability
- Make the i2c use SET_RUNTIME_PM_OPS
- Use INIT_COMPLETION instead of init_completion
- Dropping a cleanup and taking few patchs Felipe's series as it may
 not be needed.

This applies on Wolfram's i2c-embedded/for-next branch.
 
Functional testing on omap4sdp and omap3sdp. 

Previous discurssions 
http://www.spinics.net/lists/linux-i2c/msg07748.html

This series mainly is the cleanups rebased on
i2c-embedded/for-next branch.


The following changes since commit 0f009a914b40be8786fa67b1f4345cacc263b48c:

  i2c: tegra: make all resource allocation through devm_* (2012-06-13 16:01:38 +0200)

are available in the git repository at:
  git://gitorious.org/linus-tree/linus-tree.git for_next/i2c_omap

Felipe Balbi (4):
      I2C: OMAP: simplify num_bytes handling
      I2C: OMAP: decrease indentation level on data handling
      I2C: OMAP: add blank lines
      I2C: OMAP: simplify omap_i2c_ack_stat()

Jon Hunter (1):
      I2C: OMAP: Correct I2C revision for OMAP3

Shubhrajyoti D (7):
      I2C: OMAP: I2C register restore only if context is lost
      I2C: OMAP: Remove the definition of SYSS_RESETDONE_MASK
      I2C: OMAP: Remove reset at init
      I2C: OMAP: Optimise the remove code
      I2C: OMAP: use devm_* functions
      I2C: OMAP: Use SET_RUNTIME_PM_OPS
      I2C: OMAP: Do not initialise the completion everytime

Vikram Pandita (1):
      I2C: OMAP: Recover from Bus Busy condition

 arch/arm/plat-omap/i2c.c      |    3 +
 drivers/i2c/busses/i2c-omap.c |  261 +++++++++++++++++++++++-----------------
 include/linux/i2c-omap.h      |    1 +
 3 files changed, 154 insertions(+), 111 deletions(-)
Wolfram Sang - June 18, 2012, 3:22 p.m.
On Mon, Jun 18, 2012 at 08:00:25PM +0530, Shubhrajyoti D wrote:
> From: Felipe Balbi <balbi@ti.com>
> 
> trivial patch, no functional changes

Wrong. This patch does change some behaviour, are you aware of that?

So, please check if the side-effect is affectong the code and adapt the
commit message, if everything is okay.

> 
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> Reviewed-by : Santosh Shilimkar <santosh.shilimkar@ti.com>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
>  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 e24eb1f..080193a 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -844,8 +844,7 @@ complete:
>  							OMAP_I2C_BUFSTAT_REG)
>  							>> 8) & 0x3F;
>  			}
> -			while (num_bytes) {
> -				num_bytes--;
> +			while (num_bytes--) {
>  				w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
>  				if (dev->buf_len) {
>  					*dev->buf++ = w;
> @@ -887,8 +886,7 @@ complete:
>  							OMAP_I2C_BUFSTAT_REG)
>  							& 0x3F;
>  			}
> -			while (num_bytes) {
> -				num_bytes--;
> +			while (num_bytes--) {
>  				w = 0;
>  				if (dev->buf_len) {
>  					w = *dev->buf++;
> -- 
> 1.7.5.4
>
Wolfram Sang - June 18, 2012, 3:25 p.m.
On Mon, Jun 18, 2012 at 08:00:26PM +0530, Shubhrajyoti D wrote:
> From: Felipe Balbi <balbi@ti.com>
> 
> trivial patch, no functional changes.

This patch seems to be correct, but it is not trivial. In fact, it is
pretty easy to miss a "!" here which can cause subtle bugs.

> 
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> Reviewed-by : Santosh Shilimkar <santosh.shilimkar@ti.com>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
>  drivers/i2c/busses/i2c-omap.c |   63 ++++++++++++++++++++---------------------
>  1 files changed, 31 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 080193a..0e0ab8f 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -845,22 +845,7 @@ complete:
>  							>> 8) & 0x3F;
>  			}
>  			while (num_bytes--) {
> -				w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
> -				if (dev->buf_len) {
> -					*dev->buf++ = w;
> -					dev->buf_len--;
> -					/*
> -					 * Data reg in 2430, omap3 and
> -					 * omap4 is 8 bit wide
> -					 */
> -					if (dev->flags &
> -						 OMAP_I2C_FLAG_16BIT_DATA_REG) {
> -						if (dev->buf_len) {
> -							*dev->buf++ = w >> 8;
> -							dev->buf_len--;
> -						}
> -					}
> -				} else {
> +				if (!dev->buf_len) {
>  					if (stat & OMAP_I2C_STAT_RRDY)
>  						dev_err(dev->dev,
>  							"RRDY IRQ while no data"
> @@ -871,6 +856,21 @@ complete:
>  								" requested\n");
>  					break;
>  				}
> +
> +				w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
> +				*dev->buf++ = w;
> +				dev->buf_len--;
> +				/*
> +				 * Data reg in 2430, omap3 and
> +				 * omap4 is 8 bit wide
> +				 */
> +				if (dev->flags &
> +						OMAP_I2C_FLAG_16BIT_DATA_REG) {
> +					if (dev->buf_len) {
> +						*dev->buf++ = w >> 8;
> +						dev->buf_len--;
> +					}
> +				}
>  			}
>  			omap_i2c_ack_stat(dev,
>  				stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR));
> @@ -887,22 +887,7 @@ complete:
>  							& 0x3F;
>  			}
>  			while (num_bytes--) {
> -				w = 0;
> -				if (dev->buf_len) {
> -					w = *dev->buf++;
> -					dev->buf_len--;
> -					/*
> -					 * Data reg in 2430, omap3 and
> -					 * omap4 is 8 bit wide
> -					 */
> -					if (dev->flags &
> -						 OMAP_I2C_FLAG_16BIT_DATA_REG) {
> -						if (dev->buf_len) {
> -							w |= *dev->buf++ << 8;
> -							dev->buf_len--;
> -						}
> -					}
> -				} else {
> +				if (!dev->buf_len) {
>  					if (stat & OMAP_I2C_STAT_XRDY)
>  						dev_err(dev->dev,
>  							"XRDY IRQ while no "
> @@ -914,6 +899,20 @@ complete:
>  					break;
>  				}
>  
> +				w = *dev->buf++;
> +				dev->buf_len--;
> +				/*
> +				 * Data reg in 2430, omap3 and
> +				 * omap4 is 8 bit wide
> +				 */
> +				if (dev->flags &
> +						OMAP_I2C_FLAG_16BIT_DATA_REG) {
> +					if (dev->buf_len) {
> +						w |= *dev->buf++ << 8;
> +						dev->buf_len--;
> +					}
> +				}
> +
>  				if ((dev->errata & I2C_OMAP_ERRATA_I462) &&
>  				    errata_omap3_i462(dev, &stat, &err))
>  					goto complete;
> -- 
> 1.7.5.4
>
Wolfram Sang - June 18, 2012, 3:30 p.m.
On Mon, Jun 18, 2012 at 08:00:28PM +0530, Shubhrajyoti D wrote:
> From: Felipe Balbi <balbi@ti.com>
> 
> stat & BIT(1) is the same as BIT(1),

Not true. I'd guess you are missing some context in the patch
description.

> so let's
> simplify things a bit by removing "stat &" from
> all omap_i2c_ack_stat() calls.
> 
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> Reviewed-by : Santosh Shilimkar <santosh.shilimkar@ti.com>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
>  drivers/i2c/busses/i2c-omap.c |   19 ++++++++++---------
>  1 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 6a79089..bac6305 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -763,7 +763,7 @@ static int errata_omap3_i462(struct omap_i2c_dev *dev, u16 *stat, int *err)
>  
>  	while (--timeout && !(*stat & OMAP_I2C_STAT_XUDF)) {
>  		if (*stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
> -			omap_i2c_ack_stat(dev, *stat & (OMAP_I2C_STAT_XRDY |
> +			omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_XRDY |
>  							OMAP_I2C_STAT_XDR));
>  			return -ETIMEDOUT;
>  		}
> @@ -824,10 +824,11 @@ complete:
>  		 */
>  		if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK |
>  					OMAP_I2C_STAT_AL)) {
> -			omap_i2c_ack_stat(dev, stat &
> -				(OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR |
> -				OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR |
> -				OMAP_I2C_STAT_ARDY));
> +			omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_RRDY |
> +						OMAP_I2C_STAT_RDR |
> +						OMAP_I2C_STAT_XRDY |
> +						OMAP_I2C_STAT_XDR |
> +						OMAP_I2C_STAT_ARDY));
>  			omap_i2c_complete_cmd(dev, err);
>  			return IRQ_HANDLED;
>  		}
> @@ -874,8 +875,8 @@ complete:
>  					}
>  				}
>  			}
> -			omap_i2c_ack_stat(dev,
> -				stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR));
> +			omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_RRDY |
> +						OMAP_I2C_STAT_RDR));
>  			continue;
>  		}
>  
> @@ -922,8 +923,8 @@ complete:
>  
>  				omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
>  			}
> -			omap_i2c_ack_stat(dev,
> -				stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
> +			omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_XRDY |
> +						OMAP_I2C_STAT_XDR));
>  			continue;
>  		}
>  
> -- 
> 1.7.5.4
>
Wolfram Sang - June 18, 2012, 3:33 p.m.
On Mon, Jun 18, 2012 at 08:00:15PM +0530, Shubhrajyoti D wrote:
> 
> The patch series does the following
> 
> - I2C register restore only if context if the context is lost
> - Bus busy recovery mechanism.
> - the reset is not done in init.
> - Adds a patch to use devm_* functions
> - Adds a pdata function pointer to do context save restore
> - Split the omap_i2c_isr to increase readability
> - Make the i2c use SET_RUNTIME_PM_OPS
> - Use INIT_COMPLETION instead of init_completion
> - Dropping a cleanup and taking few patchs Felipe's series as it may
>  not be needed.

Patches look mostly okay for me without knowing the platform too much.
For that reason, I'd be really appreciating Tested-by or even
Reviewed-by tags!

> This series mainly is the cleanups rebased on
> i2c-embedded/for-next branch.

Thanks.

Regards,

   Wolfram
Shubhrajyoti Datta - June 19, 2012, 9:20 a.m.
On Mon, Jun 18, 2012 at 9:00 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> On Mon, Jun 18, 2012 at 08:00:28PM +0530, Shubhrajyoti D wrote:
>> From: Felipe Balbi <balbi@ti.com>
>>
>> stat & BIT(1) is the same as BIT(1),
>
> Not true. I'd guess you are missing some context in the patch
> description.


See the explanation , hope I understood your concern correctly.

http://www.mail-archive.com/linux-omap@vger.kernel.org/msg70277.html

>
>> so let's
>> simplify things a bit by removing "stat &" from
>> all omap_i2c_ack_stat() calls.
>>
>> Signed-off-by: Felipe Balbi <balbi@ti.com>
>> Reviewed-by : Santosh Shilimkar <santosh.shilimkar@ti.com>
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
>> ---
>>  drivers/i2c/busses/i2c-omap.c |   19 ++++++++++---------
>>  1 files changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>> index 6a79089..bac6305 100644
>> --- a/drivers/i2c/busses/i2c-omap.c
>> +++ b/drivers/i2c/busses/i2c-omap.c
>> @@ -763,7 +763,7 @@ static int errata_omap3_i462(struct omap_i2c_dev *dev, u16 *stat, int *err)
>>
>>       while (--timeout && !(*stat & OMAP_I2C_STAT_XUDF)) {
>>               if (*stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
>> -                     omap_i2c_ack_stat(dev, *stat & (OMAP_I2C_STAT_XRDY |
>> +                     omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_XRDY |
>>                                                       OMAP_I2C_STAT_XDR));
>>                       return -ETIMEDOUT;
>>               }
>> @@ -824,10 +824,11 @@ complete:
>>                */
>>               if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK |
>>                                       OMAP_I2C_STAT_AL)) {
>> -                     omap_i2c_ack_stat(dev, stat &
>> -                             (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR |
>> -                             OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR |
>> -                             OMAP_I2C_STAT_ARDY));
>> +                     omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_RRDY |
>> +                                             OMAP_I2C_STAT_RDR |
>> +                                             OMAP_I2C_STAT_XRDY |
>> +                                             OMAP_I2C_STAT_XDR |
>> +                                             OMAP_I2C_STAT_ARDY));
>>                       omap_i2c_complete_cmd(dev, err);
>>                       return IRQ_HANDLED;
>>               }
>> @@ -874,8 +875,8 @@ complete:
>>                                       }
>>                               }
>>                       }
>> -                     omap_i2c_ack_stat(dev,
>> -                             stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR));
>> +                     omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_RRDY |
>> +                                             OMAP_I2C_STAT_RDR));
>>                       continue;
>>               }
>>
>> @@ -922,8 +923,8 @@ complete:
>>
>>                               omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
>>                       }
>> -                     omap_i2c_ack_stat(dev,
>> -                             stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
>> +                     omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_XRDY |
>> +                                             OMAP_I2C_STAT_XDR));
>>                       continue;
>>               }
>>
>> --
>> 1.7.5.4
>>
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Wolfram Sang - June 19, 2012, 1:33 p.m.
On Tue, Jun 19, 2012 at 02:50:23PM +0530, Shubhrajyoti Datta wrote:
> On Mon, Jun 18, 2012 at 9:00 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> > On Mon, Jun 18, 2012 at 08:00:28PM +0530, Shubhrajyoti D wrote:
> >> From: Felipe Balbi <balbi@ti.com>
> >>
> >> stat & BIT(1) is the same as BIT(1),
> >
> > Not true. I'd guess you are missing some context in the patch
> > description.
> 
> 
> See the explanation , hope I understood your concern correctly.
> 
> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg70277.html

The code may be right, but as Russell pointed out, the patch description
is very weak. I could read the mail now to understand, but I prefer to
wait for a better patch description. Because I need to ensure other
people reading the git history later will understand easily without
having to search the web.

Regards,

   Wolfram
Tony Lindgren - June 20, 2012, 10:29 a.m.
* Shubhrajyoti D <shubhrajyoti@ti.com> [120618 07:35]:
> The reset in the driver at init is not needed anymore as the
> following patch has removed the HWMOD_INIT_NO_RESET flag.
> 6d3c55f [OMAP: hwmod: fix the i2c-reset timeout during bootup]
> 
> This patch does the following
> -removes the reset from the probe and implements a omap_i2c_reset
>  function to reset.
> - Reset is removed from omap_i2c_init, which was called
>  not only during probe, but also after time out and error handling.
>  omap_i2c_reset is added in those places to effect the reset.

See the comments regarding driver specific resets in hwmod code.

The way to set this up is to have a shared inline function in
i2c-omap.h that both the driver and hwmod code can use.

Eventually hwmod code will do the reset only in late initcall
if no driver is loaded for the device in question.

Tony
Tony Lindgren - June 20, 2012, 10:32 a.m.
* Shubhrajyoti D <shubhrajyoti@ti.com> [120618 07:35]:
> From: Jon Hunter <jon-hunter@ti.com>
> 
> The OMAP3530 is based upon the same silicon as the OMAP3430 and so the I2C
> revision is the same for 3430 and 3530. However, the OMAP3630 device has the
> same I2C revision as OMAP4. Correct the revision definition to reflect this.
> 
> This patch is based on work done by Jon Hunter <jon-hunter@ti.com>
> Changes from his patch
> - Update OMAP_I2C_REV_ON_3430 also to reflect that it is same as 3530
...
>  /* timeout waiting for the controller to respond */
>  #define OMAP_I2C_TIMEOUT (msecs_to_jiffies(1000))
> @@ -298,7 +298,7 @@ static int omap_i2c_reset(struct omap_i2c_dev *dev)
>  			omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG,
>  					   SYSC_AUTOIDLE_MASK);
>  
> -		} else if (dev->rev >= OMAP_I2C_REV_ON_3430) {
> +		} else if (dev->rev >= OMAP_I2C_REV_ON_3430_3530) {
>  			dev->syscstate = SYSC_AUTOIDLE_MASK;
>  			dev->syscstate |= SYSC_ENAWAKEUP_MASK;
>  			dev->syscstate |= (SYSC_IDLEMODE_SMART <<

Having to patch all over the place for these revision defines leads
into unmaintainable code as new SoCs get added.

Please instead just check the revision once during init, then set
up some feature bits like I2C_OMAP_NEEDS_XYZ_RESET that the runtime
code can check.


> @@ -1055,7 +1055,7 @@ omap_i2c_probe(struct platform_device *pdev)
>  	if (dev->flags & OMAP_I2C_FLAG_APPLY_ERRATA_I207)
>  		dev->errata |= I2C_OMAP_ERRATA_I207;
>  
> -	if (dev->rev <= OMAP_I2C_REV_ON_3430)
> +	if (dev->rev <= OMAP_I2C_REV_ON_3430_3530)
>  		dev->errata |= I2C_OMAP_ERRATA_I462;
>  
>  	if (!(dev->flags & OMAP_I2C_FLAG_NO_FIFO)) {
> @@ -1073,7 +1073,7 @@ omap_i2c_probe(struct platform_device *pdev)
>  
>  		dev->fifo_size = (dev->fifo_size / 2);
>  
> -		if (dev->rev >= OMAP_I2C_REV_ON_3530_4430)
> +		if (dev->rev >= OMAP_I2C_REV_ON_3630_4430)
>  			dev->b_hw = 0; /* Disable hardware fixes */
>  		else
>  			dev->b_hw = 1; /* Enable hardware fixes */

That way the code does not need to change when new SoC revisions
get added.

Tony
Datta, Shubhrajyoti - June 20, 2012, 1:01 p.m.
On Wednesday 20 June 2012 04:02 PM, Tony Lindgren wrote:
> * Shubhrajyoti D <shubhrajyoti@ti.com> [120618 07:35]:
>> From: Jon Hunter <jon-hunter@ti.com>
>>
>> The OMAP3530 is based upon the same silicon as the OMAP3430 and so the I2C
>> revision is the same for 3430 and 3530. However, the OMAP3630 device has the
>> same I2C revision as OMAP4. Correct the revision definition to reflect this.
>>
>> This patch is based on work done by Jon Hunter <jon-hunter@ti.com>
>> Changes from his patch
>> - Update OMAP_I2C_REV_ON_3430 also to reflect that it is same as 3530
> ...
>>  /* timeout waiting for the controller to respond */
>>  #define OMAP_I2C_TIMEOUT (msecs_to_jiffies(1000))
>> @@ -298,7 +298,7 @@ static int omap_i2c_reset(struct omap_i2c_dev *dev)
>>  			omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG,
>>  					   SYSC_AUTOIDLE_MASK);
>>  
>> -		} else if (dev->rev >= OMAP_I2C_REV_ON_3430) {
>> +		} else if (dev->rev >= OMAP_I2C_REV_ON_3430_3530) {
>>  			dev->syscstate = SYSC_AUTOIDLE_MASK;
>>  			dev->syscstate |= SYSC_ENAWAKEUP_MASK;
>>  			dev->syscstate |= (SYSC_IDLEMODE_SMART <<
> Having to patch all over the place for these revision defines leads
> into unmaintainable code as new SoCs get added.
>
> Please instead just check the revision once during init, then set
> up some feature bits like I2C_OMAP_NEEDS_XYZ_RESET that the runtime
> code can check.
Even now at probe

dev->rev is set by reading the rev register.

The (macro)name used to identify is only changed.

Also all the ips need reset the difference is that 2430 has less bits eg only autoidle and reset.
So a flag needs reset may not be meaningful.



>
>
>> @@ -1055,7 +1055,7 @@ omap_i2c_probe(struct platform_device *pdev)
>>  	if (dev->flags & OMAP_I2C_FLAG_APPLY_ERRATA_I207)
>>  		dev->errata |= I2C_OMAP_ERRATA_I207;
>>  
>> -	if (dev->rev <= OMAP_I2C_REV_ON_3430)
>> +	if (dev->rev <= OMAP_I2C_REV_ON_3430_3530)
>>  		dev->errata |= I2C_OMAP_ERRATA_I462;
>>  
>>  	if (!(dev->flags & OMAP_I2C_FLAG_NO_FIFO)) {
>> @@ -1073,7 +1073,7 @@ omap_i2c_probe(struct platform_device *pdev)
>>  
>>  		dev->fifo_size = (dev->fifo_size / 2);
>>  
>> -		if (dev->rev >= OMAP_I2C_REV_ON_3530_4430)
>> +		if (dev->rev >= OMAP_I2C_REV_ON_3630_4430)
>>  			dev->b_hw = 0; /* Disable hardware fixes */
>>  		else
>>  			dev->b_hw = 1; /* Enable hardware fixes */
> That way the code does not need to change when new SoC revisions
> get added.
I think it might still not need to change because of >= .
I have just renamed

OMAP_I2C_REV_ON_3530_4430 as 3530 rev is not the same as 4430 that the name implies.

>
> Tony
Tony Lindgren - June 20, 2012, 2:14 p.m.
* Shubhrajyoti <shubhrajyoti@ti.com> [120620 06:06]:
> On Wednesday 20 June 2012 04:02 PM, Tony Lindgren wrote:
> > * Shubhrajyoti D <shubhrajyoti@ti.com> [120618 07:35]:
> >> From: Jon Hunter <jon-hunter@ti.com>
> >>
> >> The OMAP3530 is based upon the same silicon as the OMAP3430 and so the I2C
> >> revision is the same for 3430 and 3530. However, the OMAP3630 device has the
> >> same I2C revision as OMAP4. Correct the revision definition to reflect this.
> >>
> >> This patch is based on work done by Jon Hunter <jon-hunter@ti.com>
> >> Changes from his patch
> >> - Update OMAP_I2C_REV_ON_3430 also to reflect that it is same as 3530
> > ...
> >>  /* timeout waiting for the controller to respond */
> >>  #define OMAP_I2C_TIMEOUT (msecs_to_jiffies(1000))
> >> @@ -298,7 +298,7 @@ static int omap_i2c_reset(struct omap_i2c_dev *dev)
> >>  			omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG,
> >>  					   SYSC_AUTOIDLE_MASK);
> >>  
> >> -		} else if (dev->rev >= OMAP_I2C_REV_ON_3430) {
> >> +		} else if (dev->rev >= OMAP_I2C_REV_ON_3430_3530) {
> >>  			dev->syscstate = SYSC_AUTOIDLE_MASK;
> >>  			dev->syscstate |= SYSC_ENAWAKEUP_MASK;
> >>  			dev->syscstate |= (SYSC_IDLEMODE_SMART <<
> > Having to patch all over the place for these revision defines leads
> > into unmaintainable code as new SoCs get added.
> >
> > Please instead just check the revision once during init, then set
> > up some feature bits like I2C_OMAP_NEEDS_XYZ_RESET that the runtime
> > code can check.
> Even now at probe
> 
> dev->rev is set by reading the rev register.
> 
> The (macro)name used to identify is only changed.
> 
> Also all the ips need reset the difference is that 2430 has less bits eg only autoidle and reset.
> So a flag needs reset may not be meaningful.
 
Hmm OK so this is a cosmetic/readability fix..

..but your earlier patch now spreads the checking of revision
to the new non-init function omap_i2c_reset.

Why don't you just do this cosmetic/readability rename fix
before your 03/13 patch? Then set the already existing
OMAP_I2C_FLAG_RESET_REGS_POSTIDLE during init for some
revisions and use that instead of the rev check in
omap_i2c_reset?

Tony
Shubhrajyoti Datta - June 21, 2012, 6:56 a.m.
On Wed, Jun 20, 2012 at 7:44 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Shubhrajyoti <shubhrajyoti@ti.com> [120620 06:06]:
>> On Wednesday 20 June 2012 04:02 PM, Tony Lindgren wrote:
>> > * Shubhrajyoti D <shubhrajyoti@ti.com> [120618 07:35]:
>> >> From: Jon Hunter <jon-hunter@ti.com>
>> >>
>> >> The OMAP3530 is based upon the same silicon as the OMAP3430 and so the I2C
>> >> revision is the same for 3430 and 3530. However, the OMAP3630 device has the
>> >> same I2C revision as OMAP4. Correct the revision definition to reflect this.
>> >>
>> >> This patch is based on work done by Jon Hunter <jon-hunter@ti.com>
>> >> Changes from his patch
>> >> - Update OMAP_I2C_REV_ON_3430 also to reflect that it is same as 3530
>> > ...
>> >>  /* timeout waiting for the controller to respond */
>> >>  #define OMAP_I2C_TIMEOUT (msecs_to_jiffies(1000))
>> >> @@ -298,7 +298,7 @@ static int omap_i2c_reset(struct omap_i2c_dev *dev)
>> >>                    omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG,
>> >>                                       SYSC_AUTOIDLE_MASK);
>> >>
>> >> -          } else if (dev->rev >= OMAP_I2C_REV_ON_3430) {
>> >> +          } else if (dev->rev >= OMAP_I2C_REV_ON_3430_3530) {
>> >>                    dev->syscstate = SYSC_AUTOIDLE_MASK;
>> >>                    dev->syscstate |= SYSC_ENAWAKEUP_MASK;
>> >>                    dev->syscstate |= (SYSC_IDLEMODE_SMART <<
>> > Having to patch all over the place for these revision defines leads
>> > into unmaintainable code as new SoCs get added.
>> >
>> > Please instead just check the revision once during init, then set
>> > up some feature bits like I2C_OMAP_NEEDS_XYZ_RESET that the runtime
>> > code can check.
>> Even now at probe
>>
>> dev->rev is set by reading the rev register.
>>
>> The (macro)name used to identify is only changed.
>>
>> Also all the ips need reset the difference is that 2430 has less bits eg only autoidle and reset.
>> So a flag needs reset may not be meaningful.
>
> Hmm OK so this is a cosmetic/readability fix..

Yes

>
> ..but your earlier patch now spreads the checking of revision
> to the new non-init function omap_i2c_reset.
>
> Why don't you just do this cosmetic/readability rename fix
> before your 03/13 patch?

OK thats doable.I can reorder the patch.

> Then set the already existing
> OMAP_I2C_FLAG_RESET_REGS_POSTIDLE during init for some
> revisions and use that instead of the rev check in
> omap_i2c_reset?

OMAP_I2C_FLAG_RESET_REGS_POSTIDLE is a hwmod flag whose intention is to find
know if it can lose context after idle.

The rev check that we have is because post reset( triggered by driver)
to know if we have to restore only autoidle or other bits like clockactivity
sidle , enable wakeup .

>
> Tony
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Datta, Shubhrajyoti - June 21, 2012, 7:03 a.m.
On Wednesday 20 June 2012 03:59 PM, Tony Lindgren wrote:
> See the comments regarding driver specific resets in hwmod code.
you mean omap_hwmod.c
>
> The way to set this up is to have a shared inline function in
> i2c-omap.h that both the driver and hwmod code can use.
hwmod reset function uses oh (omap_hwmod).

How could the driver also pass oh ?
Could we keep a local copy in driver data?
> Eventually hwmod code will do the reset only in late initcall
> if no driver is loaded for the device in question.
Tony Lindgren - June 21, 2012, 7:20 a.m.
* Shubhrajyoti <shubhrajyoti@ti.com> [120621 00:08]:
> On Wednesday 20 June 2012 03:59 PM, Tony Lindgren wrote:
> > See the comments regarding driver specific resets in hwmod code.
> you mean omap_hwmod.c
> >
> > The way to set this up is to have a shared inline function in
> > i2c-omap.h that both the driver and hwmod code can use.
> hwmod reset function uses oh (omap_hwmod).
> 
> How could the driver also pass oh ?
> Could we keep a local copy in driver data?
> > Eventually hwmod code will do the reset only in late initcall
> > if no driver is loaded for the device in question.
> 

There's no need for the driver to know anything about oh.
The driver just needs to know the iobase.

Tony
Datta, Shubhrajyoti - June 21, 2012, 9:30 a.m.
On Thursday 21 June 2012 12:50 PM, Tony Lindgren wrote:
> * Shubhrajyoti <shubhrajyoti@ti.com> [120621 00:08]:
>> On Wednesday 20 June 2012 03:59 PM, Tony Lindgren wrote:
>>> See the comments regarding driver specific resets in hwmod code.
>> you mean omap_hwmod.c
>>> The way to set this up is to have a shared inline function in
>>> i2c-omap.h that both the driver and hwmod code can use.
>> hwmod reset function uses oh (omap_hwmod).
>>
>> How could the driver also pass oh ?
>> Could we keep a local copy in driver data?
>>> Eventually hwmod code will do the reset only in late initcall
>>> if no driver is loaded for the device in question.
> There's no need for the driver to know anything about oh.
> The driver just needs to know the iobase.
I will rework to make the hwmod and driver use the same function.
In the meanwhile I will send a minimal/ remaining cleanups/ fixes so that
it might get some time to bake in the next branch.


>
> Tony
Tony Lindgren - June 26, 2012, 11:15 a.m.
* Shubhrajyoti <shubhrajyoti@ti.com> [120621 02:35]:
> On Thursday 21 June 2012 12:50 PM, Tony Lindgren wrote:
> > * Shubhrajyoti <shubhrajyoti@ti.com> [120621 00:08]:
> >> On Wednesday 20 June 2012 03:59 PM, Tony Lindgren wrote:
> >>> See the comments regarding driver specific resets in hwmod code.
> >> you mean omap_hwmod.c
> >>> The way to set this up is to have a shared inline function in
> >>> i2c-omap.h that both the driver and hwmod code can use.
> >> hwmod reset function uses oh (omap_hwmod).
> >>
> >> How could the driver also pass oh ?
> >> Could we keep a local copy in driver data?
> >>> Eventually hwmod code will do the reset only in late initcall
> >>> if no driver is loaded for the device in question.
> > There's no need for the driver to know anything about oh.
> > The driver just needs to know the iobase.
> I will rework to make the hwmod and driver use the same function.
> In the meanwhile I will send a minimal/ remaining cleanups/ fixes so that
> it might get some time to bake in the next branch.

OK thanks!

Tony