Patchwork [PATCHv7,00/18] I2C updates

login
register
mail settings
Submitter Datta, Shubhrajyoti
Date April 11, 2012, 11:12 a.m.
Message ID <1334142776-10583-1-git-send-email-shubhrajyoti@ti.com>
Download mbox
Permalink /patch/151760/
State New
Headers show

Pull-request

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

Comments

Datta, Shubhrajyoti - April 11, 2012, 11:12 a.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.


[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 |  348 +++++++++++++++++++++++------------------
 include/linux/i2c-omap.h      |    1 +
 3 files changed, 198 insertions(+), 154 deletions(-)
Felipe Balbi - April 11, 2012, 11:32 a.m.
On Wed, Apr 11, 2012 at 04:42:39PM +0530, Shubhrajyoti D wrote:
> The functions omap_i2c_unidle/idle are called from omap_i2c_runtime_resume
> and omap_i2c_runtime_suspend which is compiled for CONFIG_PM_RUNTIME.
> This patch removes the omap_i2c_unidle/idle functions and folds them
> into the runtime callbacks.
> 
> This fixes the below warn when CONFIG_PM_RUNTIME is not defined
> 
>  CC      arch/arm/mach-omap2/board-ti8168evm.o
> drivers/i2c/busses/i2c-omap.c:272: warning: 'omap_i2c_unidle' defined but not used
> drivers/i2c/busses/i2c-omap.c:293: warning: 'omap_i2c_idle' defined but not used
>   CC      net/ipv4/ip_forward.o
> 
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>

how about just moving them into the #ifdef ? /me thinks it's easier to
read with the function. Matter of taste though, it's not that much code.
Felipe Balbi - April 11, 2012, 11:34 a.m.
On Wed, Apr 11, 2012 at 04:42:49PM +0530, Shubhrajyoti D wrote:
> The various devm_ functions allocate memory that is released when a driver
> detaches. This patch uses devm_kzalloc, devm_request_mem_region and
> devm_ioremap for data that is allocated in the probe function of a platform
> device and is only freed in the remove function.
> 
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
>  drivers/i2c/busses/i2c-omap.c |   29 +++++++++--------------------
>  1 files changed, 9 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 121c52e..86be475 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -995,17 +995,17 @@ omap_i2c_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> -	ioarea = request_mem_region(mem->start, resource_size(mem),
> -			pdev->name);
> +	ioarea = devm_request_mem_region(&pdev->dev, mem->start,
> +			resource_size(mem), pdev->name);

you could use devm_request_and_ioremap()
Felipe Balbi - April 11, 2012, 11:35 a.m.
On Wed, Apr 11, 2012 at 04:42:52PM +0530, Shubhrajyoti D wrote:
> Use SET_RUNTIME_PM_OPS macro to set runtime functions.
> 
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
>  drivers/i2c/busses/i2c-omap.c |   11 ++++-------
>  1 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index dd65416..28de1d2 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -1217,15 +1217,12 @@ static int omap_i2c_runtime_resume(struct device *dev)
>  
>  	return 0;
>  }
> +#endif
>  
>  static struct dev_pm_ops omap_i2c_pm_ops = {
> -	.runtime_suspend = omap_i2c_runtime_suspend,
> -	.runtime_resume = omap_i2c_runtime_resume,
> +	SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
> +			   omap_i2c_runtime_resume, NULL)
>  };
> -#define OMAP_I2C_PM_OPS (&omap_i2c_pm_ops)
> -#else
> -#define OMAP_I2C_PM_OPS NULL
> -#endif

I think you should keep this define, otherwise the pm pointer will
always be valid.
Datta, Shubhrajyoti - April 11, 2012, 11:56 a.m.
On Wednesday 11 April 2012 05:04 PM, Felipe Balbi wrote:
> On Wed, Apr 11, 2012 at 04:42:49PM +0530, Shubhrajyoti D wrote:
>> The various devm_ functions allocate memory that is released when a driver
>> detaches. This patch uses devm_kzalloc, devm_request_mem_region and
>> devm_ioremap for data that is allocated in the probe function of a platform
>> device and is only freed in the remove function.
>>
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
>> ---
>>  drivers/i2c/busses/i2c-omap.c |   29 +++++++++--------------------
>>  1 files changed, 9 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>> index 121c52e..86be475 100644
>> --- a/drivers/i2c/busses/i2c-omap.c
>> +++ b/drivers/i2c/busses/i2c-omap.c
>> @@ -995,17 +995,17 @@ omap_i2c_probe(struct platform_device *pdev)
>>  		return -ENODEV;
>>  	}
>>  
>> -	ioarea = request_mem_region(mem->start, resource_size(mem),
>> -			pdev->name);
>> +	ioarea = devm_request_mem_region(&pdev->dev, mem->start,
>> +			resource_size(mem), pdev->name);
> you could use devm_request_and_ioremap()
OK will do that.

