Dear Wolfgang,
On Monday 16 May 2011 08:37 PM, Aneesh V wrote:
On Monday 16 May 2011 12:14 AM, Wolfgang Denk wrote:
Dear Aneesh V,
In message1305202276-27784-3-git-send-email-aneesh@ti.com you wrote:
add utility macros for:
- bit field operations
- log2n functions
...
+/* extract a bit field from a bit vector */ +#define get_bit_field(nr, start, mask)\
- (((nr)& (mask))>> (start))
+/* Set a field in a bit vector */ +#define set_bit_field(nr, start, mask, val)\
- do { \
- (nr) = ((nr)& ~(mask)) | (((val)<< (start))& (mask));\
- } while (0);
I really dislike such "useful" helpers, because they make the code unreadable. Being nonstandrd, nbody knows what they are doing, so you always will have to look up the implementation before you can continue reading the code. It's a waste of time an resources.
What if I change the above to something like this(please note there isn't any Linux, U-Boot substitute for this):
+/* extract a bit field from a bit vector */ +#define get_bit_field(nr, field)\ + (((nr) & (field##_MASK)) >> (field##_OFFSET)) + +/* Set a field in a bit vector */ +#define set_bit_field(nr, field, val)\ + do { \ + (nr) = ((nr) & ~(field##_MASK)) | (((val) << (field##_OFFSET))\ + & (field##_MASK));\ + } while (0); +
And use it like this: assoc = get_bit_field(ccsidr, CCSIDR_ASSOCIATIVITY); set_bit_field(ccsidr, CCSIDR_ASSOCIATIVITY, assoc + 1);
Isn't it more intuitive and almost self-explanatory now.
If you still don't like these as standard generic macros, how about having these macros just for OMAP with these names and use them only for OMAP code?
omap_get_bit_field(nr, field) omap_set_bit_field(nr, field, val)
I can live without them for the ARM generic code but I use it extensively in my SPL series.
Please note that Simon's recent work for bitfield operations doesn't help me because the field definitions available to me are in shift/mask format and not in the range format that he uses. I will not be able to now generate the range format for the hundreds of registers I use.
best regards, Aneesh