[U-Boot] [PATCH] fw_env: add NAND support

[Guennadi Liakhovetski]

On Tue, 2 Sep 2008, Wolfgang Denk wrote:

> > @@ -596,107 +844,120 @@ static char *envmatch (char * s1, char * s2)
> >  static int env_init (void)
> >  {
> >  	int crc1, crc1_ok;
> > +	char flag1, *addr1;
> >  
> >  	int crc2, crc2_ok;
> > +	char flag2, *addr2;
> 
> I think you should number these 0 an 1, respectively here.
> 
> Because then you can read this code much better:
> 
> 
> >  		if (crc1_ok && !crc2_ok) {
> > +			dev_current = 0;
> >  		} else if (!crc1_ok && crc2_ok) {
> > +			dev_current = 1;
> 
> would become:
> 	
> 		if (crc0_ok && !crc1_ok) { 
> 			dev_current = 0;
> 		} else if (!crc0_ok && crc1_ok) {
> 			dev_current = 1;
> 
> If "0" is OK, then use "0"; if "1" is ok then use "1".
> 
> Your version: if "1" is OK then use "0", if "2" is OK then use "1"
> is more difficult to read.
> 
> [Or stick with the old identifiers, i. e. use 1 and  2  consistently,
> but don't mix 0/1 here and 1/2 there.]

Sorry, don't understand. This is the original code:

		if (crc1_ok && !crc2_ok) {
			environment.data = addr1;
			environment.flags = flag1;
			environment.crc = crc1;
			curdev = 0;
			free (addr2);

Which looks to me like "if (crc1_ok...) {... curdev = 0;...}", which I 
tried to preserve. So, you would prefer me to _change_ it?

> > +	if ((fp = fopen (fname, "r")) == NULL)
> > +		return -1;
> >  
> > +	while (i < 2 && fgets (dump, sizeof (dump), fp)) {
> >  		/* Skip incomplete conversions and comment strings */
> > +		if (dump[0] == '#')
> >  			continue;
> 
> I think you must initialize ENVSECTORS(i) here...
> 
> > +		rc = sscanf (dump, "%s %lx %lx %lx %lx",
> > +			     DEVNAME (i),
> > +			     &DEVOFFSET (i),
> > +			     &ENVSIZE (i),
> > +			     &DEVESIZE (i),
> > +			     &ENVSECTORS (i));
> > +
> > +		if (rc < 4)
> > +			continue;
> > +
> > +		if (rc < 5)
> > +			/* Default - 1 sector */
> > +			ENVSECTORS (i) = 1;
> 
> Because if rc < 4, you already continued and left ENVSECTORS uninitialized.

As far as I understand, with rc == 3 there is no DEVESIZE in the line, 
which doesn't look like a valid configuration line to me. The orginal code 
however accepted such lines and only dropped lines with rc < 3. I do not 
understand how the original code managed to work with rc == 3. As I 
thought this was a bug, I changed the test to "rc < 4", i.e., now I 
require at least 4 fields, in which case I initialise ENVSECTORS to the 
default value - 1 sector. If rc < 4 the counter "i" is not incremented and 
the line is dropped - in the same way as in the original version. Or am I 
missing something?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

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