[PATCH v2 07/10] TWL CORE: Fix sparse warning

[PATCH v2 07/10] TWL CORE: Fix sparse warning

[Samuel Ortiz]

Hi Manjunath,

On Tue, Sep 21, 2010 at 03:31:17PM +0530, G, Manjunath Kondaiah wrote:
> Fixes below sparse warning.
> 
> drivers/mfd/twl-core.c:258:20: warning: symbol 'twl_map' was not declared. Should it be static?
> 
Patch applied, thanks.

Cheers,
Samuel.

> Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com>
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Nishanth Menon <nm@ti.com>
> ---
>  drivers/mfd/twl-core.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
> index 720e099..53c371a 100644
> --- a/drivers/mfd/twl-core.c
> +++ b/drivers/mfd/twl-core.c
> @@ -255,7 +255,7 @@ struct twl_mapping {
>  	unsigned char sid;	/* Slave ID */
>  	unsigned char base;	/* base address */
>  };
> -struct twl_mapping *twl_map;
> +static struct twl_mapping *twl_map;
>  
>  static struct twl_mapping twl4030_map[TWL4030_MODULE_LAST + 1] = {
>  	/*
> -- 
> 1.7.0.4
> 

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH v2 08/10] TWL IRQ: Fix fucntion declaration warnings

[Samuel Ortiz]

Hi Manjunath,

On Tue, Sep 21, 2010 at 03:31:18PM +0530, G, Manjunath Kondaiah wrote:
> Fixes following sparse warnings for twl4030 and twl6030 irq files.
> 
> drivers/mfd/twl4030-irq.c:783:5: warning: symbol 'twl4030_init_irq' was not declared. Should it be static?
> drivers/mfd/twl4030-irq.c:863:5: warning: symbol 'twl4030_exit_irq' was not declared. Should it be static?
> drivers/mfd/twl4030-irq.c:873:5: warning: symbol 'twl4030_init_chip_irq' was not declared. Should it be static?
> 
> drivers/mfd/twl6030-irq.c:226:5: warning: symbol 'twl6030_init_irq' was not declared. Should it be static?
> drivers/mfd/twl6030-irq.c:290:5: warning: symbol 'twl6030_exit_irq' was not declared. Should it be static?
> 
> Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com>
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Nishanth Menon <nm@ti.com>
> ---
>  include/linux/i2c/twl.h |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/i2c/twl.h b/include/linux/i2c/twl.h
> index 6de90bf..c61e90a 100644
> --- a/include/linux/i2c/twl.h
> +++ b/include/linux/i2c/twl.h
> @@ -172,6 +172,11 @@ int twl_i2c_read(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes);
>  
>  int twl6030_interrupt_unmask(u8 bit_mask, u8 offset);
>  int twl6030_interrupt_mask(u8 bit_mask, u8 offset);
> +int twl6030_init_irq(int irq_num, unsigned irq_base, unsigned irq_end);
> +int twl6030_exit_irq(void);
> +int twl4030_init_irq(int irq_num, unsigned irq_base, unsigned irq_end);
> +int twl4030_exit_irq(void);
> +int twl4030_init_chip_irq(const char *chip);
No, we don't want to export those. Try defining them as extern from
twl*-irq.c.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH v2 08/10] TWL IRQ: Fix fucntion declaration warnings

[G, Manjunath Kondaiah]

Hi Samuel,

> -----Original Message-----
> From: Samuel Ortiz [mailto:sameo at linux.intel.com] 
> Sent: Monday, September 27, 2010 4:46 PM
> To: G, Manjunath Kondaiah
> Cc: linux-omap at vger.kernel.org; 
> linux-arm-kernel at lists.infradead.org; Tony Lindgren; Menon, Nishanth
> Subject: Re: [PATCH v2 08/10] TWL IRQ: Fix fucntion 
> declaration warnings
> 
> Hi Manjunath,
> 
> On Tue, Sep 21, 2010 at 03:31:18PM +0530, G, Manjunath Kondaiah wrote:
> > Fixes following sparse warnings for twl4030 and twl6030 irq files.
> > 
> > drivers/mfd/twl4030-irq.c:783:5: warning: symbol 
> 'twl4030_init_irq' was not declared. Should it be static?
> > drivers/mfd/twl4030-irq.c:863:5: warning: symbol 
> 'twl4030_exit_irq' was not declared. Should it be static?
> > drivers/mfd/twl4030-irq.c:873:5: warning: symbol 
> 'twl4030_init_chip_irq' was not declared. Should it be static?
> > 
> > drivers/mfd/twl6030-irq.c:226:5: warning: symbol 
> 'twl6030_init_irq' was not declared. Should it be static?
> > drivers/mfd/twl6030-irq.c:290:5: warning: symbol 
> 'twl6030_exit_irq' was not declared. Should it be static?
> > 
> > Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com>
> > Cc: linux-arm-kernel at lists.infradead.org
> > Cc: Samuel Ortiz <sameo@linux.intel.com>
> > Cc: Tony Lindgren <tony@atomide.com>
> > Cc: Nishanth Menon <nm@ti.com>
> > ---
> >  include/linux/i2c/twl.h |    5 +++++
> >  1 files changed, 5 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/linux/i2c/twl.h 
> b/include/linux/i2c/twl.h index 
> > 6de90bf..c61e90a 100644
> > --- a/include/linux/i2c/twl.h
> > +++ b/include/linux/i2c/twl.h
> > @@ -172,6 +172,11 @@ int twl_i2c_read(u8 mod_no, u8 *value, u8 reg, 
> > unsigned num_bytes);
> >  
> >  int twl6030_interrupt_unmask(u8 bit_mask, u8 offset);  int 
> > twl6030_interrupt_mask(u8 bit_mask, u8 offset);
> > +int twl6030_init_irq(int irq_num, unsigned irq_base, unsigned 
> > +irq_end); int twl6030_exit_irq(void); int twl4030_init_irq(int 
> > +irq_num, unsigned irq_base, unsigned irq_end); int 
> > +twl4030_exit_irq(void); int twl4030_init_chip_irq(const 
> char *chip);
> No, we don't want to export those. Try defining them as 
> extern from twl*-irq.c.

Having extern in .c file will generate checkpatch warning as:
WARNING: externs should be avoided in .c files

-Manjunath

[PATCH v2 08/10] TWL IRQ: Fix fucntion declaration warnings

[G, Manjunath Kondaiah]

 

> -----Original Message-----
> From: linux-omap-owner at vger.kernel.org 
> [mailto:linux-omap-owner at vger.kernel.org] On Behalf Of G, 
> Manjunath Kondaiah
> Sent: Monday, September 27, 2010 6:40 PM
> To: Samuel Ortiz
> Cc: linux-omap at vger.kernel.org; 
> linux-arm-kernel at lists.infradead.org; Tony Lindgren; Menon, Nishanth
> Subject: RE: [PATCH v2 08/10] TWL IRQ: Fix fucntion 
> declaration warnings
> 
> 
> Hi Samuel,
> 
> > -----Original Message-----
> > From: Samuel Ortiz [mailto:sameo at linux.intel.com]
> > Sent: Monday, September 27, 2010 4:46 PM
> > To: G, Manjunath Kondaiah
> > Cc: linux-omap at vger.kernel.org;
> > linux-arm-kernel at lists.infradead.org; Tony Lindgren; Menon, Nishanth
> > Subject: Re: [PATCH v2 08/10] TWL IRQ: Fix fucntion declaration 
> > warnings
> > 
> > Hi Manjunath,
> > 
> > On Tue, Sep 21, 2010 at 03:31:18PM +0530, G, Manjunath 
> Kondaiah wrote:
> > > Fixes following sparse warnings for twl4030 and twl6030 irq files.
> > > 
> > > drivers/mfd/twl4030-irq.c:783:5: warning: symbol
> > 'twl4030_init_irq' was not declared. Should it be static?
> > > drivers/mfd/twl4030-irq.c:863:5: warning: symbol
> > 'twl4030_exit_irq' was not declared. Should it be static?
> > > drivers/mfd/twl4030-irq.c:873:5: warning: symbol
> > 'twl4030_init_chip_irq' was not declared. Should it be static?
> > > 
> > > drivers/mfd/twl6030-irq.c:226:5: warning: symbol
> > 'twl6030_init_irq' was not declared. Should it be static?
> > > drivers/mfd/twl6030-irq.c:290:5: warning: symbol
> > 'twl6030_exit_irq' was not declared. Should it be static?
> > > 
> > > Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com>
> > > Cc: linux-arm-kernel at lists.infradead.org
> > > Cc: Samuel Ortiz <sameo@linux.intel.com>
> > > Cc: Tony Lindgren <tony@atomide.com>
> > > Cc: Nishanth Menon <nm@ti.com>
> > > ---
> > >  include/linux/i2c/twl.h |    5 +++++
> > >  1 files changed, 5 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/include/linux/i2c/twl.h
> > b/include/linux/i2c/twl.h index
> > > 6de90bf..c61e90a 100644
> > > --- a/include/linux/i2c/twl.h
> > > +++ b/include/linux/i2c/twl.h
> > > @@ -172,6 +172,11 @@ int twl_i2c_read(u8 mod_no, u8 
> *value, u8 reg, 
> > > unsigned num_bytes);
> > >  
> > >  int twl6030_interrupt_unmask(u8 bit_mask, u8 offset);  int
> > > twl6030_interrupt_mask(u8 bit_mask, u8 offset);
> > > +int twl6030_init_irq(int irq_num, unsigned irq_base, unsigned 
> > > +irq_end); int twl6030_exit_irq(void); int twl4030_init_irq(int 
> > > +irq_num, unsigned irq_base, unsigned irq_end); int 
> > > +twl4030_exit_irq(void); int twl4030_init_chip_irq(const
> > char *chip);
> > No, we don't want to export those. Try defining them as extern from 
> > twl*-irq.c.
> 
> Having extern in .c file will generate checkpatch warning as:
> WARNING: externs should be avoided in .c files

As an alternate, how about having twl-core.h in drivers/mfd and defining them
as extern so that these API's will be available only to files under drivers/mfd?

-Manjunath

[PATCH v2 08/10] TWL IRQ: Fix fucntion declaration warnings

[Samuel Ortiz]

Hi Manjunath,

On Mon, Sep 27, 2010 at 07:19:33PM +0530, G, Manjunath Kondaiah wrote:
> > > No, we don't want to export those. Try defining them as extern from 
> > > twl*-irq.c.
> > 
> > Having extern in .c file will generate checkpatch warning as:
> > WARNING: externs should be avoided in .c files
> 
> As an alternate, how about having twl-core.h in drivers/mfd and defining them
> as extern so that these API's will be available only to files under drivers/mfd?
>
That would be better, yes. Even though exporting those definititions through
linux/i2c/twl.h is harmless.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH v2 03/10] OMAP: mach-omap2: Fix static function warnings

[Paul Walmsley]

Hello Manjunath,

one comment:

On Tue, 21 Sep 2010, G, Manjunath Kondaiah wrote:

>  arch/arm/mach-omap2/powerdomain.c              |   28 ------------------------
>  12 files changed, 27 insertions(+), 47 deletions(-)
>  create mode 100644 arch/arm/mach-omap2/include/mach/board-rx51.h
> 
> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
> index 6527ec3..6e51079 100644
> --- a/arch/arm/mach-omap2/powerdomain.c
> +++ b/arch/arm/mach-omap2/powerdomain.c
> @@ -981,34 +981,6 @@ bool pwrdm_has_hdwr_sar(struct powerdomain *pwrdm)
>  }
>  
>  /**
> - * pwrdm_set_lowpwrstchange - Request a low power state change
> - * @pwrdm: struct powerdomain *
> - *
> - * Allows a powerdomain to transtion to a lower power sleep state
> - * from an existing sleep state without waking up the powerdomain.
> - * Returns -EINVAL if the powerdomain pointer is null or if the
> - * powerdomain does not support LOWPOWERSTATECHANGE, or returns 0
> - * upon success.
> - */
> -int pwrdm_set_lowpwrstchange(struct powerdomain *pwrdm)
> -{
> -	if (!pwrdm)
> -		return -EINVAL;
> -
> -	if (!(pwrdm->flags & PWRDM_HAS_LOWPOWERSTATECHANGE))
> -		return -EINVAL;
> -
> -	pr_debug("powerdomain: %s: setting LOWPOWERSTATECHANGE bit\n",
> -		 pwrdm->name);
> -
> -	prm_rmw_mod_reg_bits(OMAP4430_LOWPOWERSTATECHANGE_MASK,
> -			     (1 << OMAP4430_LOWPOWERSTATECHANGE_SHIFT),
> -			     pwrdm->prcm_offs, pwrstctrl_reg_offs);
> -
> -	return 0;
> -}
> -

Please don't delete this function.  It will be needed for OMAP4 power 
management.  Instead, just add a prototype in 
plat-omap/include/plat/powerdomains.h.


- Paul

[PATCH v2 03/10] OMAP: mach-omap2: Fix static function warnings

[G, Manjunath Kondaiah]

Hi Paul,

> -----Original Message-----
> From: Paul Walmsley [mailto:paul at pwsan.com] 
> Sent: Thursday, September 30, 2010 3:06 AM
> To: G, Manjunath Kondaiah
> Cc: linux-omap at vger.kernel.org; 
> linux-arm-kernel at lists.infradead.org; Tony Lindgren; Menon, Nishanth
> Subject: Re: [PATCH v2 03/10] OMAP: mach-omap2: Fix static 
> function warnings
> 
> Hello Manjunath,
> 
> one comment:
> 
> On Tue, 21 Sep 2010, G, Manjunath Kondaiah wrote:
> 
> >  arch/arm/mach-omap2/powerdomain.c              |   28 
> ------------------------
> >  12 files changed, 27 insertions(+), 47 deletions(-)  create mode 
> > 100644 arch/arm/mach-omap2/include/mach/board-rx51.h
> > 
> > diff --git a/arch/arm/mach-omap2/powerdomain.c 
> > b/arch/arm/mach-omap2/powerdomain.c
> > index 6527ec3..6e51079 100644
> > --- a/arch/arm/mach-omap2/powerdomain.c
> > +++ b/arch/arm/mach-omap2/powerdomain.c
> > @@ -981,34 +981,6 @@ bool pwrdm_has_hdwr_sar(struct powerdomain 
> > *pwrdm)  }
> >  
> >  /**
> > - * pwrdm_set_lowpwrstchange - Request a low power state change
> > - * @pwrdm: struct powerdomain *
> > - *
> > - * Allows a powerdomain to transtion to a lower power sleep state
> > - * from an existing sleep state without waking up the powerdomain.
> > - * Returns -EINVAL if the powerdomain pointer is null or if the
> > - * powerdomain does not support LOWPOWERSTATECHANGE, or returns 0
> > - * upon success.
> > - */
> > -int pwrdm_set_lowpwrstchange(struct powerdomain *pwrdm) -{
> > -	if (!pwrdm)
> > -		return -EINVAL;
> > -
> > -	if (!(pwrdm->flags & PWRDM_HAS_LOWPOWERSTATECHANGE))
> > -		return -EINVAL;
> > -
> > -	pr_debug("powerdomain: %s: setting LOWPOWERSTATECHANGE bit\n",
> > -		 pwrdm->name);
> > -
> > -	prm_rmw_mod_reg_bits(OMAP4430_LOWPOWERSTATECHANGE_MASK,
> > -			     (1 << OMAP4430_LOWPOWERSTATECHANGE_SHIFT),
> > -			     pwrdm->prcm_offs, pwrstctrl_reg_offs);
> > -
> > -	return 0;
> > -}
> > -
> 
> Please don't delete this function.  It will be needed for 
> OMAP4 power management.  Instead, just add a prototype in 
> plat-omap/include/plat/powerdomains.h.

