| Message ID | e0cad960c27371170bf2d2d4be3362d6665fbbfa.1302272395.git.valentin.longchamp@keymile.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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> ? 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
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
Dear Heiko Schocher, In message <4DBF9760.5030607@denx.de> you wrote: > > >> --- 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. You are probably right that the code with your patch is correct, i. e. that this one-line deletion fixes an actual bug in the existing code. But you don't mention this in the commit message. These are two unrelated changes, that belon into separate commits. Best regards, Wolfgang Denk
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 #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 /* pre-set Boot file name */ if ((filename = getenv("bootfile")) == NULL) { @@ -182,7 +188,11 @@ int do_cramfs_ls(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 if (argc == 2) filename = argv[1]; diff --git a/fs/cramfs/cramfs.c b/fs/cramfs/cramfs.c index 2956d39..910955d 100644 --- a/fs/cramfs/cramfs.c +++ b/fs/cramfs/cramfs.c @@ -41,8 +41,12 @@ struct cramfs_super super; /* CPU address space offset calculation macro, struct part_info offset is * device address space offset, so we need to shift it by a device start address. */ +#if !defined(CONFIG_SYS_NO_FLASH) 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) +#endif static int cramfs_read_super (struct part_info *info) {