Re: [PATCH] bitops: add equivalent of BIT(x) for bitfields

[Sebastian Frias]

On 05/12/16 16:34, Borislav Petkov wrote:
> On Mon, Dec 05, 2016 at 02:36:07PM +0100, Sebastian Frias wrote:
>> + * Equivalent of BIT(x) but for contiguous bitfields
>> + * SETBITFIELD(1, 0,0xff) = 0x00000003
>> + * SETBITFIELD(3, 0,0xff) = 0x0000000f
>> + * SETBITFIELD(15,8,0xff) = 0x0000ff00
>> + * SETBITFIELD(6, 6,   1) = 0x00000040 == BIT(6)
>> + */
>> +#define SETBITFIELD(msb, lsb, val)     \
>> +	(((val) << (lsb)) & (GENMASK((msb), (lsb))))
>> +#define SETBITFIELD_ULL(msb, lsb, val) \
>> +	(((val) << (lsb)) & (GENMASK_ULL((msb), (lsb))))
> 
> What is the use case for these macros?
> 

Well, right now I'm using them for stuff like:

#define TIMEOUT_CLK_UNIT_MHZ       BIT(6)
#define BUS_CLK_FREQ_FOR_SD_CLK(x) SETBITFIELD(14,7,x)
...
    val = 0;
    val |= TIMEOUT_CLK_UNIT_MHZ;         /* unit: MHz */
    val |= BUS_CLK_FREQ_FOR_SD_CLK(200); /* SDIO clock: 200MHz */
...

which makes it very practical for writing macros for associated HW
documentation.

It is true though that the name SETBITFIELD may not be great, since the
macro is more about taking a value and massaging it to fit the given
bitfield so that it can be ORed to place said bits.
I wouldn't call it a mask, because IMHO it does not really mask (i.e.:
hides something).

> I'm thinking it would be better if they were really generic so that I
> can be able to hand in a value and to say, set bits from [lsb, msb] to
> this bit pattern. Because then you'd have to punch a hole in the value
> with a mask and then OR-in the actual mask you want to have.
> 
> I.e., something like this:
> 
> #define SET_BITMASK(val, msb, lsb, mask)        \
>         (val = (val & ~(GENMASK(msb, lsb))) | (mask << lsb))
> 
> So that you can be able to do:
> 
> unsigned int val = 0x11110000;
> 
> SET_BITMASK(val, 19, 12, 0x5a)
> val = 0x1115a000;
> 
> I'm not sure, though, whether this won't make the actual use too cryptic
> and people having to go look it up each time they use it instead of
> actually doing the ORing in by hand which everyone can understand from
> staring at it instead of seeing a funny macro SET_BITMASK().
> 

Well, we could have both, with your proposed SET_BITMASK() building on top
of the proposed SETBITFIELD:

#define SET_BITMASK(target, msb, lsb, val) \
   (target = (target | SETBITFIELD(msb, lsb, val)))

> Actually, you could simply add another macro which is called
> 
> GENMASK_MASK(msb, lsb, mask)
> 
> or so (yeah, yucky name) which gives you that arbitrary mask which you
> then can use anywhere.
> 
> Because then it becomes more readable:
> 
>         val &= ~GENMASK(19, 12);
>         val |= GENMASK_MASK(19, 12, 0xa5);
> 
> Now you know exactly what happens: first line punches the hole, second
> ORs in the new mask.
> 
> Hmmm...
> 

Ok, so you are basically proposing to keep the macro, but with a different
name (SETBITFIELD to GENMASK_MASK).

I think GENMASK_MASK may not be much better than SETBITFIELD.