Thanks. Taken care with new pull request sent to tony.

-Manjunath

[PATCH v2 09/10] OMAP2/3: Convert write/read functions to raw read/write

[Menon, Nishanth]

> -----Original Message-----
> From: linux-omap-owner at vger.kernel.org [mailto:linux-omap-
> owner at vger.kernel.org] On Behalf Of G, Manjunath Kondaiah
> Sent: Tuesday, September 21, 2010 5:01 AM
> To: linux-omap at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org; linux-mtd at lists.infradead.org
> Subject: [PATCH v2 09/10] OMAP2/3: Convert write/read functions to raw
> read/write
> 
> Following sparse warnings exists due to use of writel/w and readl/w
> functions.
> 
> This patch fixes the sparse warnings by converting readl/w functions usage
> into
> __raw_readl/__raw_readw functions.

Apologies on bringing up an old topic here -> Is'nt it better to fix readl/w or writel/w than replacing it with __raw_readl/w etc?



Regards,
Nishanth Menon

[...]

[PATCH v2 09/10] OMAP2/3: Convert write/read functions to raw read/write

[Russell King - ARM Linux]

On Thu, Oct 07, 2010 at 07:17:08AM -0500, Menon, Nishanth wrote:
> 
> > -----Original Message-----
> > From: linux-omap-owner at vger.kernel.org [mailto:linux-omap-
> > owner at vger.kernel.org] On Behalf Of G, Manjunath Kondaiah
> > Sent: Tuesday, September 21, 2010 5:01 AM
> > To: linux-omap at vger.kernel.org
> > Cc: linux-arm-kernel at lists.infradead.org; linux-mtd at lists.infradead.org
> > Subject: [PATCH v2 09/10] OMAP2/3: Convert write/read functions to raw
> > read/write
> > 
> > Following sparse warnings exists due to use of writel/w and readl/w
> > functions.
> > 
> > This patch fixes the sparse warnings by converting readl/w functions usage
> > into
> > __raw_readl/__raw_readw functions.
> 
> Apologies on bringing up an old topic here -> Is'nt it better to fix
> readl/w or writel/w than replacing it with __raw_readl/w etc?

