[U-Boot] [PATCH v3 02/10] armv7: add miscellaneous utility macros

[Wolfgang Denk]

Dear Aneesh V,

In message <4DECF8DA.9030806 at 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.

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*() ?

> 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.

> 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

I don;t think I'm going to accept that code either.

> able to now generate the range format for the hundreds of registers I
> use.

Ah!  I thought you were talking about registers.  See note above about
I/O accessors.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A Perl script is correct if it's halfway readable and  gets  the  job
done before your boss fires you.
                       - L. Wall & R. L. Schwartz, _Programming Perl_