Hi Wolfgang, On Tuesday 07 June 2011 12:20 AM, Wolfgang Denk wrote: > Dear Aneesh V, > > In message<4decf8da.9030...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot