On Sun, 31 Aug 2008, Wolfgang Denk wrote:
> Dear Guennadi Liakhovetski,
>
> In message <[EMAIL PROTECTED]> you wrote:
> >
> > Ok, how about this: we leave the current fw_env.c as it is, I submit
> > _exactly_ the state after applying my 6 patches as a new file, with
> > suitable changes to the Makefile, fix building with MTD_VERSION=old, and
> > try to improve comments in the code. Would this be accepted?
>
> Just improvements of comments? I think there were comments about code
> as well. These shall be addressed, too.
Ok, I looked through all your comments today, and this is what I extracted
from all of them as code improvement suggestions (sorry if I missed
anything, please remind):
1. do not use the union
well, I would still prefer to use it and I hope I will be allowed to do so
in a separate NAND-tool. I agree, it would be better to use the definition
from the environment.h directly. But:
a) env_t in environment.h does indeed describe the environment layout on
the media, but, this is only possible thanks to the compile-time
decisions whether or not to include flags and how long is data.
b) it wasn't done before, env_t in the original tool was not equivalent
with the environment.h definition: it couldn't decide at compile time
whether or not to have flags, so, the struct always included it, and
it had "char *data" in it, which is not the same as "char data[...]",
so you couldn't cast this type on the environment image.
c) Now the union I proposed does describe the environment layout, can be
casted on the image, and I really don't see another clean way of doing
this.
2. do not use single.crc in redundant case
This is done only at two places, yes, I realise, this is not very clean,
but in the present configuration with a comment explaining why this is
correct, I would consider this acceptable. Otherwise, I could either
introducs a reference to crc in struct environment similar to the pointer
to data, or I could open code using the correct - single or redundant -
crc at these two occasions, which, of course, is actually not needed.
3. use a type equivalent with the one from the environment.h header
See 1. above.
4. fix MTD_OLD
Would we still need this with NAND-only tool?
5. clarify back-up mode
This is actually a comment improvement, can do.
6. erase / write only as much as needed
Yes, this is important, I'll have a look at it.
7. in flash_bad_blocks do not modify blockstart
Can do this, good point.
Have I missed anything? Would the current v2 version _with_ these changes
be accepted?
A couple more questions then:
Shall I keep support for NOR in the separate NAND version or completely
remove it? The "type == MTD_NORFLASH" code is quite small, so, removing it
would not significantly simplify or reduce the size of the nand-version.
In fact, I still think having one tool support both might be preferable
from the maintenance point of view - all fixes, improvements, changes,
that affect both versions would only have to be done once... So, maybe if
we now add a new tool, which supports both, after we have sufficiently
tested it, we could remove the original one?
How shall I name it? nand_env?
> New issue: I just noted that the default environment built into the
> fw_ tool has not much to do with the default environment build into
> the U-Boot binary image; in theory both should be the same. Don;t
> know yet if this is a new or an old bug, though.
Will have a look.
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: [EMAIL PROTECTED]
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
