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) |	\
+		((_mask) << 16);					\
+	})
+
+/**
+ * FIELD_PREP_HI16_WE_CONST() - prepare a constant 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_CONST() 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.
+ *
+ * Unlike FIELD_PREP_HI16_WE(), this is a constant expression and can therefore
+ * be used in initializers. Error checking is less comfortable for this
+ * version, and non-constant masks cannot be used.
+ */
+#define FIELD_PREP_HI16_WE_CONST(_mask, _val)				 \
+	(								 \
+		FIELD_PREP_CONST(_mask, _val) |				 \
+		(BUILD_BUG_ON_ZERO(const_true((u64) (_mask) > U16_MAX)) + \
+		 ((_mask) << 16))					 \
+	)
+
 /**
  * FIELD_GET() - extract a bitfield element
  * @_mask: shifted mask defining the field's length and position

-- 
2.49.0

On Mon, Jun 02, 2025 at 06:19:14PM +0200, Nicolas Frattaroli wrote:
> 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.

Can you list them all explicitly please? I grepped myself for the
'HIGHWORD_UPDATE' and 'FIELD_PREP_HIGWORD', and found just 4 or 5 in
addition to the rockchip.
 
> 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.

I agree. Nice catch.

> 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.

I'm not sure that the name you've chosen reflects the intention. If
you just give me the name without any background, I'd bet it updates
the HI16 part of presumably 32-bit field. The 'WE' part here is most
likely excessive because at this level of abstraction you can't
guarantee that 'write-enable mask' is the only purpose for the macro.

> 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.

This is a wrong way to go. Once you introduce a macro that replaces
functionality of few other arch or driver macros, you should consolidate
them all in the same series. Otherwise, it will be just another flavor
of the same, but now living in a core header. 

Can you please prepare a series that introduces the new macro and
wires all arch duplications to it?
 
> 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) |	\
> +		((_mask) << 16);					\
> +	})

This pretty much is a duplication of the FIELD_PREP(), isn't? Why don't
you borrow the approach from drivers/clk/clk-sp7021.c:

	/* HIWORD_MASK FIELD_PREP */
	#define HWM_FIELD_PREP(mask, value)             \
	({                                              \
	        u64 _m = mask;                          \
	        (_m << 16) | FIELD_PREP(_m, value);     \
	})

If you do so, the existing FIELD_PREP() will do all the work without
copy-pasting. The only questionI have  to the above macro is why '_m'
is u64? Seemingly, it should be u32?

Regarding the name... I can't invent a good one as well, so the best
thing I can suggest is not to invent something that can mislead. The
HWM_FIELD_PREP() is not bad because it tells almost nothing and
encourages one to refer to the documentation. If you want something
self-explaining, maybe MASK_HI_FIELD_LO_PREP_U16(), or something? 

Thanks,
Yury

> +
> +/**
> + * FIELD_PREP_HI16_WE_CONST() - prepare a constant 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_CONST() 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.
> + *
> + * Unlike FIELD_PREP_HI16_WE(), this is a constant expression and can therefore
> + * be used in initializers. Error checking is less comfortable for this
> + * version, and non-constant masks cannot be used.
> + */
> +#define FIELD_PREP_HI16_WE_CONST(_mask, _val)				 \
> +	(								 \
> +		FIELD_PREP_CONST(_mask, _val) |				 \
> +		(BUILD_BUG_ON_ZERO(const_true((u64) (_mask) > U16_MAX)) + \
> +		 ((_mask) << 16))					 \
> +	)
> +
>  /**
>   * FIELD_GET() - extract a bitfield element
>   * @_mask: shifted mask defining the field's length and position
> 
> -- 
> 2.49.0

On Monday, 2 June 2025 22:02:19 Central European Summer Time Yury Norov wrote:
> On Mon, Jun 02, 2025 at 06:19:14PM +0200, Nicolas Frattaroli wrote:
> > 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.
> 
> Can you list them all explicitly please? I grepped myself for the
> 'HIGHWORD_UPDATE' and 'FIELD_PREP_HIGWORD', and found just 4 or 5 in
> addition to the rockchip.

Most of the ones Heiko brought up[1] just appear to be the clock stuff,
I'm only aware of the drivers/mmc/host/sdhci-of-arasan.c one outside of
Rockchip. For a complete listing I'd have to do a semantic search with
e.g. Coccinelle, which I've never used before and would need to wrap
my head around first. grep is a bad fit for catching them all as some
macros are split across lines, or reverse the operators of the OR.
Weggli[2] is another possibility but it's abandoned and undocumented, and
I've ran into its limitations before fairly quickly.

>  
> > 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.
> 
> I agree. Nice catch.
> 
> > 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.
> 
> I'm not sure that the name you've chosen reflects the intention. If
> you just give me the name without any background, I'd bet it updates
> the HI16 part of presumably 32-bit field. The 'WE' part here is most
> likely excessive because at this level of abstraction you can't
> guarantee that 'write-enable mask' is the only purpose for the macro.
> 
> > 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.
> 
> This is a wrong way to go. Once you introduce a macro that replaces
> functionality of few other arch or driver macros, you should consolidate
> them all in the same series. Otherwise, it will be just another flavor
> of the same, but now living in a core header. 
> 
> Can you please prepare a series that introduces the new macro and
> wires all arch duplications to it?

Okay, I will do that after I learn Coccinelle. Though I suspect the reason
why I'm the first person to address this is because it's much easier to
hide duplicated macros away in drivers than go the long route of fixing up
every single other user. I'm not too miffed about it though, it's cleanup
of technical debt that's long overdue.

>  
> > 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) |	\
> > +		((_mask) << 16);					\
> > +	})
> 
> This pretty much is a duplication of the FIELD_PREP(), isn't? Why don't
> you borrow the approach from drivers/clk/clk-sp7021.c:
> 
> 	/* HIWORD_MASK FIELD_PREP */
> 	#define HWM_FIELD_PREP(mask, value)             \
> 	({                                              \
> 	        u64 _m = mask;                          \
> 	        (_m << 16) | FIELD_PREP(_m, value);     \
> 	})
> 
> If you do so, the existing FIELD_PREP() will do all the work without
> copy-pasting.

