From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefan Roese <sr@denx.de>
Date: Sat, 21 Oct 2006 12:32:24 +0200
Subject: [U-Boot-Users] [PATCH] NAND feature update
In-Reply-To: <20061019022942.GA12357@orphique>
References: <20060820221446.GA19488@orphique> <20061019022942.GA12357@orphique>
Message-ID: <200610211232.24780.sr@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

Hi Ladis,

On Thursday 19 October 2006 04:29, Ladislav Michl wrote:
> Here is updated patch, regenerated against top of git tree. Please give
> it a try.
> 
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>

Does not apply clean anymore. Could you please fix this and resubmit. Thanks.

And here already some comments:

<snip>

> @@ -83,50 +92,70 @@ static int nand_dump(nand_info_t *nand, 
>  
>  /* ------------------------------------------------------------------------- */
>  
> -static void
> -arg_off_size(int argc, char *argv[], ulong *off, ulong *size, ulong totsize)
> +static int str2long(char *p, ulong *num)

Please either remove this small helper function are make it inline.

<snip>

> @@ -181,7 +210,7 @@ int do_nand(cmd_tbl_t * cmdtp, int flag,
>  		return 0;
>  	}
>  
> -	if (strcmp(cmd, "bad") != 0 && strcmp(cmd, "erase") != 0 &&
> +	if (strcmp(cmd, "bad") != 0 && strncmp(cmd, "erase", 3) != 0 &&

Please don't change the check on "erase" here. The string should match
completely and not only the first 3 chars.

>  	    strncmp(cmd, "dump", 4) != 0 &&
>  	    strncmp(cmd, "read", 4) != 0 && strncmp(cmd, "write", 5) != 0 &&
>  	    strcmp(cmd, "scrub") != 0 && strcmp(cmd, "markbad") != 0 &&
> @@ -205,35 +234,15 @@ int do_nand(cmd_tbl_t * cmdtp, int flag,
>  		return 0;
>  	}
>  
> -	if (strcmp(cmd, "erase") == 0 || strcmp(cmd, "scrub") == 0) {
> +	if (strncmp(cmd, "erase", 3) == 0 || strcmp(cmd, "scrub") == 0) {

Again.

>  		nand_erase_options_t opts;
>  		int clean = argc >= 3 && !strcmp("clean", argv[2]);
> -		int rest_argc = argc - 2;
> -		char **rest_argv = argv + 2;
> +		int o = clean ? 3 : 2;
>  		int scrub = !strcmp(cmd, "scrub");
>  
> -		if (clean) {
> -			rest_argc--;
> -			rest_argv++;
> -		}
> -
> -		if (rest_argc == 0) {
> -
> -			printf("\nNAND %s: device %d whole chip\n",
> -			       cmd,
> -			       nand_curr_device);
> -
> -			off = size = 0;
> -		} else {
> -			arg_off_size(rest_argc, rest_argv, &off, &size,
> -				     nand->size);
> -
> -			if (off == 0 && size == 0)
> -				return 1;
> -
> -			printf("\nNAND %s: device %d offset 0x%x, size 0x%x\n",
> -			       cmd, nand_curr_device, off, size);
> -		}
> +		printf("\nNAND %s: ", scrub ? "scrub" : "erase");
> +		if (arg_off_size(argc - o, argv + o, nand, &off, &size) != 0)
> +			return 1;

This seems quite obscure (at least to Wolfgang and me ;-)). Could you please
either make the code a little more "readable", or at least add some comments
here. Thanks.

>  
>  		memset(&opts, 0, sizeof(opts));
>  		opts.offset = off;
> @@ -242,23 +251,23 @@ int do_nand(cmd_tbl_t * cmdtp, int flag,
>  		opts.quiet  = quiet;
>  
>  		if (scrub) {
> -			printf("Warning: "
> -			       "scrub option will erase all factory set "
> -			       "bad blocks!\n"
> -			       "	 "
> -			       "There is no reliable way to recover them.\n"
> -			       "	 "
> -			       "Use this command only for testing purposes "
> -			       "if you\n"
> -			       "	 "
> -			       "are sure of what you are doing!\n"
> -			       "\nReally scrub this NAND flash? <y/N>\n"
> -				);
> +			puts("Warning: "
> +			     "scrub option will erase all factory set "
> +			     "bad blocks!\n"
> +			     "         "
> +			     "There is no reliable way to recover them.\n"
> +			     "         "
> +			     "Use this command only for testing purposes "
> +			     "if you\n"
> +			     "         "
> +			     "are sure of what you are doing!\n"
> +			     "\nReally scrub this NAND flash? <y/N>\n"
> +			    );

Indentation with TAB's please.

<snip>

> @@ -404,9 +408,9 @@ int do_nand(cmd_tbl_t * cmdtp, int flag,
>  		       }
>  		} else {
>  			if (!nand_lock(nand, tight)) {
> -				printf ("NAND flash successfully locked\n");
> +				puts("NAND flash successfully locked\n");
>  			} else {
> -				printf ("Error locking NAND flash. \n");
> +				puts("Error locking NAND flash. \n");

Please remove the space after the ".". Thanks.

>  				return 1;
>  			}
>  		}
> @@ -414,19 +418,14 @@ int do_nand(cmd_tbl_t * cmdtp, int flag,
>  	}
>  
>  	if (strcmp(cmd, "unlock") == 0) {
> -		if (argc == 2) {
> -			off = 0;
> -			size = nand->size;
> -		} else {
> -			arg_off_size(argc - 2, argv + 2, &off, &size,
> -				     nand->size);
> -		}
> +		if (arg_off_size(argc - 2, argv + 2, nand, &off, &size) < 0)
> +			return 1;
>  
>  		if (!nand_unlock(nand, off, size)) {
> -			printf("NAND flash successfully unlocked\n");
> +			puts("NAND flash successfully unlocked\n");
>  		} else {
> -			printf("Error unlocking NAND flash. "
> -			       "Write and erase will probably fail\n");
> +			puts("Error unlocking NAND flash. "

Please remove the space after the ".". Thanks.

Best regards,
Stefan