From mboxrd@z Thu Jan  1 00:00:00 1970
From: Heiko Schocher <hs@denx.de>
Date: Tue, 03 May 2011 07:49:20 +0200
Subject: [U-Boot] [PATCH 17/30] cramfs: make cramfs usable without a NOR
	flash
In-Reply-To: <20110430082000.81B78D5270E@gemini.denx.de>
References: <cover.1302272395.git.valentin.longchamp@keymile.com>
	<cover.1302272395.git.valentin.longchamp@keymile.com>
	<e0cad960c27371170bf2d2d4be3362d6665fbbfa.1302272395.git.valentin.longchamp@keymile.com>
	<20110430082000.81B78D5270E@gemini.denx.de>
Message-ID: <4DBF9760.5030607@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany