Patchwork [U-Boot,5/5] NAND: Add scrub.quiet command option

login
register
mail settings
Submitter Marek Vasut
Date Sept. 8, 2011, 8:39 p.m.
Message ID <1315514380-19089-6-git-send-email-marek.vasut@gmail.com>
Download mbox | patch
Permalink /patch/113951/
State Changes Requested
Headers show

Comments

Marek Vasut - Sept. 8, 2011, 8:39 p.m.
This allows the scrub command to scrub without asking the user if he really
wants to scrub the area. Useful in scripts.

Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
Cc: Scott Wood <scottwood@freescale.com>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Detlev Zundel <dzu@denx.de>
---
 common/cmd_nand.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)
Detlev Zundel - Sept. 9, 2011, 3:39 p.m.
Hi Marek,

> This allows the scrub command to scrub without asking the user if he really
> wants to scrub the area. Useful in scripts.
>
> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> Cc: Scott Wood <scottwood@freescale.com>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Wolfgang Denk <wd@denx.de>
> Cc: Detlev Zundel <dzu@denx.de>
> ---
>  common/cmd_nand.c |   14 +++++++++++++-
>  1 files changed, 13 insertions(+), 1 deletions(-)
>
> diff --git a/common/cmd_nand.c b/common/cmd_nand.c
> index 5b7e83d..45179e9 100644
> --- a/common/cmd_nand.c
> +++ b/common/cmd_nand.c
> @@ -502,11 +502,19 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
>  		int clean = argc > 2 && !strcmp("clean", argv[2]);
>  		int o = clean ? 3 : 2;
>  		int scrub = !strncmp(cmd, "scrub", 5);
> +		int scrub_quiet = !strncmp(cmd, "scrub.quiet", 11);
>  		int part = 0;
>  		int chip = 0;
>  		int spread = 0;
>  		int args = 2;
>  
> +		/*
> +		 * Quiet scrub is a special option only for the scrub command,
> +		 * ignore it in the following construction.
> +		 */
> +		if (scrub_quiet)
> +			cmd[5] = 0;
> +

This is _really_ hackish and makes an effort not to fit into the coding
style at hand.  Please use the available code and extend the construct
below to match the ".quiet" suffix.  It is not that different.