No.  If you're getting sparse warnings its because _you_ are using
readl/writel wrongly.

They take a void __iomem pointer, not a u32, unsigned long, int, or
even a void pointer.

[PATCH v2 09/10] OMAP2/3: Convert write/read functions to raw read/write

[Nishanth Menon]

Russell King - ARM Linux had written, on 10/07/2010 01:56 PM, the following:
> On Thu, Oct 07, 2010 at 07:17:08AM -0500, Menon, Nishanth wrote:
>>> -----Original Message-----
>>> From: linux-omap-owner at vger.kernel.org [mailto:linux-omap-
>>> owner at vger.kernel.org] On Behalf Of G, Manjunath Kondaiah
>>> Sent: Tuesday, September 21, 2010 5:01 AM
>>> To: linux-omap at vger.kernel.org
>>> Cc: linux-arm-kernel at lists.infradead.org; linux-mtd at lists.infradead.org
>>> Subject: [PATCH v2 09/10] OMAP2/3: Convert write/read functions to raw
>>> read/write
>>>
>>> Following sparse warnings exists due to use of writel/w and readl/w
>>> functions.
>>>
>>> This patch fixes the sparse warnings by converting readl/w functions usage
>>> into
>>> __raw_readl/__raw_readw functions.
>> Apologies on bringing up an old topic here -> Is'nt it better to fix
>> readl/w or writel/w than replacing it with __raw_readl/w etc?
> 
> No.  If you're getting sparse warnings its because _you_ are using
> readl/writel wrongly.
> 
> They take a void __iomem pointer, not a u32, unsigned long, int, or
> even a void pointer.
void __iomem *p;
...

