Hi Wolfgang,
On Tuesday 07 June 2011 12:20 AM, Wolfgang Denk wrote:
Dear Aneesh V,
In message4DECF8DA.9030806@ti.com you wrote:
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.
To me it is definitely NOT self-explanatory.
Actually I cannot even understand the argument names, now how it's supposed to be used. I would interpret "nr" as "number"; in the context of a bit field probably a bit number? Wrong guess...
It's highly cryptic and will only work with very special #defines providing definitions of foo_MASK and foo_OFFSET.
It is not clear that you can use this on plain simple memory stored variables only, and that you must not use this on any device registers; yet your example looks as if this were intended to be used on registers - but then we would need proper I/O accessors.
No. it's not meant to be directly used on registers. Please see below.
And there are still many cases where you cannot use this because for example evaluating several bits from the same object will cause repeated accesses, which may have side effects (when accessing device registers).
And, last but not least: they are nonstandard. I still don't understand why you cannot use the standard macros that already exist in Linux and in U-Boot, here for example clrbits*(), setbits*() and clrsetbits*() ?
As I had mentioned in a previous mail, please note that the above macros are not for the same use-case as clrsetbits*() or friends (I had one macro that did something similar to clrsetbits*() and I intent to remove that in the next revision)
The above macros are for bit-field manipulation in a C integer variable - nothing more.
However, the typical use of this is for extracting bit-fields from an IO register that is already read into an integer variable (instead of reading the IO register multiple times). And similarly for write.
So, if I have to write 5 different fields in a register I first write them into a variable and finally call writel() instead of making 5 clrsetbits*() calls.
There aren't any standard routines available for this need in Linux or U-Boot. I think you had agreed on this fact sometime back.
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?
Would that make it anything better? If it's not good enough for general use, we can still use it for OMAP? Like: oh, it's ony OMAP, so code quality does not matter? I think that's not good reasoning.
No. It was not about code quality. The question was whether these macros were generic enough to be used as the standard U-boot ones. The key question is how do you represent bit fields. There are different alternatives for this.
a. bit range (say 5:3) b. shift(3) and field width(3) c. shift(3) and mask(0x38)
We traditionally use (c) and we have auto-generated defines in this form. So, my macros use this format. I was not sure if other SoCs follow the same approach. That's why I suggested making them OMAP specific if you think (c) is not the standard approach.
best regards, Aneesh