Patchwork [PATCHv8,00/18] I2C Updates

login
register
mail settings
Submitter Datta, Shubhrajyoti
Date April 12, 2012, 1:06 p.m.
Message ID <1334235995-6727-1-git-send-email-shubhrajyoti@ti.com>
Download mbox
Permalink /patch/152059/
State New
Headers show

Pull-request

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

Comments

Datta, Shubhrajyoti - April 12, 2012, 1:06 p.m.
The patch series does the following

- Warn fixes if CONFIG_PM_RUNTIME is not selected.
- 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
- Also checks the return type of the get_sync and in case
 of errors prevents register access, also print the cause of
 failure in case of errors.
- In case of i2c remove register access was done without any
 get_sync fix the same.
- 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
- 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.

- As discussed in [2], Paul has queued the flag for context restore patch,
 removing it from the series.

v8 
Fix Felipe's comments  and use devm_request_and_ioremap.


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

Tested on omap4sdp and omap3sdp.


The following changes since commit 258f742635360175564e9470eb060ff4d4b984e7:

  modpost: Fix modpost license checking of vmlinux.o (2012-04-09 20:52:56 -0700)

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



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

Shubhrajyoti D (15):
  I2C: OMAP: make omap_i2c_unidle/idle functions depend on
    CONFIG_PM_RUNTIME
  I2C: OMAP: Remove reset at init
  I2C: OMAP: I2C register restore only if context is lost
  I2C: OMAP: Fix the interrupt clearing in OMAP4
  I2C: OMAP: Fix the mismatch of pm_runtime enable and disable
  I2C: OMAP: Optimise the remove code
  I2C: OMAP: Fix the error handling
  I2C: OMAP: Don't check if wait_for_completion_timeout() returns less
    than zero
  I2C: OMAP: use devm_* functions
  I2C: OMAP: Fix the crash in i2c remove
  I2C: OMAP: Handle error check for pm runtime
  I2C: OMAP: Use SET_RUNTIME_PM_OPS
  I2C: OMAP: make the read ready processing a separate function
  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

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

 arch/arm/plat-omap/i2c.c      |    3 +
 drivers/i2c/busses/i2c-omap.c |  350 +++++++++++++++++++++++------------------
 include/linux/i2c-omap.h      |    1 +
 3 files changed, 199 insertions(+), 155 deletions(-)
Tony Lindgren - April 16, 2012, 6:13 p.m.
* Shubhrajyoti D <shubhrajyoti@ti.com> [120412 06:10]:
>  Currently i2c register restore is done always.
>  Adding conditional restore.
>  The i2c register restore is done only if the context is lost.
>  Also remove the definition of SYSS_RESETDONE_MASK and use the
>  one in omap_hwmod.h.
> 
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
>  arch/arm/plat-omap/i2c.c      |    3 ++
>  drivers/i2c/busses/i2c-omap.c |   52 ++++++++++++++++++++++++++--------------
>  include/linux/i2c-omap.h      |    1 +
>  3 files changed, 38 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
> index db071bc..4ccab07 100644
> --- a/arch/arm/plat-omap/i2c.c
> +++ b/arch/arm/plat-omap/i2c.c
> @@ -179,6 +179,9 @@ static inline int omap2_i2c_add_bus(int bus_id)
>  	 */
>  	if (cpu_is_omap34xx())
>  		pdata->set_mpu_wkup_lat = omap_pm_set_max_mpu_wakeup_lat_compat;
> +
> +	pdata->get_context_loss_count = omap_pm_get_dev_context_loss_count;
> +
>  	pdev = omap_device_build(name, bus_id, oh, pdata,
>  			sizeof(struct omap_i2c_bus_platform_data),
>  			NULL, 0, 0);

This should be acked by Kevin, adding him to Cc.

For the arch/arm/plat-omap/i2c.c part:

Acked-by: Tony Lindgren <tony@atomide.com>





> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index a882558..45389db 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -43,6 +43,7 @@
>  #include <linux/slab.h>
>  #include <linux/i2c-omap.h>
>  #include <linux/pm_runtime.h>
> +#include <plat/omap_device.h>
>  
>  /* I2C controller revisions */
>  #define OMAP_I2C_OMAP1_REV_2		0x20
> @@ -157,9 +158,6 @@ enum {
>  #define OMAP_I2C_SYSTEST_SDA_I		(1 << 1)	/* SDA line sense in */
>  #define OMAP_I2C_SYSTEST_SDA_O		(1 << 0)	/* SDA line drive out */
>  
> -/* OCP_SYSSTATUS bit definitions */
> -#define SYSS_RESETDONE_MASK		(1 << 0)
> -
>  /* OCP_SYSCONFIG bit definitions */
>  #define SYSC_CLOCKACTIVITY_MASK		(0x3 << 8)
>  #define SYSC_SIDLEMODE_MASK		(0x3 << 3)
> @@ -184,6 +182,7 @@ struct omap_i2c_dev {
>  	u32			latency;	/* maximum mpu wkup latency */
>  	void			(*set_mpu_wkup_lat)(struct device *dev,
>  						    long latency);
> +	int			(*get_context_loss_count)(struct device *dev);
>  	u32			speed;		/* Speed of bus in kHz */
>  	u32			dtrev;		/* extra revision from DT */
>  	u32			flags;
> @@ -206,6 +205,7 @@ struct omap_i2c_dev {
>  	u16			syscstate;
>  	u16			westate;
>  	u16			errata;
> +	int			dev_lost_count;
>  };
>  
>  static const u8 reg_map_ip_v1[] = {
> @@ -1025,6 +1025,7 @@ omap_i2c_probe(struct platform_device *pdev)
>  		dev->speed = pdata->clkrate;
>  		dev->flags = pdata->flags;
>  		dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
> +		dev->get_context_loss_count = pdata->get_context_loss_count;
>  		dev->dtrev = pdata->rev;
>  	}
>  
> @@ -1151,12 +1152,32 @@ omap_i2c_remove(struct platform_device *pdev)
>  }
>  
>  #ifdef CONFIG_PM_RUNTIME
> +static void omap_i2c_restore(struct omap_i2c_dev *dev)
> +{
> +	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_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 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;
>  
> +	if (_dev->get_context_loss_count)
> +		_dev->dev_lost_count = _dev->get_context_loss_count(dev);
> +
>  	_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);
> @@ -1179,24 +1200,19 @@ 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);
> +	int loss_cnt;
>  
> -	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);
> +	if (_dev->get_context_loss_count) {
> +		loss_cnt = _dev->get_context_loss_count(dev);
> +		if (loss_cnt < 0)
> +			return loss_cnt;
> +
> +		if (_dev->dev_lost_count == loss_cnt && _dev->dev_lost_count)
> +			return 0;
>  	}
>  
> -	/*
> -	 * 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);
> +	if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE)
> +		omap_i2c_restore(_dev);
>  
>  	return 0;
>  }
> diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
> index 92a0dc7..c76cbc0 100644
> --- a/include/linux/i2c-omap.h
> +++ b/include/linux/i2c-omap.h
> @@ -35,6 +35,7 @@ struct omap_i2c_bus_platform_data {
>  	u32		rev;
>  	u32		flags;
>  	void		(*set_mpu_wkup_lat)(struct device *dev, long set);
> +	int		(*get_context_loss_count)(struct device *dev);
>  };
>  
>  #endif
> -- 
> 1.7.4.1
>
Datta, Shubhrajyoti - April 17, 2012, 5:50 a.m.
On Monday 16 April 2012 11:43 PM, Tony Lindgren wrote:
> * Shubhrajyoti D <shubhrajyoti@ti.com> [120412 06:10]:
>>  Currently i2c register restore is done always.
>>  Adding conditional restore.
>>  The i2c register restore is done only if the context is lost.
>>  Also remove the definition of SYSS_RESETDONE_MASK and use the
>>  one in omap_hwmod.h.
>>
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
>> ---
>>  arch/arm/plat-omap/i2c.c      |    3 ++
>>  drivers/i2c/busses/i2c-omap.c |   52 ++++++++++++++++++++++++++--------------
>>  include/linux/i2c-omap.h      |    1 +
>>  3 files changed, 38 insertions(+), 18 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
>> index db071bc..4ccab07 100644
>> --- a/arch/arm/plat-omap/i2c.c
>> +++ b/arch/arm/plat-omap/i2c.c
>> @@ -179,6 +179,9 @@ static inline int omap2_i2c_add_bus(int bus_id)
>>  	 */
>>  	if (cpu_is_omap34xx())
>>  		pdata->set_mpu_wkup_lat = omap_pm_set_max_mpu_wakeup_lat_compat;
>> +
>> +	pdata->get_context_loss_count = omap_pm_get_dev_context_loss_count;
>> +
>>  	pdev = omap_device_build(name, bus_id, oh, pdata,
>>  			sizeof(struct omap_i2c_bus_platform_data),
>>  			NULL, 0, 0);
> This should be acked by Kevin, 
I should have cced Kevin.
> adding him to Cc.
Thanks.
> For the arch/arm/plat-omap/i2c.c part:
>
> Acked-by: Tony Lindgren <tony@atomide.com>
>
Thanks.
Kevin Hilman - April 17, 2012, 1:45 p.m.
Shubhrajyoti D <shubhrajyoti@ti.com> writes:

