[U-Boot] [PATCH 17/30] cramfs: make cramfs usable without a NOR flash

[Wolfgang Denk]

Dear Valentin Longchamp,

In message <e0cad960c27371170bf2d2d4be3362d6665fbbfa.1302272395.git.valentin.longchamp at keymile.com> you wrote:
> From: Heiko Schocher <hs at denx.de>
> 
> Signed-off-by: Heiko Schocher <hs at denx.de>
> cc: Wolfgang Denk <wd at denx.de>
> cc: Detlev Zundel <dzu at denx.de>
> cc: Valentin Longchamp <valentin.longchamp at keymile.com>
> cc: Holger Brunck <holger.brunck at keymile.com>
> Signed-off-by: Valentin Longchamp <valentin.longchamp at keymile.com>
> ---
>  common/cmd_cramfs.c |   12 +++++++++++-
>  fs/cramfs/cramfs.c  |    4 ++++
>  2 files changed, 15 insertions(+), 1 deletions(-)
> 
> diff --git a/common/cmd_cramfs.c b/common/cmd_cramfs.c
> index 8c86dc5..5e1487f 100644
> --- a/common/cmd_cramfs.c
> +++ b/common/cmd_cramfs.c
> @@ -43,7 +43,9 @@
>  #endif
>  
>  #ifdef CONFIG_CRAMFS_CMDLINE
> -flash_info_t flash_info[1];
> +#if !defined(CONFIG_SYS_NO_FLASH)
> +#include <flash.h>
> +#endif

Do we need the #ifndef here?  I don;t thik it hurts if we
unconditionally #include <flash.h> ?

But note: there was no "extern" in this declaration of flash_info[],
i. e. we _did_ allocate storage here. Is the new code really
equivalent? How extensively has it been tested?


>  #ifndef CONFIG_CMD_JFFS2
>  #include <linux/stat.h>
> @@ -119,7 +121,11 @@ int do_cramfs_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  	dev.id = &id;
>  	part.dev = &dev;
>  	/* fake the address offset */
> +#if !defined(CONFIG_SYS_NO_FLASH)
>  	part.offset = addr - flash_info[id.num].start[0];
> +#else
> +	part.offset = addr;
> +#endif

Sequences like this repeat a number of times. How about 

#ifdef CONFIG_SYS_NO_FLASH
# define OFFSET_ADJUSTMENT(x)	0
#else
# define OFFSET_ADJUSTMENT(x)	(flash_info[id.num].start[x])
#endif
...
	dev.id = &id;
	part.dev = &dev;
	/* fake the address offset */
	part.offset = addr - OFFSET_ADJUSTMENT(0);

> +#if !defined(CONFIG_SYS_NO_FLASH)
>  	part.offset = addr - flash_info[id.num].start[0];
> +#else
> +	part.offset = addr;
> +#endif

	part.offset = addr - OFFSET_ADJUSTMENT(0);

>  extern flash_info_t flash_info[];
>  #define PART_OFFSET(x)	(x->offset + flash_info[x->dev->id->num].start[0])
> +#else
> +#define PART_OFFSET(x)	(x->offset)

#define PART_OFFSET(x)	(x->offset + OFFSET_ADJUSTMENT(0))


[If we always refer to start[0] only, we can even omit the argument.]

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
We are all agreed that your  theory  is  crazy.  The  question  which
divides  us  is  whether it is crazy enough to have a chance of being
correct. My own feeling is that it is not crazy enough.  - Niels Bohr