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

[Aneesh V <aneesh@ti.com>]

On Monday 16 May 2011 12:14 AM, Wolfgang Denk wrote:
> Dear Aneesh V,
>
> In message<1305202276-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.
>

I will be very happy to use a standard one if one exists. I checked in
bitops.h but couldn't find something that's equivalent to the above. Can 
you point me to a standard one that does something equivalent.

Yes, you may have to look-up the implementation, but maybe just once.
That goes with any API, right?

On the other hand, doing shifting ORing, ANDing etc directly in the
code is less readable in my opinion.

>> +/*
>> + * Utility macro for read-modify-write of a hardware register
>> + *	addr - address of the register
>> + *	shift - starting bit position of the field to be modified
>> + *	msk - mask for the field
>> + *	val - value to be shifted masked and written to the field
>> + */
>> +#define modify_reg_32(addr, shift, msk, val) \
>> +	do {\
>> +		writel(((readl(addr)&  ~(msk))|(((val)<<  (shift))&  (msk))),\
>> +		       (addr));\
>> +	} while (0);
>
> NAK again, for the same reasons.
>
> Note that there are some semi-standardized I/O accessro macros
> available, at least for some architectures, like clrbits.*(),
> setbits_(), or clrsetbits().
>
> See for example "arch/arm/include/asm/io.h",
> "arch/powerpc/include/asm/io.h" for reference.
>
> Instead of reinventing the wheel (just differently shaped) we should
> rather try and use a single, standardized set of such helpers.
>
> So please use the existing macros instead of inventing new,
> non-standard ones.

clrsetbits will work for this need albeit not as clean as the above
one. I will use that.

>
> Best regards,
>
> Wolfgang Denk
>