On Tue, Oct 30, 2012 at 04:57:54PM -0500, Kim Phillips wrote: > Projects such as linux and u-boot run sparse on libfdt. libfdt > contains the notion of endianness via usage of endian conversion > functions such as fdt32_to_cpu. As such, in order to pass endian > checks, libfdt has to annotate its fdt variables as big endian. > This patch does that ifdef __CHECKER__ (a symbol sparse defines), > for two new fdt types: fdt32_t and fdt64_t, and subsequently > silences warnings emitted by sparse when parsing libfdt. > > Signed-off-by: Kim Phillips <kim.phill...@freescale.com> > --- > note: wasn't sure whether to introduce the new fdt32 types, or > just have libfdt use __be32 directly.
I prefer having the fdt32 types, to match the cpu_to_fdt32() and so forth functions we already use. So I like this in principle, but a couple of nits. First, I'd really like to see an accompanying patch that adds targets to the dtc makefiles to run sparse over the sources. I couldn't really test this, because I couldn't figure out quite what options I needed to invoke sparse with to get it to work properly. And I'd also like to see the default libfdt_env.h updated to supply the necessary sparse stuff - including the necessary __force casts in its byteswap functions. [snip] > diff --git a/libfdt/fdt.h b/libfdt/fdt.h > index 48ccfd9..0d9c856 100644 > --- a/libfdt/fdt.h > +++ b/libfdt/fdt.h > @@ -3,46 +3,54 @@ > > #ifndef __ASSEMBLY__ > > +#ifdef __CHECKER__ So, I'd prefer not to use __CHECKER__ directly here. I'd rather we defined a new specific symbol, that libfdt_env.h can set based on __CHECKER__ if it wants. Let's say _FDT_SPARSE The reason is because.. > +typedef __be32 fdt32_t; > +typedef __be64 fdt64_t; ..just running sparse does *not* immediately give you __be32 and __be64 types - those have to be defined in terms of __attribute__((bitwise)) and whatnot. Your uboot env probably does that already, but the default libfdt_env.h certainly doesn't. Effectively _FDT_SPARSE is sayint two things, first that we're compiling under sparse, but also that we have suitably defined endian types in the environment. Actually, given that libfdt_env.h is already required to provide the cpu_to_fdt32() and so forth macros, I think it's slightly neater to just require it to directly supply the fdt32_t etc. types when it defines _FDT_SPARSE as well, rather than defining them in terms of __beXX here then in terms of the attributes in the environment. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot