Re: [PATCH v2 3/7] bitfield: introduce HI16_WE bitfield prep macros

["Heiko Stübner" <heiko@sntech.de>]

Am Montag, 2. Juni 2025, 18:19:14 Mitteleuropäische Sommerzeit schrieb Nicolas Frattaroli:
> Hardware of various vendors, but very notably Rockchip, often uses
> 32-bit registers where the upper 16-bit half of the register is a
> write-enable mask for the lower half.
> 
> This type of hardware setup allows for more granular concurrent register
> write access.
> 
> Over the years, many drivers have hand-rolled their own version of this
> macro, usually without any checks, often called something like
> HIWORD_UPDATE or FIELD_PREP_HIWORD, commonly with slightly different
> semantics between them.
> 
> Clearly there is a demand for such a macro, and thus the demand should
> be satisfied in a common header file.
> 
> Add two macros: FIELD_PREP_HI16_WE, and FIELD_PREP_HI16_WE_CONST. The
> latter is a version that can be used in initializers, like
> FIELD_PREP_CONST. The macro names are chosen to explicitly reference the
> assumed half-register width, and its function, while not clashing with
> any potential other macros that drivers may already have implemented
> themselves.
> 
> Future drivers should use these macros instead of handrolling their own,
> and old drivers can be ported to the new macros as time and opportunity
> allows.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  include/linux/bitfield.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index 6d9a53db54b66c0833973c880444bd289d9667b1..2b3e7cb90ccb5d48f510104f61443b06748bb7eb 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -8,6 +8,7 @@
>  #define _LINUX_BITFIELD_H
>  
>  #include <linux/build_bug.h>
> +#include <linux/limits.h>
>  #include <linux/typecheck.h>
>  #include <asm/byteorder.h>
>  
> @@ -142,6 +143,52 @@
>  		(((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask))	\
>  	)
>  
> +/**
> + * FIELD_PREP_HI16_WE() - prepare a bitfield element with a write-enable mask
> + * @_mask: shifted mask defining the field's length and position
> + * @_val:  value to put in the field
> + *
> + * FIELD_PREP_HI16_WE() masks and shifts up the value, as well as bitwise ORs
> + * the result with the mask shifted up by 16.
> + *
> + * This is useful for a common design of hardware registers where the upper
> + * 16-bit half of a 32-bit register is used as a write-enable mask. In such a
> + * register, a bit in the lower half is only updated if the corresponding bit
> + * in the upper half is high.
> + */
> +#define FIELD_PREP_HI16_WE(_mask, _val)					\
> +	({								\
> +		__BF_FIELD_CHECK(_mask, ((u16) 0U), _val,		\
> +				 "FIELD_PREP_HI16_WE: ");		\
> +		((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask) |	\

gcc is quite adamant about suggesting more parentheses here:

../include/linux/bitfield.h:163:70: warning: suggest parentheses around arithmetic in operand of ‘|’ [-Wparentheses]
  163 |                 ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask) |  \
      |                                                                      ^
../include/soc/rockchip/mfpwm.h:225:41: note: in expansion of macro ‘FIELD_PREP_HI16_WE’
  225 | #define PWMV4_MODE(v)                   FIELD_PREP_HI16_WE(PWMV4_MODE_MASK, (v))
      |                                         ^~~~~~~~~~~~~~~~~~
../include/soc/rockchip/mfpwm.h:230:34: note: in expansion of macro ‘PWMV4_MODE’
  230 | #define PWMV4_CTRL_CONT_FLAGS   (PWMV4_MODE(PWMV4_MODE_CONT) | \
      |                                  ^~~~~~~~~~
../drivers/pwm/pwm-rockchip-v4.c:237:57: note: in expansion of macro ‘PWMV4_CTRL_CONT_FLAGS’
  237 |         mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_CTRL, PWMV4_CTRL_CONT_FLAGS);
      |                                                         ^~~~~~~~~~~~~~~~~~~~~


Heiko