On Fri, 31 Jan 2025 14:46:51 +0100
Geert Uytterhoeven <geert+renesas@xxxxxxxxx> wrote:
> The existing FIELD_{GET,PREP}() macros are limited to compile-time
> constants. However, it is very common to prepare or extract bitfield
> elements where the bitfield mask is not a compile-time constant.
>
> To avoid this limitation, the AT91 clock driver and several other
> drivers already have their own non-const field_{prep,get}() macros.
> Make them available for general use by consolidating them in
> <linux/bitfield.h>, and improve them slightly:
> 1. Avoid evaluating macro parameters more than once,
> 2. Replace "ffs() - 1" by "__ffs()",
> 3. Support 64-bit use on 32-bit architectures.
...
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index 63928f1732230700..c62324a9fcc81241 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -203,4 +203,38 @@ __MAKE_OP(64)
> #undef __MAKE_OP
> #undef ____MAKE_OP
>
> +/**
> + * field_prep() - prepare a bitfield element
> + * @_mask: shifted mask defining the field's length and position
> + * @_val: value to put in the field
> + *
> + * field_prep() masks and shifts up the value. The result should be
> + * combined with other fields of the bitfield using logical OR.
> + * Unlike FIELD_PREP(), @_mask is not limited to a compile-time constant.
> + */
> +#define field_prep(_mask, _val) \
You don't need an _ prefix on the 'parameters' - it doesn't gain anything.
> + ({ \
> + typeof(_mask) __mask = (_mask); \
Use: __auto_type __mask = (_mask);
> + unsigned int __shift = sizeof(_mask) <= 4 ? \
> + __ffs(__mask) : __ffs64(__mask); \
> + (((typeof(_mask))(_val) << __shift) & (__mask)); \
There are a lot of () in that line, perhaps:
__auto_type(__mask) = (_mask);
typeof (__mask) __val = (_val);
unsigned int __shift = ...;
(__val << __shift) & __mask;
Note the typeof (__mask) - avoids line-length 'bloat' when the arguments are non-trivial.
David
