Patchwork [PATCHv7,00/23] I2C big cleanup

login
register
mail settings
Submitter Datta, Shubhrajyoti
Date Sept. 11, 2012, 9:41 a.m.
Message ID <1347356538-23835-1-git-send-email-shubhrajyoti@ti.com>
Download mbox
Permalink /patch/183073/
State New
Headers show

Pull-request

git://gitorious.org/linus-tree/linus-tree.git for_3.7/i2c/big_cleanup

Comments

Datta, Shubhrajyoti - Sept. 11, 2012, 9:41 a.m.
Changes since v1:
       - removed tabification on patch 6/17
       - removed dev_err() which was introduced on patch 09/17
Changes since v2:
        - do not set full fifo depth in the RDR interrupt.
        - some changelog updates.
        - rebase to the Wolfram's tree.
Changes since v3:
        - Remove a redundant read of status register
        - Read the dev->buf_len variable instead of the register
        as the information of the remaining bytes is there.
Changes since v4:
        - Ack the arbitration lost.
        - Rebase to the i2c-embedded/for-next branch.
Changes since v5:
        - Rebase to latest mainline
        - Added some more cleanup patches so as have a consolidated series.
Changes since v6:
        - Fix comments on setting the pdev to NULL.
        - Trivial changelog update

Previous discussions can be found here 
http://www.spinics.net/lists/linux-i2c/msg09482.html

This is the cleanup only series.
  
Tested on omap4sdp and 3430sdp.

The following changes since commit 55d512e245bc7699a8800e23df1a24195dd08217:

  Linux 3.6-rc5 (2012-09-08 16:43:45 -0700)

are available in the git repository at:
  git://gitorious.org/linus-tree/linus-tree.git for_3.7/i2c/big_cleanup



Felipe Balbi (22):
  i2c: omap: switch to devm_* API
  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()
  i2c: omap: split out [XR]DR and [XR]RDY
  i2c: omap: improve i462 errata handling
  i2c: omap: re-factor receive/transmit data loop
  i2c: omap: switch over to do {} while loop
  i2c: omap: ack IRQ in parts
  i2c: omap: switch to platform_get_irq()
  i2c: omap: bus: add a receiver flag
  i2c: omap: simplify errata check
  i2c: omap: always return IRQ_HANDLED
  i2c: omap: simplify IRQ exit path
  i2c: omap: resize fifos before each message
  i2c: omap: get rid of the "complete" label
  i2c: omap: always return IRQ_HANDLED
  i2c: omap: switch to threaded IRQ support
  i2c: omap: remove unnecessary pm_runtime_suspended check
  i2c: omap: switch over to autosuspend API
  i2c: omap: sanitize exit path

Shubhrajyoti D (1):
  i2c: omap: remove redundant status read

 drivers/i2c/busses/i2c-omap.c |  442 +++++++++++++++++++++++++----------------
 1 files changed, 271 insertions(+), 171 deletions(-)
Datta, Shubhrajyoti - Sept. 11, 2012, 10:18 a.m.
On Tuesday 11 September 2012 03:11 PM, Shubhrajyoti D wrote:
> Changes since v1:
>        - removed tabification on patch 6/17
>        - removed dev_err() which was introduced on patch 09/17
> Changes since v2:
>         - do not set full fifo depth in the RDR interrupt.
>         - some changelog updates.
>         - rebase to the Wolfram's tree.
> Changes since v3:
>         - Remove a redundant read of status register
>         - Read the dev->buf_len variable instead of the register
>         as the information of the remaining bytes is there.
> Changes since v4:
>         - Ack the arbitration lost.
>         - Rebase to the i2c-embedded/for-next branch.
> Changes since v5:
>         - Rebase to latest mainline
>         - Added some more cleanup patches so as have a consolidated series.
> Changes since v6:
>         - Fix comments on setting the pdev to NULL.
>         - Trivial changelog update
>
> Previous discussions can be found here 
> http://www.spinics.net/lists/linux-i2c/msg09482.html
Also this gives better performance
With the patches:

 Performance counter stats for '/build/i2c/bin/i2cdump -y -f 1 0x48
b':        
                                                                               

         78.796376 task-clock                #    0.453 CPUs
