[U-Boot-Users] [PATCH] M18 flash (Sibley) support (attempt 2)

[Stefan Roese]

On Tuesday 29 April 2008, Vasiliy Leoenenko wrote:
> The first patch (support of long commands):

I like the idea of splitting this patch up in two separate patches/emails. But 
please provide a descriptive subject and a short description that can will be 
added to the git repository as commit text for each patch. And don't forget 
your Signed-off-by line.

Please take a look at other patches on the list how this should be done. Best 
would be if you could use the git tools to create (and send) the patches.

Thanks.

Some further remarks below:

> ===================================================
> diff -aupNr a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
> --- a/drivers/mtd/cfi_flash.c	2008-04-21 02:39:38.000000000 +0400
> +++ b/drivers/mtd/cfi_flash.c	2008-04-29 15:57:51.000000000 +0400
> @@ -298,17 +298,29 @@ static inline void flash_unmap(flash_inf
>  /*-----------------------------------------------------------------------
>   * make a proper sized command based on the port and chip widths
>   */
> -static void flash_make_cmd (flash_info_t * info, uchar cmd, void *cmdbuf)
> +static void flash_make_cmd (flash_info_t * info, ulong cmd, void *cmdbuf)
>  {
>  	int i;
> +	int cpofft;
>  	uchar *cp = (uchar *) cmdbuf;
> +	uchar cp_val;
>
>  #if defined(__LITTLE_ENDIAN) || defined(CFG_WRITE_SWAPPED_DATA)
> -	for (i = info->portwidth; i > 0; i--)
> +	for (i = sizeof(cfiword_t); i > 0; i--)
> +	{

+	for (i = sizeof(cfiword_t); i > 0; i--) {

The code down below has some coding style issues too. Please 

> +		cpofft=(i-1);

+		cpofft = i - 1;

The code down below has some coding style issues too. Please try to be more 
careful here.

And the resulting code looks like this:

#if defined(__LITTLE_ENDIAN) || defined(CFG_WRITE_SWAPPED_DATA)
	for (i = sizeof(cfiword_t); i > 0; i--)
	{
		cpofft=(i-1);
#else
	for (i = 1; i <= sizeof(cfiword_t); i++)
	{
		cpofft=(sizeof(cfiword_t)-i);
#endif
		if( cpofft%info->chipwidth >= sizeof(ulong) || cpofft>=info->portwidth)
			cp_val = 0x00;
		else
			cp_val = *((uchar*)&cmd + cpofft%info->chipwidth);
			
		cp[i-1] = cp_val;		
	}


Apart from the coding-style issues, this is getting quite complex and unclear. 
At least to me. Are you sure that this can't be written differently to make 
it easier to understand? 

Thanks.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================