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

Heiko Stübner
2025-06-02T19:04:47Z

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