thanks,
Shubhrajyoti Datta - April 11, 2012, 11:59 a.m.
Hi Felipe,

On Wed, Apr 11, 2012 at 5:02 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Wed, Apr 11, 2012 at 04:42:39PM +0530, Shubhrajyoti D wrote:
>> The functions omap_i2c_unidle/idle are called from omap_i2c_runtime_resume
>> and omap_i2c_runtime_suspend which is compiled for CONFIG_PM_RUNTIME.
>> This patch removes the omap_i2c_unidle/idle functions and folds them
>> into the runtime callbacks.
>>
>> This fixes the below warn when CONFIG_PM_RUNTIME is not defined
>>
>>  CC      arch/arm/mach-omap2/board-ti8168evm.o
>> drivers/i2c/busses/i2c-omap.c:272: warning: 'omap_i2c_unidle' defined but not used
>> drivers/i2c/busses/i2c-omap.c:293: warning: 'omap_i2c_idle' defined but not used
>>   CC      net/ipv4/ip_forward.o
>>
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
>
> how about just moving them into the #ifdef ? /me thinks it's easier to
> read with the function. Matter of taste though, it's not that much code.

Kevin preferred folding into the  runtime functions.

http://www.spinics.net/lists/linux-omap/msg62764.html

Don't feel strongly though.

>
> --
> balbi
Felipe Balbi - April 11, 2012, noon
On Wed, Apr 11, 2012 at 05:29:07PM +0530, Shubhrajyoti Datta wrote:
> Hi Felipe,
> 
> On Wed, Apr 11, 2012 at 5:02 PM, Felipe Balbi <balbi@ti.com> wrote:
> > On Wed, Apr 11, 2012 at 04:42:39PM +0530, Shubhrajyoti D wrote:
> >> The functions omap_i2c_unidle/idle are called from omap_i2c_runtime_resume
> >> and omap_i2c_runtime_suspend which is compiled for CONFIG_PM_RUNTIME.
> >> This patch removes the omap_i2c_unidle/idle functions and folds them
> >> into the runtime callbacks.
> >>
> >> This fixes the below warn when CONFIG_PM_RUNTIME is not defined
> >>
> >>  CC      arch/arm/mach-omap2/board-ti8168evm.o
> >> drivers/i2c/busses/i2c-omap.c:272: warning: 'omap_i2c_unidle' defined but not used
> >> drivers/i2c/busses/i2c-omap.c:293: warning: 'omap_i2c_idle' defined but not used
> >>   CC      net/ipv4/ip_forward.o
> >>
> >> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> >
> > how about just moving them into the #ifdef ? /me thinks it's easier to
> > read with the function. Matter of taste though, it's not that much code.
> 
> Kevin preferred folding into the  runtime functions.
> 
> http://www.spinics.net/lists/linux-omap/msg62764.html
> 
> Don't feel strongly though.

ok, I stand corrected ;-) No need to change your patch again ;-)
Hebbar, Gururaja - April 11, 2012, 12:09 p.m.
Bablpi,

On Wed, Apr 11, 2012 at 17:05:38, Balbi, Felipe wrote:
> On Wed, Apr 11, 2012 at 04:42:52PM +0530, Shubhrajyoti D wrote:
> > Use SET_RUNTIME_PM_OPS macro to set runtime functions.
> > 
> > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> > ---
> >  drivers/i2c/busses/i2c-omap.c |   11 ++++-------
> >  1 files changed, 4 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index dd65416..28de1d2 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -1217,15 +1217,12 @@ static int omap_i2c_runtime_resume(struct device *dev)
> >  
> >  	return 0;
> >  }
> > +#endif
> >  
> >  static struct dev_pm_ops omap_i2c_pm_ops = {
> > -	.runtime_suspend = omap_i2c_runtime_suspend,
> > -	.runtime_resume = omap_i2c_runtime_resume,
> > +	SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
> > +			   omap_i2c_runtime_resume, NULL)
> >  };
> > -#define OMAP_I2C_PM_OPS (&omap_i2c_pm_ops)
> > -#else
> > -#define OMAP_I2C_PM_OPS NULL
> > -#endif
> 
> I think you should keep this define, otherwise the pm pointer will
> always be valid.

No. using SET_RUNTIME_PM_OPS will make it NULL when !CONFIG_PM_RUNTIME
Kindly correct me if I am wrong.

> 
> -- 
> balbi
> 


