RE: [RESUBMIT][PATCH 4/7] OMAP4: Update common omap platform common sources.

2009-05-18 Thread Shilimkar, Santosh

> > > is a static function.  The only user of omap4_globals is 
> > > __omap4_set_globals.
> > > It looks to me like the only purpose of omap4_globals is to pass a
> > > structure to __omap4_set_globals.  Why not use a function 
> > > argument instead?
> > Indeed. Actually I just more or less followed what is done 
> for OMAP2/OMAP3 here. I will clean this for OMAP4.
> > 
> > Tony,
> > We may want to clean up this for OMAP2/OMAP3 as well. Can 
> we add this to the clean up patches planned if any. I can 
> create the patch.
> 
> Sure, please post that patch for review.

OK


Regards,
Santosh
 --
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RESUBMIT][PATCH 4/7] OMAP4: Update common omap platform common sources.

2009-05-18 Thread Tony Lindgren
* Shilimkar, Santosh  [090516 13:04]:
> Thanks Russell for scanning all the patches minutely !!
> 
> > -Original Message-
> > From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk] 
> > Sent: Saturday, May 16, 2009 3:24 PM
> > To: Shilimkar, Santosh
> > Cc: linux-arm-ker...@lists.arm.linux.org.uk; 
> > linux-omap@vger.kernel.org
> > Subject: Re: [RESUBMIT][PATCH 4/7] OMAP4: Update common omap 
> > platform common sources.
> > 
> > On Thu, May 07, 2009 at 11:59:13AM +0530, Santosh Shilimkar wrote:
> > > @@ -309,3 +313,26 @@ void __init omap2_set_globals_343x(void)
> > >  }
> > >  #endif
> > >  
> > > +#if defined(CONFIG_ARCH_OMAP4)
> > > +static struct omap_globals *omap4_globals;
> > > +
> > > +static void __init __omap4_set_globals(void)
> > > +{
> > > + omap2_set_globals_tap(omap4_globals);
> > > + omap2_set_globals_control(omap4_globals);
> > > +}
> > > +static struct omap_globals omap443x_globals = {
> > > + .class  = OMAP443X_CLASS,
> > > + .tap= OMAP2_IO_ADDRESS(0x4830A000),
> > > + .ctrl   = OMAP2_IO_ADDRESS(OMAP443X_CTRL_BASE),
> > > + .prm= OMAP2_IO_ADDRESS(OMAP4430_PRM_BASE),
> > > + .cm = OMAP2_IO_ADDRESS(OMAP4430_CM_BASE),
> > > +};
> > > +
> > > +void __init omap2_set_globals_443x(void)
> > > +{
> > > + omap4_globals = &omap443x_globals;
> > > + __omap4_set_globals();
> > 
> > Hmm, confused.  omap4_globals is a static variable, and 
> > __omap4_set_globals
> > is a static function.  The only user of omap4_globals is 
> > __omap4_set_globals.
> > It looks to me like the only purpose of omap4_globals is to pass a
> > structure to __omap4_set_globals.  Why not use a function 
> > argument instead?
> Indeed. Actually I just more or less followed what is done for OMAP2/OMAP3 
> here. I will clean this for OMAP4.
> 
> Tony,
> We may want to clean up this for OMAP2/OMAP3 as well. Can we add this to the 
> clean up patches planned if any. I can create the patch.

Sure, please post that patch for review.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RESUBMIT][PATCH 4/7] OMAP4: Update common omap platform common sources.

2009-05-18 Thread Russell King - ARM Linux
On Mon, May 18, 2009 at 05:37:24PM +0530, Shilimkar, Santosh wrote:
> > As for the pointer to the array of names, why can't this be declared
> > const and therefore that cast be removed?
> I am sorry but didn't get this point. 
> dm_source_names = (char **)omap4_dm_source_names; 
> In this 'omap4_dm_source_names' is already const. The problem is 
> dm_source_names is declared as 'char ** ' and hence compiler cribs for right 
> reasons without cast.
> 
> Did you mean don't declare 'omap4_dm_source_name' as const ?

No, I'm saying dm_source_names should also be const.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RE: [RESUBMIT][PATCH 4/7] OMAP4: Update common omap platform common sources.

2009-05-18 Thread Shilimkar, Santosh
Russell,

