Re: [PATCH-V2 3/4] ARM: OMAP2+: CLEANUP: Remove unnecessary ifdef around __omap2_set_globals

[Kevin Hilman <khilman@ti.com>]

"Hiremath, Vaibhav" <hvaibhav@ti.com> writes:

> On Wed, May 09, 2012 at 04:08:09, Hilman, Kevin wrote:
>> Vaibhav Hiremath <hvaibhav@ti.com> writes:
>> 
>> > The function __omap2_set_globals() can be common across all
>> > platforms/architectures, even in case of omap4, internally it
>> > calls same set of functions as in __omap2_set_globals() function
>> > (except for sdrc).
>> 
>> OK so far.
>> 
>> > This patch adds new config flag SOC_HAS_OMAP2_SDRC to handle sdrc,
>> > so that we can reuse same function across omap2/3/4...
>> 
>> But what happens when a single kernel is built that has support for an
>> SoC with an SDRC (OMAP4) and one that doesn't (AM33xx)?
>> 
>
> As such Nothing...I looking into this direction while implementing.
>
> In that case, sdrc.c file will be compiled in and execution will jump to
> omap2_set_globals_sdrc(). But inside this function, we are already checking 
> whether the omap2_globals->sdrc and omap2_globals->sms for NULL and then use 
> it.
>
> And function omap2_sdrc_init() is also depends on machine, so in case of
> Am33xx, it won't get into sdrc execution at all. And in case of omap4, it 
> will.

Then why bother with the #ifdef at all?

If it already safe to call on all SoCs, just get rid of the #ifdef all
together.

>> In that case this new SOC_HAS_OMAP2_SDRC will be set, and
>> set_globals_sdrc() will be called even for the SoCs without SDRC.
>> 
>> So, rather than add a new Kconfig option for this, I would rather see 
>> you using the existing runtime feature check for the SDRC: omap_has_sdrc()
>> 
>
> There is NO difference between runtime feature check Vs this patch, refer to the function implementation,
>
> void __init omap2_set_globals_sdrc(struct omap_globals *omap2_globals)
> {
> 	if (omap2_globals->sdrc)
> 		omap2_sdrc_base = omap2_globals->sdrc;
> 	if (omap2_globals->sms)
> 		omap2_sms_base = omap2_globals->sms;
> }
>
> The initialization happens after checking for NULL, so even if you execute it or not, the variable are set to NULL in case of am33xx.
>
> So I don't find any difference between runtime and this patch.

Except readability.

Kevin