Hello Wolfgang, Wolfgang Denk wrote: > Dear Valentin Longchamp, > > In message > <e0cad960c27371170bf2d2d4be3362d6665fbbfa.1302272395.git.valentin.longch...@keymile.com> > you wrote: >> From: Heiko Schocher <h...@denx.de> >> >> Signed-off-by: Heiko Schocher <h...@denx.de> >> cc: Wolfgang Denk <w...@denx.de> >> cc: Detlev Zundel <d...@denx.de> >> cc: Valentin Longchamp <valentin.longch...@keymile.com> >> cc: Holger Brunck <holger.bru...@keymile.com> >> Signed-off-by: Valentin Longchamp <valentin.longch...@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> ?
Yep, you are right. > 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? flash_info is defined in the flash driver, so this is OK. It is tested on the keymile boards, and a MAKEALL runs clean. >> #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); Yep, that looks better, I change this. >> +#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.] Yep. Wolfgang? Is it OK, to send a v2 which is not in this patchseries, as I think this is an independent patch? bye, Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
