Messages in this thread | | | Subject | Re: [PATCH] bitops: add equivalent of BIT(x) for bitfields | From | Sebastian Frias <> | Date | Tue, 6 Dec 2016 15:56:53 +0100 |
| |
Hi Geert,
On 06/12/16 12:12, Geert Uytterhoeven wrote: >>> ... which means "generate a value"?? >>> >> >> Yes. >> Although I'm not sure if I understood the essence of your point. >> Are you suggesting that the name should be GENERATE_A_VALUE? > > No. I mean that "value" is a way too generic name. > Hence "GENVALUE" may be suitable for a macro local to a driver, but is way > too generic and fuzzy for a global function.
IMHO GENMASK presents the same situation, but actually I don't really mind about the name.
> >> There's already GENMASK, which "generates a mask". > > Yes. And it generates a (bit)mask, which is clear from its name. > But a "value" is just too generic for a global function, and make me think of > a pseudo-random number generator ;-)
:-)
>>> "val |= 0x5a << 12;" looks much more readable to me... >>> >> >> Well, the idea behind this is that one can use it like: >> >> (see https://marc.info/?l=linux-kernel&m=148095872915717&w=2) >> >> ... >> #define TIMEOUT_CLK_UNIT_MHZ BIT(6) >> #define BUS_CLK_FREQ_FOR_SD_CLK(x) GENVALUE(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. > > Actually I more like the SETBITFIELD name... >
Well, Linus did not like it for example. Since the start I was open to suggestions because I felt the name was not great either.
https://marc.info/?l=linux-kernel&m=148095805515487&w=2
>>> OK. So it inserts a value into a bitfield. >>> >>> Yes, that can be useful. Now let's find a sensible name for this. >>> Perhaps inspired by a PowerPC mnemonic? At least that would be more >>> obvious than "GENVALUE", IMHO... >> >> I'm open to suggestions. > > BITFIELD_INSERT()?
The problem is that right now it does not inserts into anything, it just generates a value. The user can then OR that with something else essentially "inserting" the value.
(see my reply to Borislav here: https://marc.info/?l=linux-kernel&m=148095872915717&w=2 )
This allows a use case where BIT() and GENVALUE() can be used in a similar way:
#define TIMEOUT_CLK_UNIT_MHZ BIT(6) #define BUS_CLK_FREQ_FOR_SD_CLK(x) GENVALUE(14,7,x) ... val = 0; val |= TIMEOUT_CLK_UNIT_MHZ; /* unit: MHz */ val |= BUS_CLK_FREQ_FOR_SD_CLK(200); /* SDIO clock: 200MHz */ ...
I can write BITFIELD_INSERT as well, but I would not want to have to choose between BITFIELD_INSERT and GENVALUE, because that would mean that the snippet above would become:
#define BITFIELD_INSERT(target, msb, lsb, val) \ (target = ((target & ~GENMASK(msb, lsb)) | GENVALUE(msb, lsb, val))) ... #define TIMEOUT_CLK_UNIT_MHZ BIT(6) #define BUS_CLK_FREQ_FOR_SD_CLK(y, x) BITFIELD_INSERT(y,14,7,x) ... val = 0; val |= TIMEOUT_CLK_UNIT_MHZ; /* unit: MHz */ BUS_CLK_FREQ_FOR_SD_CLK(val, 200); /* SDIO clock: 200MHz */ ...
NOTES: - Going for BITFIELD_INSERT() means that we probably need to decide its behaviour w.r.t existing bits, does it OR (thus preserving bits set) or does it overwrite? (most likely the later option) - Going for BITFIELD_INSERT() calls for its counter-part BITFIELD_EXTRACT(), so that we can do:
... val = 0x1115a000; if (BITFIELD_EXTRACT(val, 19, 12) == 0x5a) ...
Best regards,
Sebastian
| |