utilized          
               516 context-switches          #    0.007
M/sec                  
                 0 CPU-migrations            #    0.000
K/sec                  
               114 page-faults               #    0.001 M/sec  

      0.174011183 seconds time elapsed  

Without the patches
 Performance counter stats for '/build/i2c/bin/i2cdump -y -f 1 0x48
b':        
                                                                               

        123.504640 task-clock                #    0.049 CPUs
utilized          
               337 context-switches          #    0.003
M/sec                  
                 0 CPU-migrations            #    0.000
K/sec                  
               144 page-faults               #    0.001
M/sec                  
                              
                                                                               

       2.534424040 seconds time elapsed 

The auto suspend is probably the one  :-)

> This is the cleanup only series.
>   
> Tested on omap4sdp and 3430sdp.
>
> The following changes since commit 55d512e245bc7699a8800e23df1a24195dd08217:
>
>   Linux 3.6-rc5 (2012-09-08 16:43:45 -0700)
>
> are available in the git repository at:
>   git://gitorious.org/linus-tree/linus-tree.git for_3.7/i2c/big_cleanup
>
>
>
> Felipe Balbi (22):
>   i2c: omap: switch to devm_* API
>   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()
>   i2c: omap: split out [XR]DR and [XR]RDY
>   i2c: omap: improve i462 errata handling
>   i2c: omap: re-factor receive/transmit data loop
>   i2c: omap: switch over to do {} while loop
>   i2c: omap: ack IRQ in parts
>   i2c: omap: switch to platform_get_irq()
>   i2c: omap: bus: add a receiver flag
>   i2c: omap: simplify errata check
>   i2c: omap: always return IRQ_HANDLED
>   i2c: omap: simplify IRQ exit path
>   i2c: omap: resize fifos before each message
>   i2c: omap: get rid of the "complete" label
>   i2c: omap: always return IRQ_HANDLED
>   i2c: omap: switch to threaded IRQ support
>   i2c: omap: remove unnecessary pm_runtime_suspended check
>   i2c: omap: switch over to autosuspend API
>   i2c: omap: sanitize exit path
>
> Shubhrajyoti D (1):
>   i2c: omap: remove redundant status read
>
>  drivers/i2c/busses/i2c-omap.c |  442 +++++++++++++++++++++++++----------------
>  1 files changed, 271 insertions(+), 171 deletions(-)
>
Felipe Balbi - Sept. 11, 2012, 12:05 p.m.
Hi,

On Tue, Sep 11, 2012 at 03:48:34PM +0530, Shubhrajyoti wrote:
> On Tuesday 11 September 2012 03:11 PM, Shubhrajyoti D wrote:
> > Changes since v1:
> >        - removed tabification on patch 6/17
> >        - removed dev_err() which was introduced on patch 09/17
> > Changes since v2:
> >         - do not set full fifo depth in the RDR interrupt.
> >         - some changelog updates.
> >         - rebase to the Wolfram's tree.
> > Changes since v3:
> >         - Remove a redundant read of status register
> >         - Read the dev->buf_len variable instead of the register
> >         as the information of the remaining bytes is there.
> > Changes since v4:
> >         - Ack the arbitration lost.
> >         - Rebase to the i2c-embedded/for-next branch.
> > Changes since v5:
> >         - Rebase to latest mainline
> >         - Added some more cleanup patches so as have a consolidated series.
> > Changes since v6:
> >         - Fix comments on setting the pdev to NULL.
> >         - Trivial changelog update
> >
> > Previous discussions can be found here 
> > http://www.spinics.net/lists/linux-i2c/msg09482.html
> Also this gives better performance
> With the patches:
> 
>  Performance counter stats for '/build/i2c/bin/i2cdump -y -f 1 0x48
> b':        
>                                                                                
> 
>          78.796376 task-clock                #    0.453 CPUs
> utilized          
>                516 context-switches          #    0.007
> M/sec                  
>                  0 CPU-migrations            #    0.000
> K/sec                  
>                114 page-faults               #    0.001 M/sec  
> 
>       0.174011183 seconds time elapsed  
> 
> Without the patches
>  Performance counter stats for '/build/i2c/bin/i2cdump -y -f 1 0x48
> b':        
>                                                                                
> 
>         123.504640 task-clock                #    0.049 CPUs
> utilized          
>                337 context-switches          #    0.003
> M/sec                  
>                  0 CPU-migrations            #    0.000
> K/sec                  
>                144 page-faults               #    0.001
> M/sec                  
>                               
>                                                                                
> 
>        2.534424040 seconds time elapsed 
> 
> The auto suspend is probably the one  :-)

