Re: [PATCH treewide v2 1/3] bitfield: Add non-constant field_{prep,get}() helpers

[David Laight]

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