[U-Boot-Users] [PATCH libfdt 3/3] More clean up of cmd_bootm.c

[Kim Phillips]

On Tue, 29 May 2007 21:46:01 -0400
Jerry Van Baren <gvb.uboot at gmail.com> wrote:

> Kim Phillips wrote:
> > On Sat, 26 May 2007 08:54:50 -0400
> > Jerry Van Baren <gvb.uboot at gmail.com> wrote:
> > 
<snip>
> >> +/*
> >> + * Some FDT-related defines to reduce clutter in the main code.
> >> + */
> >> +#if defined(CONFIG_OF_FLAT_TREE)
> >> +#define FDT_VALIDATE \
> >> +	(*((ulong *)(of_flat_tree + sizeof(image_header_t))) != OF_DT_HEADER)
> >> +#endif
> >> +#if defined(CONFIG_OF_LIBFDT)
> >> +#define FDT_VALIDATE \
> >> +	(fdt_check_header(of_flat_tree + sizeof(image_header_t)) != 0)
> >> +#endif
> >> +
> > 
> > I'd actually prefer to see what the code is doing, where it's doing it. 
> > Please read linux-2.6/Documentation/CodingStyle, chapter 12, while
> > you're at it.
> 
> My intention is to removed CONFIG_OF_FLAT_TREE once CONFIG_OF_LIBFDT is 
> stable and all the boards have been converted over.  CONFIG_OF_LIBFDT 
> makes CONFIG_OF_FLAT_TREE obsolete, but we have are straddling the fence 
> at the moment (ouch).
> 
> The above #define makes a rather horrible #if:
> 
> #if defined(CONFIG_OF_LIBFDT)
> 		if (fdt_check_header(of_flat_tree) == 0) {
> #else
> 		if (*(ulong *)of_flat_tree == OF_DT_HEADER) {
> #endif
> 
> into a readable one:
> 		if (FDT_VALIDATE) {

I disagree; I can't immediately see what the latter is doing.  I can't
see what variables it accesses, and the name doesn't match either
operation.  FDT_CHECK_HEADER(of_flat_tree) would be a much better
implementation IMO.  This is in direct violation of CodingStyle ch. 12,
item 2 - did you read it?

> 
> 1) The horrible #if is repeated in 3 places, so it is 9x ugly

that's valid, but I don't mind it too much, esp. given the conversion
stage we're in.

> 2) When CONFIG_OF_FLAT_TREE is retired, the #define will be removed and 
> since it will then be a simple expression:
> 		if (fdt_check_header(of_flat_tree) == 0) {
> 

retiring OF_FLAT_TREE does not automatically remove the #define.  LIBFDT
can happily continue to use it if the person removing OF_FLAT_TREE
forgets to remove it.  If you're telling me it'll be you, and you won't
forget, that's fine; I'm just interested in protecting and maintaining
readability of the code at this point.

Kim