>  Currently i2c register restore is done always.
>  Adding conditional restore.
>  The i2c register restore is done only if the context is lost.

OK, minor comment below.

>  Also remove the definition of SYSS_RESETDONE_MASK and use the
>  one in omap_hwmod.h.

The definition is removed, but I don't see any of the users removed.
If the users were cleaned up earlier in the series, then the removal of
this should be done with that, or as a separate patch.  I don't see why
it belongs with this patch.

> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
>  arch/arm/plat-omap/i2c.c      |    3 ++
>  drivers/i2c/busses/i2c-omap.c |   52 ++++++++++++++++++++++++++--------------
>  include/linux/i2c-omap.h      |    1 +
>  3 files changed, 38 insertions(+), 18 deletions(-)
>
> diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
> index db071bc..4ccab07 100644
> --- a/arch/arm/plat-omap/i2c.c
> +++ b/arch/arm/plat-omap/i2c.c
> @@ -179,6 +179,9 @@ static inline int omap2_i2c_add_bus(int bus_id)
>  	 */
>  	if (cpu_is_omap34xx())
>  		pdata->set_mpu_wkup_lat = omap_pm_set_max_mpu_wakeup_lat_compat;
> +
> +	pdata->get_context_loss_count = omap_pm_get_dev_context_loss_count;
> +
>  	pdev = omap_device_build(name, bus_id, oh, pdata,
>  			sizeof(struct omap_i2c_bus_platform_data),
>  			NULL, 0, 0);
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index a882558..45389db 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -43,6 +43,7 @@
>  #include <linux/slab.h>
>  #include <linux/i2c-omap.h>
>  #include <linux/pm_runtime.h>
> +#include <plat/omap_device.h>
>  
>  /* I2C controller revisions */
>  #define OMAP_I2C_OMAP1_REV_2		0x20
> @@ -157,9 +158,6 @@ enum {
>  #define OMAP_I2C_SYSTEST_SDA_I		(1 << 1)	/* SDA line sense in */
>  #define OMAP_I2C_SYSTEST_SDA_O		(1 << 0)	/* SDA line drive out */
>  
> -/* OCP_SYSSTATUS bit definitions */
> -#define SYSS_RESETDONE_MASK		(1 << 0)
> -
>  /* OCP_SYSCONFIG bit definitions */
>  #define SYSC_CLOCKACTIVITY_MASK		(0x3 << 8)
>  #define SYSC_SIDLEMODE_MASK		(0x3 << 3)
> @@ -184,6 +182,7 @@ struct omap_i2c_dev {
>  	u32			latency;	/* maximum mpu wkup latency */
>  	void			(*set_mpu_wkup_lat)(struct device *dev,
>  						    long latency);
> +	int			(*get_context_loss_count)(struct device *dev);
>  	u32			speed;		/* Speed of bus in kHz */
>  	u32			dtrev;		/* extra revision from DT */
>  	u32			flags;
> @@ -206,6 +205,7 @@ struct omap_i2c_dev {
>  	u16			syscstate;
>  	u16			westate;
>  	u16			errata;
> +	int			dev_lost_count;
>  };
>  
>  static const u8 reg_map_ip_v1[] = {
> @@ -1025,6 +1025,7 @@ omap_i2c_probe(struct platform_device *pdev)
>  		dev->speed = pdata->clkrate;
>  		dev->flags = pdata->flags;
>  		dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
> +		dev->get_context_loss_count = pdata->get_context_loss_count;
>  		dev->dtrev = pdata->rev;
>  	}
>  
> @@ -1151,12 +1152,32 @@ omap_i2c_remove(struct platform_device *pdev)
>  }
>  
>  #ifdef CONFIG_PM_RUNTIME
> +static void omap_i2c_restore(struct omap_i2c_dev *dev)
> +{
> +	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_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 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;
>  
> +	if (_dev->get_context_loss_count)
> +		_dev->dev_lost_count = _dev->get_context_loss_count(dev);
> +
>  	_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);
> @@ -1179,24 +1200,19 @@ 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);
> +	int loss_cnt;
>  
> -	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);
> +	if (_dev->get_context_loss_count) {
> +		loss_cnt = _dev->get_context_loss_count(dev);
> +		if (loss_cnt < 0)
> +			return loss_cnt;

To avoid messing up driver state, upon error, you should probably
restore context, not abort.

> +		if (_dev->dev_lost_count == loss_cnt && _dev->dev_lost_count)
> +			return 0;

Why the non-zero check?

Seems like all you need to check is if (_dev->dev_lost_count != loss_cnt).

Kevin

>  	}
>  
> -	/*
> -	 * 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);
> +	if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE)
> +		omap_i2c_restore(_dev);
>  
>  	return 0;
>  }
> diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
> index 92a0dc7..c76cbc0 100644
> --- a/include/linux/i2c-omap.h
> +++ b/include/linux/i2c-omap.h
> @@ -35,6 +35,7 @@ struct omap_i2c_bus_platform_data {
>  	u32		rev;
>  	u32		flags;
>  	void		(*set_mpu_wkup_lat)(struct device *dev, long set);
> +	int		(*get_context_loss_count)(struct device *dev);
>  };
>  
>  #endif
Shubhrajyoti Datta - April 17, 2012, 3:08 p.m.
Hi Kevin,
Thanks for your review,

On Tue, Apr 17, 2012 at 7:15 PM, Kevin Hilman <khilman@ti.com> wrote:
> Shubhrajyoti D <shubhrajyoti@ti.com> writes:
>
>>  Currently i2c register restore is done always.
>>  Adding conditional restore.
>>  The i2c register restore is done only if the context is lost.
>
> OK, minor comment below.
>
>>  Also remove the definition of SYSS_RESETDONE_MASK and use the
>>  one in omap_hwmod.h.
>
> The definition is removed, but I don't see any of the users removed.
> If the users were cleaned up earlier in the series, then the removal of
> this should be done with that, or as a separate patch.  I don't see why
> it belongs with this patch.

I have not removed the users actually the macro was redefined.
I have instead used the definition in omap_hwmod.h which gets added
when I include omap_device.h

Let me know if you prefer a  separate patch?

>
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
>> ---
>>  arch/arm/plat-omap/i2c.c      |    3 ++
>>  drivers/i2c/busses/i2c-omap.c |   52 ++++++++++++++++++++++++++--------------
>>  include/linux/i2c-omap.h      |    1 +
>>  3 files changed, 38 insertions(+), 18 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
>> index db071bc..4ccab07 100644
>> --- a/arch/arm/plat-omap/i2c.c
>> +++ b/arch/arm/plat-omap/i2c.c
>> @@ -179,6 +179,9 @@ static inline int omap2_i2c_add_bus(int bus_id)
>>        */
>>       if (cpu_is_omap34xx())
>>               pdata->set_mpu_wkup_lat = omap_pm_set_max_mpu_wakeup_lat_compat;
>> +
>> +     pdata->get_context_loss_count = omap_pm_get_dev_context_loss_count;
>> +
>>       pdev = omap_device_build(name, bus_id, oh, pdata,
>>                       sizeof(struct omap_i2c_bus_platform_data),
>>                       NULL, 0, 0);
>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>> index a882558..45389db 100644
>> --- a/drivers/i2c/busses/i2c-omap.c
>> +++ b/drivers/i2c/busses/i2c-omap.c
>> @@ -43,6 +43,7 @@
>>  #include <linux/slab.h>
>>  #include <linux/i2c-omap.h>
>>  #include <linux/pm_runtime.h>
>> +#include <plat/omap_device.h>
>>
>>  /* I2C controller revisions */
>>  #define OMAP_I2C_OMAP1_REV_2         0x20
>> @@ -157,9 +158,6 @@ enum {
>>  #define OMAP_I2C_SYSTEST_SDA_I               (1 << 1)        /* SDA line sense in */
>>  #define OMAP_I2C_SYSTEST_SDA_O               (1 << 0)        /* SDA line drive out */
>>
>> -/* OCP_SYSSTATUS bit definitions */
>> -#define SYSS_RESETDONE_MASK          (1 << 0)
>> -
>>  /* OCP_SYSCONFIG bit definitions */
>>  #define SYSC_CLOCKACTIVITY_MASK              (0x3 << 8)
>>  #define SYSC_SIDLEMODE_MASK          (0x3 << 3)
>> @@ -184,6 +182,7 @@ struct omap_i2c_dev {
>>       u32                     latency;        /* maximum mpu wkup latency */
>>       void                    (*set_mpu_wkup_lat)(struct device *dev,
>>                                                   long latency);
>> +     int                     (*get_context_loss_count)(struct device *dev);
>>       u32                     speed;          /* Speed of bus in kHz */
>>       u32                     dtrev;          /* extra revision from DT */
>>       u32                     flags;
>> @@ -206,6 +205,7 @@ struct omap_i2c_dev {
>>       u16                     syscstate;
>>       u16                     westate;
>>       u16                     errata;
>> +     int                     dev_lost_count;
>>  };
>>
>>  static const u8 reg_map_ip_v1[] = {
>> @@ -1025,6 +1025,7 @@ omap_i2c_probe(struct platform_device *pdev)
>>               dev->speed = pdata->clkrate;
>>               dev->flags = pdata->flags;
>>               dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
>> +             dev->get_context_loss_count = pdata->get_context_loss_count;
>>               dev->dtrev = pdata->rev;
>>       }
>>
>> @@ -1151,12 +1152,32 @@ omap_i2c_remove(struct platform_device *pdev)
>>  }
>>
>>  #ifdef CONFIG_PM_RUNTIME
>> +static void omap_i2c_restore(struct omap_i2c_dev *dev)
>> +{
>> +     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_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 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;
>>
>> +     if (_dev->get_context_loss_count)
>> +             _dev->dev_lost_count = _dev->get_context_loss_count(dev);
>> +
>>       _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);
>> @@ -1179,24 +1200,19 @@ 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);
>> +     int loss_cnt;
>>
>> -     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);
>> +     if (_dev->get_context_loss_count) {
>> +             loss_cnt = _dev->get_context_loss_count(dev);
>> +             if (loss_cnt < 0)
>> +                     return loss_cnt;
>
> To avoid messing up driver state, upon error, you should probably
> restore context, not abort.
Agreed , will fix this.

>
>> +             if (_dev->dev_lost_count == loss_cnt && _dev->dev_lost_count)
>> +                     return 0;
>
> Why the non-zero check?

Actually the driver probe-->omap_i2c_init

here
<snip>
                dev->pscstate = psc;
                dev->scllstate = scll;
                dev->sclhstate = sclh;
                dev->bufstate = buf;
<snip>

variables are updated and the first write happens in the handler.

>
> Seems like all you need to check is if (_dev->dev_lost_count != loss_cnt).
>
> Kevin
>
>>       }
>>
>> -     /*
>> -      * 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);
>> +     if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE)
>> +             omap_i2c_restore(_dev);
>>
>>       return 0;
>>  }
>> diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
>> index 92a0dc7..c76cbc0 100644
>> --- a/include/linux/i2c-omap.h
>> +++ b/include/linux/i2c-omap.h
>> @@ -35,6 +35,7 @@ struct omap_i2c_bus_platform_data {
>>       u32             rev;
>>       u32             flags;
>>       void            (*set_mpu_wkup_lat)(struct device *dev, long set);
>> +     int             (*get_context_loss_count)(struct device *dev);
>>  };
>>
>>  #endif
> --
> 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
Kevin Hilman - April 18, 2012, 2:08 p.m.
"Datta, Shubhrajyoti" <shubhrajyoti@ti.com> writes:

> Hi Kevin,
>
>> Hi Kevin,
>> Thanks for your review,
>>
>> On Tue, Apr 17, 2012 at 7:15 PM, Kevin Hilman <khilman@ti.com> wrote:
>>> Shubhrajyoti D <shubhrajyoti@ti.com> writes:
>>>
>>>>  Currently i2c register restore is done always.
>>>>  Adding conditional restore.
>>>>  The i2c register restore is done only if the context is lost.
>>>
>>> OK, minor comment below.
>
> Thanks for the suggestion of the error case restore.
> Updated the patch. Also added Tony's ack for the plat part.
>
>
> From 1ca222f6f50868e07d9a46575e73dd66fd2d542e Mon Sep 17 00:00:00 2001
> From: Shubhrajyoti D <shubhrajyoti@ti.com>
> Date: Tue, 17 Jan 2012 16:13:03 +0530
> Subject: [PATCH] I2C: OMAP: I2C register restore only if context is lost
>
>  Currently i2c register restore is done always.
>  Adding conditional restore.
>  The i2c register restore is done only if the context is lost
>  or in case of error to be on the safe side.
>  Also remove the definition of SYSS_RESETDONE_MASK and use the
>  one in omap_hwmod.h.
>
> Cc: Kevin Hilman <khilman@ti.com>
> [For the arch/arm/plat-omap/i2c.c part]
> Acked-by: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
>  arch/arm/plat-omap/i2c.c      |    3 ++
>  drivers/i2c/busses/i2c-omap.c |   53 +++++++++++++++++++++++++++--------------
>  include/linux/i2c-omap.h      |    1 +
>  3 files changed, 39 insertions(+), 18 deletions(-)
>
> diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
> index db071bc..4ccab07 100644
> --- a/arch/arm/plat-omap/i2c.c
> +++ b/arch/arm/plat-omap/i2c.c
> @@ -179,6 +179,9 @@ static inline int omap2_i2c_add_bus(int bus_id)
>  	 */
>  	if (cpu_is_omap34xx())
>  		pdata->set_mpu_wkup_lat = omap_pm_set_max_mpu_wakeup_lat_compat;
> +
> +	pdata->get_context_loss_count = omap_pm_get_dev_context_loss_count;
> +
>  	pdev = omap_device_build(name, bus_id, oh, pdata,
>  			sizeof(struct omap_i2c_bus_platform_data),
>  			NULL, 0, 0);
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index a882558..76cf066 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -43,6 +43,7 @@
>  #include <linux/slab.h>
>  #include <linux/i2c-omap.h>
>  #include <linux/pm_runtime.h>
> +#include <plat/omap_device.h>
>
>  /* I2C controller revisions */
>  #define OMAP_I2C_OMAP1_REV_2		0x20
> @@ -157,9 +158,6 @@ enum {
>  #define OMAP_I2C_SYSTEST_SDA_I		(1 << 1)	/* SDA line sense in */
>  #define OMAP_I2C_SYSTEST_SDA_O		(1 << 0)	/* SDA line drive out */
>
> -/* OCP_SYSSTATUS bit definitions */
> -#define SYSS_RESETDONE_MASK		(1 << 0)
> -

Unrelated to this patch.

>  /* OCP_SYSCONFIG bit definitions */
>  #define SYSC_CLOCKACTIVITY_MASK		(0x3 << 8)
>  #define SYSC_SIDLEMODE_MASK		(0x3 << 3)
> @@ -184,6 +182,7 @@ struct omap_i2c_dev {
>  	u32			latency;	/* maximum mpu wkup latency */
>  	void			(*set_mpu_wkup_lat)(struct device *dev,
>  						    long latency);
> +	int			(*get_context_loss_count)(struct device *dev);
>  	u32			speed;		/* Speed of bus in kHz */
>  	u32			dtrev;		/* extra revision from DT */
>  	u32			flags;
> @@ -206,6 +205,7 @@ struct omap_i2c_dev {
>  	u16			syscstate;
>  	u16			westate;
>  	u16			errata;
> +	int			dev_lost_count;
>  };
>
>  static const u8 reg_map_ip_v1[] = {
> @@ -1025,6 +1025,7 @@ omap_i2c_probe(struct platform_device *pdev)
>  		dev->speed = pdata->clkrate;
>  		dev->flags = pdata->flags;
>  		dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
> +		dev->get_context_loss_count = pdata->get_context_loss_count;
>  		dev->dtrev = pdata->rev;
>  	}
>
> @@ -1151,12 +1152,32 @@ omap_i2c_remove(struct platform_device *pdev)
>  }
>
>  #ifdef CONFIG_PM_RUNTIME
> +static void omap_i2c_restore(struct omap_i2c_dev *dev)
> +{
> +	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_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 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;
>
> +	if (_dev->get_context_loss_count)
> +		_dev->dev_lost_count = _dev->get_context_loss_count(dev);
> +
>  	_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);
> @@ -1179,24 +1200,20 @@ 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);
> +	int loss_cnt;
>
> -	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);
> +	if (_dev->get_context_loss_count) {
> +		loss_cnt = _dev->get_context_loss_count(dev);
> +		if (loss_cnt < 0) {
> +			omap_i2c_restore(_dev);
> +			return 0;
> +		}
> +		if (_dev->dev_lost_count == loss_cnt && _dev->dev_lost_count)
> +			return 0;

Again, why does it matter if _dev->dev_lost_count != 0?

IMO, this is still more confusing than it needs to be.  What is wrong
with

	if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE)
		return 0;

	/* read current loss_cnt */

	if (_dev->dev_lost_count != loss_cnt)
		omap_i2c_restore(_dev);

        return 0;

Kevin

>
> -	/*
> -	 * 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);
> +	if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE)
> +		omap_i2c_restore(_dev);
>
>  	return 0;
>  }
> diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
> index 92a0dc7..c76cbc0 100644
> --- a/include/linux/i2c-omap.h
> +++ b/include/linux/i2c-omap.h
> @@ -35,6 +35,7 @@ struct omap_i2c_bus_platform_data {
>  	u32		rev;
>  	u32		flags;
>  	void		(*set_mpu_wkup_lat)(struct device *dev, long set);
> +	int		(*get_context_loss_count)(struct device *dev);
>  };
>
>  #endif