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

[Graeme Russ]

Hi Aneesh,

On Thu, Jun 16, 2011 at 3:39 PM, Aneesh V <aneesh at ti.com> wrote:
> Hi Grame,
>
> On Wednesday 15 June 2011 06:12 PM, Graeme Russ wrote:
>>
>> On 15/06/11 22:04, Wolfgang Denk wrote:
>>>
>>> Dear Aneesh V,
>>>
>>> In message<4DF89102.9040508 at ti.com>  you wrote:
>>>>
>>>> Will you accept something like this?
>>>>
>>>> a_val = (reg&  a_mask)>>  a_shift;
>>>
>>> Yes, of course (that's what seems most natural to me).
>>>
>>
>> Me too - The code is obvious - the desired value is being masked out of a
>> larger composite value and then shifted right to bit 0
>>
>> And to set the value then you have:
>>
>>        reg&= ~a_mask;                          /* Clear a_val */
>>        reg |= (a_val<<  a_shift)&  a_mask;     /* Set new a_val */
>>
>> AND'ing with a_mask is required to prevent accidental clobbering when
>> a_val
>> is out-of-range. May give undesirable results by setting an illegal a_val,
>> but at least you don't clobber unrelated bit fields
>
> These are exactly what my helper functions were doing. Are you
> suggesting that doing these directly is better than doing them
> using helper functions?

I (personally) think that the two lines of memberwise bit mask/shift is
more obvious in its intent that even clrsetbits. I understand there is
defensive programming issues that can be applied with macros or helper
functions, but then the you loose 'obviousness' and sometimes this can
make figuring out the problem even harder (one assumes the macro or
helper function works properly).

When push comes to shove, the compiler will probably produce identical
code anyway.

Sometimes I like seeing the raw elegance of what is going on under the
hood :)

Now, that being said, I see no reason not to do the following if I had,
for example, multiple serial port configuration registers which are all
identical:

/* num data bits is stored in bits 2-4 of the serial config register */
#define DATA_BITS_MASK		0x001c
#define DATA_BITS_OFFSET	2

u32 set_serial_data_bits(u32 ser_cfg, u8 data_bits)
{
	ser_cfg &= ~DATA_BITS_MASK;
	ser_cfg |= ((u32)ser_cfg << DATA_BITS_OFFSET) &  DATA_BITS_MASK;

	return ser_cfg;
}

void serial_init(void)
{
	u32 ser_cfg;

	for (i=0; i<NUM_SERIAL_PORTS; i++) {
		ser_cfg = read_serial_cfg(i);
		ser_cfg = set_serial_data_bits(ser_cfg, 7);
		write_serial_cfg(i, ser_cfg);
	}
}

But that's just me - I tend to avoid #define macros

Regards,

Graeme