On Tue, 29 May 2007 21:46:01 -0400 Jerry Van Baren gvb.uboot@gmail.com wrote:
Kim Phillips wrote:
On Sat, 26 May 2007 08:54:50 -0400 Jerry Van Baren gvb.uboot@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?
- 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.
- 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