Because then the __BF_FIELD_CHECK macro will be invoked twice, once without
the proper prefix. Factoring the actual prep-no-check operation out into a
separate macro is macro definition + 1-line invocation * 2, whereas copy-
pasting the implementation that will never change is 1-line invocation*2.

> The only questionI have  to the above macro is why '_m'
> is u64? Seemingly, it should be u32?

I didn't write the HWM_FIELD_PREP macro in clk-sp7021.c, nor am I familiar
with the hardware. It's possible they were trying to prevent an overflow
wraparound here though, but they're not checking if the result ends up
greater than 32 bits so that seems suspect.

> Regarding the name... I can't invent a good one as well, so the best
> thing I can suggest is not to invent something that can mislead. The
> HWM_FIELD_PREP() is not bad because it tells almost nothing and
> encourages one to refer to the documentation. If you want something
> self-explaining, maybe MASK_HI_FIELD_LO_PREP_U16(), or something?

This seems a bit unwieldy, at 25 characters. "FIELD32_HIMASK_LOPREP"
(or FIELD16, depending on which end of the cornet to eat) would be 21
characters but I'm also not in love with it.

I think the name should include the following parts:
1. it's a field
2. the field is halved into two halves of 16 bits
3. the mask is copied into the upper 16 bits

Since we're on the subject of bit widths, I have a somewhat sacrilegious
point to raise: should this be a function-like macro at all, as opposed
to a static __pure inline function? It's not generic with regards to the
data types, as we're always assuming a u16 value and mask input and a
u32 output. The __pure inline definition should let the compiler treat it
essentially similar to what the pre-processor expanded macro does, which
is as not a function call at all but a bunch of code to constant fold away
if possible. What we get in return is type checking and less awful syntax.
Then we could call it something like `himask_field_prep_u32`, which is
also 21 characters but the ambiguity of whether the u32 refers to the mask
or the whole register width is cleared up by the types of the function
arguments.

The const version of the macro may still need to remain though because I'm
not sure C11 can do that for us. With C23 maybe there's a way with
constexpr but I've never used it before.

> 
> Thanks,
> Yury
> 

Kind Regards,
Nicolas Frattaroli

Link: https://lore.kernel.org/linux-rockchip/1895349.atdPhlSkOF@diego/ [1]
Link: https://github.com/weggli-rs/weggli [2]

