Hello Wolfgang,
Wolfgang Denk wrote:
Dear Valentin Longchamp,
In message e0cad960c27371170bf2d2d4be3362d6665fbbfa.1302272395.git.valentin.longchamp@keymile.com you wrote:
From: Heiko Schocher hs@denx.de
Signed-off-by: Heiko Schocher hs@denx.de cc: Wolfgang Denk wd@denx.de cc: Detlev Zundel dzu@denx.de cc: Valentin Longchamp valentin.longchamp@keymile.com cc: Holger Brunck holger.brunck@keymile.com Signed-off-by: Valentin Longchamp valentin.longchamp@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