>  		if (cmd[5] != 0) {
>  			if (!strcmp(&cmd[5], ".spread")) {
>  				spread = 1;
> @@ -543,7 +551,7 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
>  		opts.quiet  = quiet;
>  		opts.spread = spread;
>  
> -		if (scrub) {
> +		if (scrub && !scrub_quiet) {
>  			puts("Warning: "
>  			     "scrub option will erase all factory set "
>  			     "bad blocks!\n"
> @@ -569,6 +577,10 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
>  				return -1;
>  			}
>  		}
> +
> +		if (scrub_quiet)
> +			opts.scrub = 1;
> +

Urgh.  Please turn this into

if (scrub) {
   if (!scrub_quiet) {
   } else {
   }  
}

Cheers
  Detlev
Marek Vasut - Sept. 10, 2011, 8:54 p.m.
On Friday, September 09, 2011 05:39:07 PM Detlev Zundel wrote:
> Hi Marek,
> 
> > This allows the scrub command to scrub without asking the user if he
> > really wants to scrub the area. Useful in scripts.
> > 
> > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> > Cc: Scott Wood <scottwood@freescale.com>
> > Cc: Stefano Babic <sbabic@denx.de>
> > Cc: Wolfgang Denk <wd@denx.de>
> > Cc: Detlev Zundel <dzu@denx.de>
> > ---
> > 
> >  common/cmd_nand.c |   14 +++++++++++++-
> >  1 files changed, 13 insertions(+), 1 deletions(-)
> > 
> > diff --git a/common/cmd_nand.c b/common/cmd_nand.c
> > index 5b7e83d..45179e9 100644
> > --- a/common/cmd_nand.c
> > +++ b/common/cmd_nand.c
> > @@ -502,11 +502,19 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc,
> > char * const argv[])
> > 
> >  		int clean = argc > 2 && !strcmp("clean", argv[2]);
> >  		int o = clean ? 3 : 2;
> >  		int scrub = !strncmp(cmd, "scrub", 5);
> > 
> > +		int scrub_quiet = !strncmp(cmd, "scrub.quiet", 11);
> > 
> >  		int part = 0;
> >  		int chip = 0;
> >  		int spread = 0;
> >  		int args = 2;
> > 
> > +		/*
> > +		 * Quiet scrub is a special option only for the scrub command,
> > +		 * ignore it in the following construction.
> > +		 */
> > +		if (scrub_quiet)
> > +			cmd[5] = 0;
> > +
> 
> This is _really_ hackish and makes an effort not to fit into the coding
> style at hand.  Please use the available code and extend the construct
> below to match the ".quiet" suffix.  It is not that different.

Right, I got a bit wild in here. Though still, it'll be a special case, like

} else if (!strncmp(cmd, "scrub.quiet", 11)) {

in the block below, because quiet should only work for scrub (it's no use for 
other commands).

> 
> >  		if (cmd[5] != 0) {
> >  		
> >  			if (!strcmp(&cmd[5], ".spread")) {
> >  			
> >  				spread = 1;
> > 
> > @@ -543,7 +551,7 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc,
> > char * const argv[])
> > 
> >  		opts.quiet  = quiet;
> >  		opts.spread = spread;
> > 
> > -		if (scrub) {
> > +		if (scrub && !scrub_quiet) {
> > 
> >  			puts("Warning: "
> >  			
> >  			     "scrub option will erase all factory set "
> >  			     "bad blocks!\n"
> > 
> > @@ -569,6 +577,10 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc,
> > char * const argv[])
> > 
> >  				return -1;
> >  			
> >  			}
> >  		
> >  		}
> > 
> > +
> > +		if (scrub_quiet)
> > +			opts.scrub = 1;
> > +
> 
> Urgh.  Please turn this into

What about:

if (scrub && scrub.quiet) {
	opts.scrub = 1;
} else if (scrub) {
...
}

To avoid fragmenting the code too much and avoid too deep indent.

Cheers!
> 
> if (scrub) {
>    if (!scrub_quiet) {
>    } else {
>    }
> }
> 
> Cheers
>   Detlev
Detlev Zundel - Sept. 12, 2011, 9:49 a.m.
Hi Marek,

> On Friday, September 09, 2011 05:39:07 PM Detlev Zundel wrote:
>> Hi Marek,
>> 
>> > This allows the scrub command to scrub without asking the user if he
>> > really wants to scrub the area. Useful in scripts.
>> > 
>> > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
>> > Cc: Scott Wood <scottwood@freescale.com>
>> > Cc: Stefano Babic <sbabic@denx.de>
>> > Cc: Wolfgang Denk <wd@denx.de>
>> > Cc: Detlev Zundel <dzu@denx.de>
>> > ---
>> > 
>> >  common/cmd_nand.c |   14 +++++++++++++-
>> >  1 files changed, 13 insertions(+), 1 deletions(-)
>> > 
>> > diff --git a/common/cmd_nand.c b/common/cmd_nand.c
>> > index 5b7e83d..45179e9 100644
>> > --- a/common/cmd_nand.c
>> > +++ b/common/cmd_nand.c
>> > @@ -502,11 +502,19 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc,
>> > char * const argv[])
>> > 
>> >  		int clean = argc > 2 && !strcmp("clean", argv[2]);
>> >  		int o = clean ? 3 : 2;
>> >  		int scrub = !strncmp(cmd, "scrub", 5);
>> > 
>> > +		int scrub_quiet = !strncmp(cmd, "scrub.quiet", 11);
>> > 
>> >  		int part = 0;
>> >  		int chip = 0;
>> >  		int spread = 0;
>> >  		int args = 2;
>> > 
>> > +		/*
>> > +		 * Quiet scrub is a special option only for the scrub command,
>> > +		 * ignore it in the following construction.
>> > +		 */
>> > +		if (scrub_quiet)
>> > +			cmd[5] = 0;
>> > +
>> 
>> This is _really_ hackish and makes an effort not to fit into the coding
>> style at hand.  Please use the available code and extend the construct
>> below to match the ".quiet" suffix.  It is not that different.
>
> Right, I got a bit wild in here. Though still, it'll be a special case, like
>
> } else if (!strncmp(cmd, "scrub.quiet", 11)) {
>
> in the block below, because quiet should only work for scrub (it's no use for 
> other commands).

Ok.

>> >  		if (cmd[5] != 0) {
>> >  		
>> >  			if (!strcmp(&cmd[5], ".spread")) {
>> >  			
>> >  				spread = 1;
>> > 
>> > @@ -543,7 +551,7 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc,
>> > char * const argv[])
>> > 
>> >  		opts.quiet  = quiet;
>> >  		opts.spread = spread;
>> > 
>> > -		if (scrub) {
>> > +		if (scrub && !scrub_quiet) {
>> > 
>> >  			puts("Warning: "
>> >  			
>> >  			     "scrub option will erase all factory set "
>> >  			     "bad blocks!\n"
>> > 
>> > @@ -569,6 +577,10 @@ int do_nand(cmd_tbl_t * cmdtp, int flag, int argc,
>> > char * const argv[])
>> > 
>> >  				return -1;
>> >  			
>> >  			}
>> >  		
>> >  		}
>> > 
>> > +
>> > +		if (scrub_quiet)
>> > +			opts.scrub = 1;
>> > +
>> 
>> Urgh.  Please turn this into
>
> What about:
>
> if (scrub && scrub.quiet) {
> 	opts.scrub = 1;
> } else if (scrub) {
> ...
> }
>
> To avoid fragmenting the code too much and avoid too deep indent.
>> 
>> if (scrub) {
>>    if (!scrub_quiet) {
>>    } else {
>>    }
>> }

I think it expresses the intention less clear, but I don't care enough
to nak such a thing - it's still better than the current version.

Cherrs
  Detlev

Patch

diff --git a/common/cmd_nand.c b/common/cmd_nand.c
index 5b7e83d..45179e9 100644
--- a/common/cmd_nand.c
+++ b/common/cmd_nand.c
@@ -502,11 +502,19 @@  int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 		int clean = argc > 2 && !strcmp("clean", argv[2]);
 		int o = clean ? 3 : 2;
 		int scrub = !strncmp(cmd, "scrub", 5);
+		int scrub_quiet = !strncmp(cmd, "scrub.quiet", 11);
 		int part = 0;
 		int chip = 0;
 		int spread = 0;
 		int args = 2;
 
+		/*
+		 * Quiet scrub is a special option only for the scrub command,
+		 * ignore it in the following construction.
+		 */
+		if (scrub_quiet)
+			cmd[5] = 0;
+
 		if (cmd[5] != 0) {
 			if (!strcmp(&cmd[5], ".spread")) {
 				spread = 1;
@@ -543,7 +551,7 @@  int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 		opts.quiet  = quiet;
 		opts.spread = spread;
 
-		if (scrub) {
+		if (scrub && !scrub_quiet) {
 			puts("Warning: "
 			     "scrub option will erase all factory set "
 			     "bad blocks!\n"
@@ -569,6 +577,10 @@  int do_nand(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 				return -1;
 			}
 		}
+
+		if (scrub_quiet)
+			opts.scrub = 1;
+
 		ret = nand_erase_opts(nand, &opts);
 		printf("%s\n", ret ? "ERROR" : "OK");