Awesome, I didn't go as far as checking performance counters :-)
Wolfram Sang - Sept. 11, 2012, 9:51 p.m.
On Tue, Sep 11, 2012 at 03:12:15PM +0530, Shubhrajyoti D wrote:
> From: Felipe Balbi <balbi@ti.com>
> 
> for OMAP2, we can easily switch over to threaded
> IRQs on the I2C driver. This will allow us to
> spend less time in hardirq context.
> 
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> [Trivial formating changes]
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
>  drivers/i2c/busses/i2c-omap.c |   43 +++++++++++++++++++++++++++++++++++-----
>  1 files changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 4a696bd..e391370 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -176,6 +176,7 @@ enum {
>  #define I2C_OMAP_ERRATA_I462		(1 << 1)
>  
>  struct omap_i2c_dev {
> +	spinlock_t		lock;		/* IRQ synchronization */
>  	struct device		*dev;
>  	void __iomem		*base;		/* virtual */
>  	int			irq;
> @@ -854,9 +855,30 @@ static int omap_i2c_transmit_data(struct omap_i2c_dev *dev, u8 num_bytes,
>  }
>  
>  static irqreturn_t
> -omap_i2c_isr(int this_irq, void *dev_id)
> +omap_i2c_isr(int irq, void *dev_id)
>  {
>  	struct omap_i2c_dev *dev = dev_id;
> +	irqreturn_t ret = IRQ_HANDLED;

Shouldn't that be IRQ_NONE?
Wolfram Sang - Sept. 11, 2012, 9:53 p.m.
On Tue, Sep 11, 2012 at 03:12:14PM +0530, Shubhrajyoti D wrote:
> From: Felipe Balbi <balbi@ti.com>
> 
> even if our clocks are disabled, we still
> handled the IRQ, so we should return IRQ_HANDLED.
> 
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>

Can't we just drop this patch since the code gets removed soon anyhow?
Or is patch 20/23 changing something I don't see yet?
Wolfram Sang - Sept. 11, 2012, 9:54 p.m.
On Tue, Sep 11, 2012 at 03:12:13PM +0530, Shubhrajyoti D wrote:
> Remove the redundant read of the status register.

Commit message is just repeating the subject.
Why was the read there and why can it be removed now?

> 
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
>  drivers/i2c/busses/i2c-omap.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 5d4bad4..498a462 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -946,7 +946,6 @@ omap_i2c_isr(int this_irq, void *dev_id)
>  				num_bytes = dev->buf_len;
>  
>  			ret = omap_i2c_transmit_data(dev, num_bytes, true);
> -			stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
>  			if (ret < 0)
>  				goto out;
>  
> @@ -962,7 +961,6 @@ omap_i2c_isr(int this_irq, void *dev_id)
>  				num_bytes = dev->threshold;
>  
>  			ret = omap_i2c_transmit_data(dev, num_bytes, false);
> -			stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
>  			if (ret < 0)
>  				goto out;
>  
> -- 
> 1.7.5.4
>
Wolfram Sang - Sept. 11, 2012, 10 p.m.
On Tue, Sep 11, 2012 at 03:11:55PM +0530, Shubhrajyoti D wrote:
> Changes since v1:
>        - removed tabification on patch 6/17
>        - removed dev_err() which was introduced on patch 09/17
> Changes since v2:
>         - do not set full fifo depth in the RDR interrupt.
>         - some changelog updates.
>         - rebase to the Wolfram's tree.
> Changes since v3:
>         - Remove a redundant read of status register
>         - Read the dev->buf_len variable instead of the register
>         as the information of the remaining bytes is there.
> Changes since v4:
>         - Ack the arbitration lost.
>         - Rebase to the i2c-embedded/for-next branch.
> Changes since v5:
>         - Rebase to latest mainline
>         - Added some more cleanup patches so as have a consolidated series.
> Changes since v6:
>         - Fix comments on setting the pdev to NULL.
>         - Trivial changelog update

Looks mostly good, thanks. Only a few comments. Oh, and I still get
reports about a section mismatch ;)
Datta, Shubhrajyoti - Sept. 12, 2012, 6:25 a.m.
On Wednesday 12 September 2012 03:21 AM, Wolfram Sang wrote:
>> -omap_i2c_isr(int this_irq, void *dev_id)
>> > +omap_i2c_isr(int irq, void *dev_id)
>> >  {
>> >  	struct omap_i2c_dev *dev = dev_id;
>> > +	irqreturn_t ret = IRQ_HANDLED;
> Shouldn't that be IRQ_NONE?
Actually we are processing it so I thought it to be ok.
Also a similar discussion.
 
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/104422.html
Datta, Shubhrajyoti - Sept. 12, 2012, 6:42 a.m.
On Wednesday 12 September 2012 03:23 AM, Wolfram Sang wrote:
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> Can't we just drop this patch since the code gets removed soon anyhow?
> Or is patch 20/23 changing something I don't see yet?
yes it is dropped will update.
Datta, Shubhrajyoti - Sept. 12, 2012, 6:51 a.m.
On Wednesday 12 September 2012 03:24 AM, Wolfram Sang wrote:
> On Tue, Sep 11, 2012 at 03:12:13PM +0530, Shubhrajyoti D wrote:
>> Remove the redundant read of the status register.
> Commit message is just repeating the subject.
> Why was the read there and why can it be removed now?
>
It is read and it is not read anywhere below.
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
>> ---
>>  drivers/i2c/busses/i2c-omap.c |    2 --
>>  1 files changed, 0 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>> index 5d4bad4..498a462 100644
>> --- a/drivers/i2c/busses/i2c-omap.c
>> +++ b/drivers/i2c/busses/i2c-omap.c
>> @@ -946,7 +946,6 @@ omap_i2c_isr(int this_irq, void *dev_id)
>>  				num_bytes = dev->buf_len;
>>  
>>  			ret = omap_i2c_transmit_data(dev, num_bytes, true);
>> -			stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
>>  			if (ret < 0)
>>  				goto out;
>>  
>> @@ -962,7 +961,6 @@ omap_i2c_isr(int this_irq, void *dev_id)
>>  				num_bytes = dev->threshold;
>>  
>>  			ret = omap_i2c_transmit_data(dev, num_bytes, false);
>> -			stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
>>  			if (ret < 0)
>>  				goto out;
>>  
>> -- 
>> 1.7.5.4
>>
Datta, Shubhrajyoti - Sept. 12, 2012, 10:11 a.m.
On Wednesday 12 September 2012 03:30 AM, Wolfram Sang wrote:
>> Changes since v6:
>> >         - Fix comments on setting the pdev to NULL.
>> >         - Trivial changelog update
> Looks mostly good, thanks. Only a few comments. Oh, and I still get
> reports about a section mismatch ;)
I am using omap2plus_defconfig
 $ make --ver
GNU Make 3.81
Copyright (C) 2006  Free Software Foundation, Inc.
This is free software; see the source for copying conditions.
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
PARTICULAR PURPOSE.

$ make CONFIG_DEBUG_SECTION_MISMATCH=y uImage
....
  LD      vmlinux.o
  MODPOST vmlinux.o
  GEN     .version
  CHK     include/generated/compile.h
  UPD     include/generated/compile.h
  CC      init/version.o
  LD      init/built-in.o
  KSYM    .tmp_kallsyms1.o
  KSYM    .tmp_kallsyms2.o
  LD      vmlinux
  SYSMAP  System.map
  OBJCOPY arch/arm/boot/Image
  Kernel: arch/arm/boot/Image is ready
  GZIP    arch/arm/boot/compressed/piggy.gzip
  CC      arch/arm/boot/compressed/misc.o
  CC      arch/arm/boot/compressed/decompress.o
  CC      arch/arm/boot/compressed/string.o
  AS      arch/arm/boot/compressed/piggy.gzip.o
  LD      arch/arm/boot/compressed/vmlinux
  OBJCOPY arch/arm/boot/zImage
  Kernel: arch/arm/boot/zImage is ready
  UIMAGE  arch/arm/boot/uImage
Image Name:   Linux-3.6.0-rc5-00022-g49fb6db
Created:      Wed Sep 12 15:24:21 2012
Image Type:   ARM Linux Kernel Image (uncompressed)
Data Size:    4074688 Bytes = 3979.19 kB = 3.89 MB
Load Address: 80008000
Entry Point:  80008000
  Image arch/arm/boot/uImage is ready

I donot see the warning. Am I missing something?
Wolfram Sang - Sept. 12, 2012, 10:18 a.m.
> I donot see the warning. Am I missing something?

I deleted my logfiles already. Ignore it for now, if it comes up again
with your new series, I will give a more detailed pointer.

Thanks,

   Wolfram
Wolfram Sang - Sept. 12, 2012, 11:51 a.m.
On Wed, Sep 12, 2012 at 11:55:39AM +0530, Shubhrajyoti wrote:
> On Wednesday 12 September 2012 03:21 AM, Wolfram Sang wrote:
> >> -omap_i2c_isr(int this_irq, void *dev_id)
> >> > +omap_i2c_isr(int irq, void *dev_id)
> >> >  {
> >> >  	struct omap_i2c_dev *dev = dev_id;
> >> > +	irqreturn_t ret = IRQ_HANDLED;
> > Shouldn't that be IRQ_NONE?
> Actually we are processing it so I thought it to be ok.

Are we processing it? There is nothing acknowledged AFAICS. Anyway, we
can fix that with a later patch.

> Also a similar discussion.
>  
> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/104422.html

I totally agree to the things said there.
Wolfram Sang - Sept. 12, 2012, 1:44 p.m.
On Wed, Sep 12, 2012 at 12:18:50PM +0200, Wolfram Sang wrote:
> 
> > I donot see the warning. Am I missing something?
> 
> I deleted my logfiles already. Ignore it for now, if it comes up again
> with your new series, I will give a more detailed pointer.

Sorry, the section mismatch was not related to I2C it seems:

WARNING: vmlinux.o(.data+0x30958): Section mismatch in reference from the variable rx51_si4713_dev to the (unknown reference) .init.data:(unknown)
The variable rx51_si4713_dev references
the (unknown reference) __initdata (unknown)
If the reference is valid then annotate the
variable with __init* or __refdata (see linux/init.h) or name the variable:
*_template, *_timer, *_sht, *_ops, *_probe, *_probe_one, *_console

Got it with an "allnoconfig" and then selecting MMU and OMAP.

Regards,

   Wolfram
Shubhrajyoti Datta - Sept. 12, 2012, 2:40 p.m.
On Wed, Sep 12, 2012 at 7:14 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> On Wed, Sep 12, 2012 at 12:18:50PM +0200, Wolfram Sang wrote:
>>
>> > I donot see the warning. Am I missing something?
>>
>> I deleted my logfiles already. Ignore it for now, if it comes up again
>> with your new series, I will give a more detailed pointer.
>
> Sorry, the section mismatch was not related to I2C it seems:

Thanks for the report just sent a patch fixing that.

>
> WARNING: vmlinux.o(.data+0x30958): Section mismatch in reference from the variable rx51_si4713_dev to the (unknown reference) .init.data:(unknown)
> The variable rx51_si4713_dev references
> the (unknown reference) __initdata (unknown)
> If the reference is valid then annotate the
> variable with __init* or __refdata (see linux/init.h) or name the variable:
> *_template, *_timer, *_sht, *_ops, *_probe, *_probe_one, *_console
>
> Got it with an "allnoconfig" and then selecting MMU and OMAP.
>
> Regards,
>
>    Wolfram
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
>
> iEYEARECAAYFAlBQkbsACgkQD27XaX1/VRtbuACgkBa0lOIN551eec9TSetVPsCE
> Ew0AoKizKon3DIILpERWJIwzAXdgRVDc
> =T4Yq
> -----END PGP SIGNATURE-----
>