[U-Boot] [PATCH 1/5] Tegra2: Add bitfield access macros

[Simon Glass]

Hi Wolfgang,

On Tue, Jun 7, 2011 at 3:06 AM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Simon Glass,
>
> In message <BANLkTi=+pqk=UQY_=NoXcMabBDm845xcEw at mail.gmail.com> you wrote:
>>
>> >> #define clrsetfield_le32(bitfield, addr, value) =A0...
>> >>
>> >> Then caller can define these in a header file:
>> >>
>> >> #define FIELD_MASK 0xf0000
>> >> #define FIELD_SHIFT 16
>> >>
>> >> And use this macro to set the bitfield to 6, for example:
>> >>
>> >> clrsetfield_le32(FIELD, &my_device->ctrl, 6)
>> >>
>> >> (this will simply shift the value left 16 bits and apply the supplied ma=
>> sk)
>> >>
>> >> This captures the essence of bitfields, in that we are abstracting the
>> >> field under a single name. The change would just add this macro to the
>> >> io.h header file.
>> >
>> > Sorry, I fail to understand how you envision to use this, and how it
>> > would be different from =A0clrsetbits*() ?
>>
>> For example this allows us to replace:
>>
>> clrsetbits_le(&my_device->ctrl, 0xf << 16, 6 << 16)
>>
>> with:
>>
>> clrsetfield_le32(FIELD, &my_device->ctrl, 6)
>>
>> So the two identical shifts are avoided, and the forming of the mask
>> is done once in the define.
>
> If you really insist:
>
>        #define FIELD_VAL(x)    (x << 16)
>        #define FIELD_MASK      FIELD_VAL(0xF)
>
>        clrsetbits_le32(&my_device->ctrl, FIELD_MASK, FIELD_VAL(6));

We now have a computed mask which is good, thank you.

But the FIELD is specified twice in the same statement. Can we
therefore please take this a step further and write something like
this?

clrsetfield_le32(&my_device->ctrl, FIELD, 6);

so:

#define clrsetfield_le32(addr, field, data)  clrsetbits_le32(add,
field ##_MASK, field ## _VAL(data))

How can we combine these two to remove the redundancy?

>
> In practical use cases, you will probablu not use magg numbers like
> '6' anyway, and define symbolic names for this anyway, so this would
> be:
>
>        clrsetbits_le32(&my_device->ctrl, FIELD_MASK, FIELD_VAL_FOO);

In our code there is quite a bit of both but yes setting single bit
fields to 0 for off, and 1 for on, is common. Also we sometime have
things like:

enum {
        OSC_FREQ_12,
        OSC_FREQ_16,
        OSC_FREQ_19_2,
        OSC_FREQ_24,
};

for 2-bit fields (and so on for 3, 4 bit fields).

Regards,
Simon

>
> or similar.
>
> 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
> News is what a chap who doesn't care much  about  anything  wants  to
> read. And it's only news until he's read it. After that it's dead.
>                           - Evelyn Waugh _Scoop_ (1938) bk. 1, ch. 5
>