readl(p);

unrolls to:
({ u32 __v = ({ u32 __v = (( __u32)(__le32)(( __le32) ((void)0, 
*(volatile unsigned int *)((p))))); __v; }); __asm__ __volatile__ ("mcr 
p15,
, %0, c7, c10, 5" : : "r" (0) : "memory"); __v; });

({ u32 __v = ({ u32 __v

seems to be the obvious cause of sparse warnings such as that attempted 
to be fixed in [1]

  warning: symbol '__v' shadows an earlier one

my comment being that by moving {read,write}[wlb] to __raw versions dont 
solve the real issue of namespace here. fix should be in 
arch/arm/include/asm/io.h IMHO. so that there are no overlaps as this.

[1]http://marc.info/?l=linux-omap&m=128506333803725&w=2

-- 
Regards,
Nishanth Menon

[PATCH v2 09/10] OMAP2/3: Convert write/read functions to raw read/write

[David Woodhouse]

On Thu, 2010-10-07 at 14:50 -0500, Nishanth Menon wrote:
> my comment being that by moving {read,write}[wlb] to __raw versions dont 
> solve the real issue of namespace here. fix should be in 
> arch/arm/include/asm/io.h IMHO. so that there are no overlaps as this. 

Indeed. This patch is complete nonsense.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse at intel.com                              Intel Corporation

[PATCH v2 09/10] OMAP2/3: Convert write/read functions to raw read/write

[G, Manjunath Kondaiah]

David,

> -----Original Message-----
> From: David Woodhouse [mailto:dwmw2 at infradead.org] 
> Sent: Monday, October 25, 2010 5:32 AM
> To: Menon, Nishanth
> Cc: Russell King - ARM Linux; G, Manjunath Kondaiah; 
> linux-omap at vger.kernel.org; linux-mtd at lists.infradead.org; 
> linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH v2 09/10] OMAP2/3: Convert write/read 
> functions to raw read/write
> 
> On Thu, 2010-10-07 at 14:50 -0500, Nishanth Menon wrote:
> > my comment being that by moving {read,write}[wlb] to __raw versions 
> > dont solve the real issue of namespace here. fix should be in 
> > arch/arm/include/asm/io.h IMHO. so that there are no 
> overlaps as this.
> 
> Indeed. This patch is complete nonsense.

nonsense? There are two types of fixes proposed to fix these sparse warnings.
1. with this patch
2. fixing in io.h
https://patchwork.kernel.org/patch/250171/

1st option is already merged with 2.6.37 merge window.

If 2 get accepted, all __raw_read/write will get replaced with readl/writel.

Apart from those two, do you have any other alternate idea to fix these sparse warnings?

-Manjunath

[PATCH v2 09/10] OMAP2/3: Convert write/read functions to raw read/write

[David Woodhouse]

On Mon, 2010-10-25 at 11:04 +0530, G, Manjunath Kondaiah wrote:
> David,
> 
> > -----Original Message-----
> > From: David Woodhouse [mailto:dwmw2 at infradead.org] 
> > Sent: Monday, October 25, 2010 5:32 AM
> > To: Menon, Nishanth
> > Cc: Russell King - ARM Linux; G, Manjunath Kondaiah; 
> > linux-omap at vger.kernel.org; linux-mtd at lists.infradead.org; 
> > linux-arm-kernel at lists.infradead.org
> > Subject: Re: [PATCH v2 09/10] OMAP2/3: Convert write/read 
> > functions to raw read/write
> > 
> > On Thu, 2010-10-07 at 14:50 -0500, Nishanth Menon wrote:
> > > my comment being that by moving {read,write}[wlb] to __raw versions 
> > > dont solve the real issue of namespace here. fix should be in 
> > > arch/arm/include/asm/io.h IMHO. so that there are no 
> > overlaps as this.
> > 
> > Indeed. This patch is complete nonsense.
> 
> nonsense? There are two types of fixes proposed to fix these sparse warnings.
> 1. with this patch
> 2. fixing in io.h
> https://patchwork.kernel.org/patch/250171/
> 
> 1st option is already merged with 2.6.37 merge window.
> 
> If 2 get accepted, all __raw_read/write will get replaced with readl/writel.
> 
> Apart from those two, do you have any other alternate idea to fix these sparse warnings?

The latter is obviously the better approach. The problem that sparse is
complaining about is the fact that you have two nested C blocks, both of
which contain a local variable called '__v'. By changing the variable
names so that they're actually unique, we fix the real problem.

The answer is not that *all* drivers which call cpu_to_le16(readw())
must be changed, which is what you seem to have assumed. And you
*certainly* can't change them to use __raw_readw(), which has
*different* semantics (and endianness in some cases), without a clear
comment in the changelog entry saying *why* that change was OK.

-- 
dwmw2

[PATCH v2 09/10] OMAP2/3: Convert write/read functions to raw read/write

[Artem Bityutskiy]

On Mon, 2010-10-25 at 01:01 +0100, David Woodhouse wrote:
> On Thu, 2010-10-07 at 14:50 -0500, Nishanth Menon wrote:
> > my comment being that by moving {read,write}[wlb] to __raw versions dont 
> > solve the real issue of namespace here. fix should be in 
> > arch/arm/include/asm/io.h IMHO. so that there are no overlaps as this. 
> 
> Indeed. This patch is complete nonsense.

Removing from my l2 tree.

-- 
Best Regards,
Artem Bityutskiy (????? ????????)

[PATCH v2 09/10] OMAP2/3: Convert write/read functions to raw read/write

[Vimal Singh]

On Tue, Sep 21, 2010 at 3:31 PM, G, Manjunath Kondaiah <manjugk@ti.com> wrote:
> Following sparse warnings exists due to use of writel/w and readl/w functions.
>
> This patch fixes the sparse warnings by converting readl/w functions usage into
> __raw_readl/__raw_readw functions.
>
[...]
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -481,7 +481,7 @@ static int omap_verify_buf(struct mtd_info *mtd, const u_char * buf, int len)
>
> ? ? ? ?len >>= 1;
> ? ? ? ?while (len--) {
> - ? ? ? ? ? ? ? if (*p++ != cpu_to_le16(readw(info->nand.IO_ADDR_R)))
> + ? ? ? ? ? ? ? if (*p++ != cpu_to_le16(__raw_readw(info->nand.IO_ADDR_R)))

There was an old comment to remove use of 'cpu_to_le16' from driver, I
just missed it. Can you rather use 'ioread16_rep' for reading data.

-- 
Regards,
Vimal Singh

[PATCH v2 01/10] OMAP: mach-omap2: Fix incorrect assignment warnings

[Kevin Hilman]

"G, Manjunath Kondaiah" <manjugk@ti.com> writes:

> This patch fixes below sparse warnings for incorrect assignments.

As pointed out by Jean, this patch fixed some sparse warnings, but also
broke some things, specifically off mode.

In the future, *please* be sure to test the code paths that are being
changed.  This patch changed some code that is only exercised during
off-mode, but was clearly not tested with off mode enabled.

As background for why this broke functionality, keep this in mind:

 	void *a = NULL;
	u32 *b = NULL;

a + 1 = 1
b + 1 = 4

IOW, you cannot simply replace a 'u32 *' by a 'void *' without checking
and fixing any pointer arithmetic.

[...]

> diff --git a/arch/arm/mach-omap2/control.c b/arch/arm/mach-omap2/control.c
> index a8d20ee..7405936 100644
> --- a/arch/arm/mach-omap2/control.c
> +++ b/arch/arm/mach-omap2/control.c
> @@ -190,7 +190,7 @@ void omap_ctrl_writel(u32 val, u16 offset)
>  void omap3_clear_scratchpad_contents(void)
>  {
>  	u32 max_offset = OMAP343X_SCRATCHPAD_ROM_OFFSET;
> -	u32 *v_addr;
> +	void __iomem *v_addr;
>  	u32 offset = 0;
>  	v_addr = OMAP2_L4_IO_ADDRESS(OMAP343X_SCRATCHPAD_ROM);
>  	if (prm_read_mod_reg(OMAP3430_GR_MOD, OMAP3_PRM_RSTST_OFFSET) &

Interestingly, this one not only fixed the sparse warning but also fixed
a bug. :) Before this change, only every 4th entry of the scratchpad was
zeroed.

> @@ -206,7 +206,7 @@ void omap3_clear_scratchpad_contents(void)
>  /* Populate the scratchpad structure with restore structure */
>  void omap3_save_scratchpad_contents(void)
>  {
> -	void * __iomem scratchpad_address;
> +	void  __iomem *scratchpad_address;
>  	u32 arm_context_addr;
>  	struct omap3_scratchpad scratchpad_contents;
>  	struct omap3_scratchpad_prcm_block prcm_block_contents;

This one is fine.

> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 7b03426..4af19a6 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -316,7 +316,7 @@ static void restore_control_register(u32 val)
>  /* Function to restore the table entry that was modified for enabling MMU */
>  static void restore_table_entry(void)
>  {
> -	u32 *scratchpad_address;
> +	void __iomem *scratchpad_address;
>  	u32 previous_value, control_reg_value;
>  	u32 *address;

This one changes the result of any 'scratchpad_address + foo'.  

So, if this is changed from u32 to void, all the offsets used when
adding to the base need to be changed as well.

Kevin

[PATCH v2 01/10] OMAP: mach-omap2: Fix incorrect assignment warnings

[G, Manjunath Kondaiah]

 

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman at deeprootsystems.com] 
> Sent: Saturday, October 09, 2010 1:43 AM
> To: G, Manjunath Kondaiah
> Cc: linux-omap at vger.kernel.org; 
> linux-arm-kernel at lists.infradead.org; Tony Lindgren; Menon, Nishanth
> Subject: Re: [PATCH v2 01/10] OMAP: mach-omap2: Fix incorrect 
> assignment warnings
> 
> "G, Manjunath Kondaiah" <manjugk@ti.com> writes:
> 
> > This patch fixes below sparse warnings for incorrect assignments.
> 
> As pointed out by Jean, this patch fixed some sparse 
> warnings, but also broke some things, specifically off mode.
> 
> In the future, *please* be sure to test the code paths that 
> are being changed.  This patch changed some code that is only 
> exercised during off-mode, but was clearly not tested with 
> off mode enabled.
> 
> As background for why this broke functionality, keep this in mind:
> 
>  	void *a = NULL;
> 	u32 *b = NULL;
> 
> a + 1 = 1
> b + 1 = 4
> 
> IOW, you cannot simply replace a 'u32 *' by a 'void *' 
> without checking and fixing any pointer arithmetic.

I apologize for breaking off mode support. Since sparse fixes is big patch
series, I was able to test only boot and same was mentioned along with
the test report in the series. Also, multiple versions of patches posted
for review, I was expecting comments on this type of regressions.

I will take care of testing code coverage paths for future patches.

Thanks to Jean and Kevin for identifying and fixing this regression.

-Manjunath