Regards, 
Gururaja
Felipe Balbi - April 11, 2012, 12:10 p.m.
On Wed, Apr 11, 2012 at 12:09:22PM +0000, Hebbar, Gururaja wrote:
> Bablpi,
> 
> On Wed, Apr 11, 2012 at 17:05:38, Balbi, Felipe wrote:
> > On Wed, Apr 11, 2012 at 04:42:52PM +0530, Shubhrajyoti D wrote:
> > > Use SET_RUNTIME_PM_OPS macro to set runtime functions.
> > > 
> > > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> > > ---
> > >  drivers/i2c/busses/i2c-omap.c |   11 ++++-------
> > >  1 files changed, 4 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > > index dd65416..28de1d2 100644
> > > --- a/drivers/i2c/busses/i2c-omap.c
> > > +++ b/drivers/i2c/busses/i2c-omap.c
> > > @@ -1217,15 +1217,12 @@ static int omap_i2c_runtime_resume(struct device *dev)
> > >  
> > >  	return 0;
> > >  }
> > > +#endif
> > >  
> > >  static struct dev_pm_ops omap_i2c_pm_ops = {
> > > -	.runtime_suspend = omap_i2c_runtime_suspend,
> > > -	.runtime_resume = omap_i2c_runtime_resume,
> > > +	SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
> > > +			   omap_i2c_runtime_resume, NULL)
> > >  };
> > > -#define OMAP_I2C_PM_OPS (&omap_i2c_pm_ops)
> > > -#else
> > > -#define OMAP_I2C_PM_OPS NULL
> > > -#endif
> > 
> > I think you should keep this define, otherwise the pm pointer will
> > always be valid.
> 
> No. using SET_RUNTIME_PM_OPS will make it NULL when !CONFIG_PM_RUNTIME
> Kindly correct me if I am wrong.

you will have a defined structure with NULL members, but the structure
itself is still valid.
Hebbar, Gururaja - April 11, 2012, 12:13 p.m.
On Wed, Apr 11, 2012 at 17:40:58, Balbi, Felipe wrote:
> On Wed, Apr 11, 2012 at 12:09:22PM +0000, Hebbar, Gururaja wrote:
> > Bablpi,
> > 
> > On Wed, Apr 11, 2012 at 17:05:38, Balbi, Felipe wrote:
> > > On Wed, Apr 11, 2012 at 04:42:52PM +0530, Shubhrajyoti D wrote:
> > > > Use SET_RUNTIME_PM_OPS macro to set runtime functions.
> > > > 
> > > > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> > > > ---
> > > >  drivers/i2c/busses/i2c-omap.c |   11 ++++-------
> > > >  1 files changed, 4 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > > > index dd65416..28de1d2 100644
> > > > --- a/drivers/i2c/busses/i2c-omap.c
> > > > +++ b/drivers/i2c/busses/i2c-omap.c
> > > > @@ -1217,15 +1217,12 @@ static int omap_i2c_runtime_resume(struct device *dev)
> > > >  
> > > >  	return 0;
> > > >  }
> > > > +#endif
> > > >  
> > > >  static struct dev_pm_ops omap_i2c_pm_ops = {
> > > > -	.runtime_suspend = omap_i2c_runtime_suspend,
> > > > -	.runtime_resume = omap_i2c_runtime_resume,
> > > > +	SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
> > > > +			   omap_i2c_runtime_resume, NULL)
> > > >  };
> > > > -#define OMAP_I2C_PM_OPS (&omap_i2c_pm_ops)
> > > > -#else
> > > > -#define OMAP_I2C_PM_OPS NULL
> > > > -#endif
> > > 
> > > I think you should keep this define, otherwise the pm pointer will
> > > always be valid.
> > 
> > No. using SET_RUNTIME_PM_OPS will make it NULL when !CONFIG_PM_RUNTIME
> > Kindly correct me if I am wrong.
> 
> you will have a defined structure with NULL members, but the structure
> itself is still valid.

Oops. You are correct. I stand corrected.

> 
> -- 
> balbi
> 


Regards, 
Gururaja
Shubhrajyoti Datta - April 11, 2012, 1:10 p.m.
Hi Felipe,

On Wed, Apr 11, 2012 at 5:05 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Wed, Apr 11, 2012 at 04:42:52PM +0530, Shubhrajyoti D wrote:
>> Use SET_RUNTIME_PM_OPS macro to set runtime functions.
>>
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
>> ---
>>  drivers/i2c/busses/i2c-omap.c |   11 ++++-------
>>  1 files changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>> index dd65416..28de1d2 100644
>> --- a/drivers/i2c/busses/i2c-omap.c
>> +++ b/drivers/i2c/busses/i2c-omap.c
>> @@ -1217,15 +1217,12 @@ static int omap_i2c_runtime_resume(struct device *dev)
>>
>>       return 0;
>>  }
>> +#endif
>>
>>  static struct dev_pm_ops omap_i2c_pm_ops = {
>> -     .runtime_suspend = omap_i2c_runtime_suspend,
>> -     .runtime_resume = omap_i2c_runtime_resume,
>> +     SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
>> +                        omap_i2c_runtime_resume, NULL)
>>  };
>> -#define OMAP_I2C_PM_OPS (&omap_i2c_pm_ops)
>> -#else
>> -#define OMAP_I2C_PM_OPS NULL
>> -#endif
>
> I think you should keep this define, otherwise the pm pointer will
> always be valid.

Will correct it .
Thanks,

>
> --
> balbi