> > +static const char *omap4_dm_source_names[] __initdata = {
> > +   "sys_ck",
> > +   "omap_32k_fck",
> > +   NULL
> > +};
> > +static struct clk **omap4_dm_source_clocks[2];
> 
> Umm.  struct clk **[2].
> 
> > +static const int dm_timer_count = ARRAY_SIZE(omap4_dm_timers);
> > +
> >  #else
> >  
> >  #error OMAP architecture not supported!
> > @@ -461,7 +508,8 @@ __u32 
> omap_dm_timer_modify_idlect_mask(__u32 inputmask)
> >  }
> >  EXPORT_SYMBOL_GPL(omap_dm_timer_modify_idlect_mask);
> >  
> > -#elif defined(CONFIG_ARCH_OMAP2) || defined (CONFIG_ARCH_OMAP3)
> > +#elif defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3) || \
> > +   defined(CONFIG_ARCH_OMAP4)
> >  
> >  struct clk *omap_dm_timer_get_fclk(struct omap_dm_timer *timer)
> >  {
> > @@ -705,6 +753,10 @@ int __init omap_dm_timer_init(void)
> > dm_timers = omap3_dm_timers;
> > dm_source_names = (char **)omap3_dm_source_names;
> > dm_source_clocks = (struct clk 
> **)omap3_dm_source_clocks;
> > +   } else if (cpu_is_omap44xx()) {
> > +   dm_timers = omap4_dm_timers;
> > +   dm_source_names = (char **)omap4_dm_source_names;
> > +   dm_source_clocks = (struct clk 
> **)omap4_dm_source_clocks;
> 
> which then gets casted to a struct clk **.  These are two different
> objects.  I don't think someone quite understood what they were
> doing here and threw a cast in to shut up the compiler warning:
> 
>   warning: assignment from incompatible pointer type
> 
> This is *very* wrong and is a prime example of why casts are _bad_
> news.  This cast is saying "THERE IS A BUG HERE" in 100ft 
> high letters.
> 
> What you want is:
> 
> static struct clk *omap4_dm_source_clocks[2];
> ...
> 
>   dm_source_clocks = omap4_dm_source_clocks;
> 
> This is because struct clk *[] is equivalent to struct clk **.
> (remember that arrays are handled in C as a pointer to the first
> array element.)

OK. I will fix this for OMAP4 now. Will create patch for OMAP2/3 bit later.

> As for the pointer to the array of names, why can't this be declared
> const and therefore that cast be removed?
I am sorry but didn't get this point. 
dm_source_names = (char **)omap4_dm_source_names; 
In this 'omap4_dm_source_names' is already const. The problem is 
dm_source_names is declared as 'char ** ' and hence compiler cribs for right 
reasons without cast.

Did you mean don't declare 'omap4_dm_source_name' as const ?

> TTOTD: Casts are bad news.  It's far better to have stuff correctly
> typed in the first place.

Regards,
Santosh
  
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RE: [RESUBMIT][PATCH 4/7] OMAP4: Update common omap platform common sources.

2009-05-16 Thread Shilimkar, Santosh
Thanks Russell for scanning all the patches minutely !!

> -Original Message-
> From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk] 
> Sent: Saturday, May 16, 2009 3:24 PM
> To: Shilimkar, Santosh
> Cc: linux-arm-ker...@lists.arm.linux.org.uk; 
> linux-omap@vger.kernel.org
> Subject: Re: [RESUBMIT][PATCH 4/7] OMAP4: Update common omap 
> platform common sources.
> 
> On Thu, May 07, 2009 at 11:59:13AM +0530, Santosh Shilimkar wrote:
> > @@ -309,3 +313,26 @@ void __init omap2_set_globals_343x(void)
> >  }
> >  #endif
> >  
> > +#if defined(CONFIG_ARCH_OMAP4)
> > +static struct omap_globals *omap4_globals;
> > +
> > +static void __init __omap4_set_globals(void)
> > +{
> > +   omap2_set_globals_tap(omap4_globals);
> > +   omap2_set_globals_control(omap4_globals);
> > +}
> > +static struct omap_globals omap443x_globals = {
> > +   .class  = OMAP443X_CLASS,
> > +   .tap= OMAP2_IO_ADDRESS(0x4830A000),
> > +   .ctrl   = OMAP2_IO_ADDRESS(OMAP443X_CTRL_BASE),
> > +   .prm= OMAP2_IO_ADDRESS(OMAP4430_PRM_BASE),
> > +   .cm = OMAP2_IO_ADDRESS(OMAP4430_CM_BASE),
> > +};
> > +
> > +void __init omap2_set_globals_443x(void)
> > +{
> > +   omap4_globals = &omap443x_globals;
> > +   __omap4_set_globals();
> 
> Hmm, confused.  omap4_globals is a static variable, and 
> __omap4_set_globals
> is a static function.  The only user of omap4_globals is 
> __omap4_set_globals.
> It looks to me like the only purpose of omap4_globals is to pass a
> structure to __omap4_set_globals.  Why not use a function 
> argument instead?
Indeed. Actually I just more or less followed what is done for OMAP2/OMAP3 
here. I will clean this for OMAP4.

Tony,
We may want to clean up this for OMAP2/OMAP3 as well. Can we add this to the 
clean up patches planned if any. I can create the patch.

> > +};
> > +static const char *omap4_dm_source_names[] __initdata = {
> > +   "sys_ck",
> > +   "omap_32k_fck",
> > +   NULL
> > +};
> > +static struct clk **omap4_dm_source_clocks[2];
> 
> Umm.  struct clk **[2].
> 
> > +static const int dm_timer_count = ARRAY_SIZE(omap4_dm_timers);
> > +
> >  #else
> >  
> >  #error OMAP architecture not supported!
> > @@ -461,7 +508,8 @@ __u32 
> omap_dm_timer_modify_idlect_mask(__u32 inputmask)
> >  }
> >  EXPORT_SYMBOL_GPL(omap_dm_timer_modify_idlect_mask);
> >  
> > -#elif defined(CONFIG_ARCH_OMAP2) || defined (CONFIG_ARCH_OMAP3)
> > +#elif defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3) || \
> > +   defined(CONFIG_ARCH_OMAP4)
> >  
> >  struct clk *omap_dm_timer_get_fclk(struct omap_dm_timer *timer)
> >  {
> > @@ -705,6 +753,10 @@ int __init omap_dm_timer_init(void)
> > dm_timers = omap3_dm_timers;
> > dm_source_names = (char **)omap3_dm_source_names;
> > dm_source_clocks = (struct clk 
> **)omap3_dm_source_clocks;
> > +   } else if (cpu_is_omap44xx()) {
> > +   dm_timers = omap4_dm_timers;
> > +   dm_source_names = (char **)omap4_dm_source_names;
> > +   dm_source_clocks = (struct clk 
> **)omap4_dm_source_clocks;
> 
> which then gets casted to a struct clk **.  These are two different
> objects.  I don't think someone quite understood what they were
> doing here and threw a cast in to shut up the compiler warning:
> 
>   warning: assignment from incompatible pointer type
> 
> This is *very* wrong and is a prime example of why casts are _bad_
> news.  This cast is saying "THERE IS A BUG HERE" in 100ft 
> high letters.
> 
> What you want is:
> 
> static struct clk *omap4_dm_source_clocks[2];
> ...
> 
>   dm_source_clocks = omap4_dm_source_clocks;
> 
> This is because struct clk *[] is equivalent to struct clk **.
> (remember that arrays are handled in C as a pointer to the first
> array element.)
> 
> As for the pointer to the array of names, why can't this be declared
> const and therefore that cast be removed?
> 
> TTOTD: Casts are bad news.  It's far better to have stuff correctly
> typed in the first place.

Here again, I followed what is being done for OMAP3 without much thought.

Tony if you agree this can be cleaned up for OMAP1/OMAP2/OMAP3 as well.

> > diff --git a/arch/arm/plat-omap/io.c b/arch/arm/plat-omap/io.c
> > index af326ef..fbd7b3c 100644
> > --- a/arch/arm/plat-omap/io.c
> > +++ b/arch/arm/plat-omap/io.c
> > @@ -1,3 +1,15 @@
> > +/*
> > + * Common io.c file
> > + *
> > + * Copyright (C) 2009 Texa

Re: [RESUBMIT][PATCH 4/7] OMAP4: Update common omap platform common sources.

2009-05-16 Thread Russell King - ARM Linux
On Thu, May 07, 2009 at 11:59:13AM +0530, Santosh Shilimkar wrote:
> @@ -309,3 +313,26 @@ void __init omap2_set_globals_343x(void)
>  }
>  #endif
>  
> +#if defined(CONFIG_ARCH_OMAP4)
> +static struct omap_globals *omap4_globals;
> +
> +static void __init __omap4_set_globals(void)
> +{
> + omap2_set_globals_tap(omap4_globals);
> + omap2_set_globals_control(omap4_globals);
> +}
> +static struct omap_globals omap443x_globals = {
> + .class  = OMAP443X_CLASS,
> + .tap= OMAP2_IO_ADDRESS(0x4830A000),
> + .ctrl   = OMAP2_IO_ADDRESS(OMAP443X_CTRL_BASE),
> + .prm= OMAP2_IO_ADDRESS(OMAP4430_PRM_BASE),
> + .cm = OMAP2_IO_ADDRESS(OMAP4430_CM_BASE),
> +};
> +
> +void __init omap2_set_globals_443x(void)
> +{
> + omap4_globals = &omap443x_globals;
> + __omap4_set_globals();

Hmm, confused.  omap4_globals is a static variable, and __omap4_set_globals
is a static function.  The only user of omap4_globals is __omap4_set_globals.
It looks to me like the only purpose of omap4_globals is to pass a
structure to __omap4_set_globals.  Why not use a function argument instead?

> diff --git a/arch/arm/plat-omap/devices.c b/arch/arm/plat-omap/devices.c
> index 87fb7ff..a016c6c 100644
> --- a/arch/arm/plat-omap/devices.c
> +++ b/arch/arm/plat-omap/devices.c
> @@ -311,6 +311,8 @@ static void omap_init_wdt(void)
>   wdt_resources[0].start = 0x49016000; /* WDT2 */
>   else if (cpu_is_omap343x())
>   wdt_resources[0].start = 0x48314000; /* WDT2 */
> + else if (cpu_is_omap44xx())
> + wdt_resources[0].start = 0x4A314000;

Normally lower case characters for hex.

> - if (cpu_class_is_omap2())
> - setup_irq(INT_24XX_SDMA_IRQ0, &omap24xx_dma_irq);
> + if (cpu_class_is_omap2()) {
> + if (cpu_is_omap44xx())
> + setup_irq(INT_44XX_SDMA_IRQ0, &omap24xx_dma_irq);
> + else
> + setup_irq(INT_24XX_SDMA_IRQ0, &omap24xx_dma_irq);
> + }

if (cpu_class_is_omap2()) {
int irq;
if (cpu_is_omap44xx())
irq = INT_44XX_SDMA_IRQ0;
else
irq = INT_24XX_SDMA_IRQ0;
setup_irq(irq, &omap24xx_dma_irq);
}

would be a cleaner approach.

> +static struct omap_dm_timer omap4_dm_timers[] = {
> + { .phys_base = 0x4A318000, .irq = INT_44XX_GPTIMER1 },
> + { .phys_base = 0x48032000, .irq = INT_44XX_GPTIMER2 },
> + { .phys_base = 0x48034000, .irq = INT_44XX_GPTIMER3 },
> + { .phys_base = 0x48036000, .irq = INT_44XX_GPTIMER4 },
> + { .phys_base = 0x40138000, .irq = INT_44XX_GPTIMER5 },
> + { .phys_base = 0x4013A000, .irq = INT_44XX_GPTIMER6 },
> + { .phys_base = 0x4013C000, .irq = INT_44XX_GPTIMER7 },
> + { .phys_base = 0x4013E000, .irq = INT_44XX_GPTIMER8 },
> + { .phys_base = 0x4803E000, .irq = INT_44XX_GPTIMER9 },
> + { .phys_base = 0x48086000, .irq = INT_44XX_GPTIMER10 },
> + { .phys_base = 0x48088000, .irq = INT_44XX_GPTIMER11 },
> + { .phys_base = 0x4A32, .irq = INT_44XX_GPTIMER12 },

Lower case for hex.

> +};
> +static const char *omap4_dm_source_names[] __initdata = {
> + "sys_ck",
> + "omap_32k_fck",
> + NULL
> +};
> +static struct clk **omap4_dm_source_clocks[2];

Umm.  struct clk **[2].

> +static const int dm_timer_count = ARRAY_SIZE(omap4_dm_timers);
> +
>  #else
>  
>  #error OMAP architecture not supported!
> @@ -461,7 +508,8 @@ __u32 omap_dm_timer_modify_idlect_mask(__u32 inputmask)
>  }
>  EXPORT_SYMBOL_GPL(omap_dm_timer_modify_idlect_mask);
>  
> -#elif defined(CONFIG_ARCH_OMAP2) || defined (CONFIG_ARCH_OMAP3)
> +#elif defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3) || \
> + defined(CONFIG_ARCH_OMAP4)
>  
>  struct clk *omap_dm_timer_get_fclk(struct omap_dm_timer *timer)
>  {
> @@ -705,6 +753,10 @@ int __init omap_dm_timer_init(void)
>   dm_timers = omap3_dm_timers;
>   dm_source_names = (char **)omap3_dm_source_names;
>   dm_source_clocks = (struct clk **)omap3_dm_source_clocks;
> + } else if (cpu_is_omap44xx()) {
> + dm_timers = omap4_dm_timers;
> + dm_source_names = (char **)omap4_dm_source_names;
> + dm_source_clocks = (struct clk **)omap4_dm_source_clocks;

which then gets casted to a struct clk **.  These are two different
objects.  I don't think someone quite understood what they were
doing here and threw a cast in to shut up the compiler warning:

warning: assignment from incompatible pointer type

This is *very* wrong and is a prime example of why casts are _bad_
news.  This cast is saying "THERE IS A BUG HERE" in 100ft high letters.

What you want is:

static struct clk *omap4_dm_source_clocks[2];
...

dm_source_clocks = omap4_dm_source_clocks;

This is because struct clk *[] is equivalent to struct clk **.
(remember that arrays are handled in C as a