[U-Boot] [PATCH V6 2/5] omap-common: add nand spl support

[Aneesh V <aneesh@ti.com>]

Hi Simon,

On Tuesday 02 August 2011 05:33 PM, Simon Schwarz wrote:
> Hi Aneesh,
>
> <snip>
>>> diff --git a/arch/arm/cpu/armv7/omap-common/spl.c
>>> b/arch/arm/cpu/armv7/omap-common/spl.c
>>> index d177652..7ec5c7c 100644
>>> --- a/arch/arm/cpu/armv7/omap-common/spl.c
>>> +++ b/arch/arm/cpu/armv7/omap-common/spl.c
>>> @@ -26,6 +26,7 @@
>>> #include<asm/u-boot.h>
>>> #include<asm/utils.h>
>>> #include<asm/arch/sys_proto.h>
>>> +#include<nand.h>
>>> #include<mmc.h>
>>> #include<fat.h>
>>> #include<timestamp_autogenerated.h>
>>> @@ -107,6 +108,7 @@ static void parse_image_header(const struct
>>> image_header *header)
>>> }
>>> }
>>>
>>> +#ifdef CONFIG_SPL_MMC_SUPPORT
>>> static void mmc_load_image_raw(struct mmc *mmc)
>>> {
>>> u32 image_size_sectors, err;
>>> @@ -140,7 +142,9 @@ end:
>>> hang();
>>> }
>>> }
>>> +#endif /* CONFIG_SPL_MMC_SUPPORT */
>>
>> here..
>>
>>>
>>> +#ifdef CONFIG_SPL_MMC_SUPPORT
>>> static void mmc_load_image_fat(struct mmc *mmc)
>>> {
>>> s32 err;
>>> @@ -173,7 +177,9 @@ end:
>>> hang();
>>> }
>>> }
>>> +#endif /* CONFIG_SPL_MMC_SUPPORT */
>>
>> and here..
>>
>> You start the same the #ifdef again immediately after the #endif. Why
>> don't you club them together into just one #ifdef block.
>>
>> Actually, since we have garbage collection of un-used functions, I
>> think doing the calls under #ifdef should be enough, which you have
>> taken care in board_init_r(). That may help to avoid some #ifdef
>> clutter.
>
> I have to dig out this mail again. I found that removing the ifdefs
> breaks OMAP4 since it lacks some NAND specific defines.
>
> So I wonder - is it ok to define them to some garbage-value in the file
> and emit an #error if the corresponding LIB is set in SPL?
>
> This would state in the file that these defines are used and it would
> avoid the need of defining them.
>
> This would look like:
> #ifndef CONFIG_SPL_NAND_BLA
> #ifdef CONFIG_SPL_NAND_SUPPORT
> #error CONFIG_SPL_NAND_BLA is not set although \
> CONFIG_SPL_NAND_SUPPORT is active
> #else
> #define CONFIG_SPL_NAND_BLA 0x0
> #endif
> #endif
>
> Maybe this can be simplified by some macro...
>
> I find it really annoying to set values for code I don't want to use...

Maybe, what we could do is to split the file into two:
spl_mmc.c
spl_nand.c

or three:
spl.c
spl_mmc.c
spl_nand.c

and then in the Makefile conditionally include them based on
CONFIG_SPL_NAND_SUPPORT, CONFIG_SPL_MMC_SUPPORT etc. I think this may
solve your difficulties.

best regards,
Aneesh