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

[Yury Norov <yury.norov@gmail.com>]

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