> > +
> > +/**
> > + * FIELD_PREP_HI16_WE_CONST() - prepare a constant 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_CONST() 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.
> > + *
> > + * Unlike FIELD_PREP_HI16_WE(), this is a constant expression and can therefore
> > + * be used in initializers. Error checking is less comfortable for this
> > + * version, and non-constant masks cannot be used.
> > + */
> > +#define FIELD_PREP_HI16_WE_CONST(_mask, _val)				 \
> > +	(								 \
> > +		FIELD_PREP_CONST(_mask, _val) |				 \
> > +		(BUILD_BUG_ON_ZERO(const_true((u64) (_mask) > U16_MAX)) + \
> > +		 ((_mask) << 16))					 \
> > +	)
> > +
> >  /**
> >   * FIELD_GET() - extract a bitfield element
> >   * @_mask: shifted mask defining the field's length and position
> > 
>

On Tue, Jun 03, 2025 at 02:55:40PM +0200, Nicolas Frattaroli wrote:
> On Monday, 2 June 2025 22:02:19 Central European Summer Time Yury Norov wrote:
> > On Mon, Jun 02, 2025 at 06:19:14PM +0200, Nicolas Frattaroli wrote:
> > > 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.
> > 
> > Can you list them all explicitly please? I grepped myself for the
> > 'HIGHWORD_UPDATE' and 'FIELD_PREP_HIGWORD', and found just 4 or 5 in
> > addition to the rockchip.
> 
> Most of the ones Heiko brought up[1] just appear to be the clock stuff,
> I'm only aware of the drivers/mmc/host/sdhci-of-arasan.c one outside of
> Rockchip. For a complete listing I'd have to do a semantic search with
> e.g. Coccinelle, which I've never used before and would need to wrap
> my head around first. grep is a bad fit for catching them all as some
> macros are split across lines, or reverse the operators of the OR.
> Weggli[2] is another possibility but it's abandoned and undocumented, and
> I've ran into its limitations before fairly quickly.

Going Coccinelle way is fine, but I think it's an endless route. :)

What I meant is: you caught this HIWORD_UPDATE() duplication, and it's
great. When people copy-paste a macro implementation and even a name,
their intention is clear: they need this functionality, but the core
headers lack it, so: I'll make another small copy in my small driver,
and nobody cares.

I think your consolidation should at first target the above users.

Those having different names or substantially different implementation,
may also be a target. But they are:
 1. Obviously a minority in terms of LOCs, and
 2. More likely have their reasons to have custom flavors of the same.

...

> > Can you please prepare a series that introduces the new macro and
> > wires all arch duplications to it?
> 
> Okay, I will do that after I learn Coccinelle. Though I suspect the reason
> why I'm the first person to address this is because it's much easier to
> hide duplicated macros away in drivers than go the long route of fixing up
> every single other user. I'm not too miffed about it though, it's cleanup
> of technical debt that's long overdue.
 
I just fired 

        $ git grep "define HIWORD"

and found 27 matches. The relevant 'hiwords' have the following
semantics:

 - HIWORD_UPDATE(val, mask, shift)
 - HIWORD_UPDATE(val, mask)
 - HIWORD_UPDATE(mask, val)
 - HIWORD_UPDATE(v, h, l)
 - HIWORD_UPDATE_BIT(val)
 - HIWORD_DISABLE_BIT(val)

Most of them don't bother doing any static checks at all.

If you will just consolidate the above, and wire those drivers
to generic version with all that checks - it would be a decent
consolidation by any measure.

Something like this:

diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 0470d7c175f4..d5e74d555a3d 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -30,7 +30,7 @@

 #define DMC_MAX_CHANNELS       4

-#define HIWORD_UPDATE(val, mask)       ((val) | (mask) << 16)
+#define HIWORD_UPDATE(val, mask)       HWORD_UPDATE(mask, val)

 /* DDRMON_CTRL */
 #define DDRMON_CTRL    0x04

...

> > Regarding the name... I can't invent a good one as well, so the best
> > thing I can suggest is not to invent something that can mislead. The
> > HWM_FIELD_PREP() is not bad because it tells almost nothing and
> > encourages one to refer to the documentation. If you want something
> > self-explaining, maybe MASK_HI_FIELD_LO_PREP_U16(), or something?
> 
> This seems a bit unwieldy, at 25 characters. "FIELD32_HIMASK_LOPREP"
> (or FIELD16, depending on which end of the cornet to eat) would be 21
> characters but I'm also not in love with it.
> 
> I think the name should include the following parts:
> 1. it's a field
> 2. the field is halved into two halves of 16 bits
> 3. the mask is copied into the upper 16 bits

Or just keep the HIWORD_UPDATE name as it already has over 300 users.
If it's commented well, and has an implementation based on FIELD_PREP,
I don't think users will struggle to understand what is actually
happening there.
 
> Since we're on the subject of bit widths, I have a somewhat sacrilegious
> point to raise: should this be a function-like macro at all, as opposed
> to a static __pure inline function? It's not generic with regards to the
> data types, as we're always assuming a u16 value and mask input and a
> u32 output. The __pure inline definition should let the compiler treat it
> essentially similar to what the pre-processor expanded macro does, which
> is as not a function call at all but a bunch of code to constant fold away
> if possible. What we get in return is type checking and less awful syntax.
> Then we could call it something like `himask_field_prep_u32`, which is
> also 21 characters but the ambiguity of whether the u32 refers to the mask
> or the whole register width is cleared up by the types of the function
> arguments.
> 
> The const version of the macro may still need to remain though because I'm
> not sure C11 can do that for us. With C23 maybe there's a way with
> constexpr but I've never used it before.

Inline function will disable parameters compile checks, and will be
too diverged from _CONST counterpart.

Thanks,
Yury

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