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

[Detlev Zundel]

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 at gmail.com>
>> > Cc: Scott Wood <scottwood at freescale.com>
>> > Cc: Stefano Babic <sbabic at denx.de>
>> > Cc: Wolfgang Denk <wd at denx.de>
>> > Cc: Detlev Zundel <dzu at 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  

-- 
Irrationality is the square root of all evil.
                                     -- Douglas Hofstadter
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de