Re: [PATCH v1 1/1] regmap: Add REG_FIELD_GENMASK() helper macro

[Victor Chernyakin]

> > + #define REG_FIELD_GENMASK(_reg_field)       GENMASK((_reg_field).msb, (_reg_field).lsb)
>
> This doesn't look good as it implies that _reg_field can be an expression that
> will be evaluated twice.
>
> > (In the patch you link to, I wrote it without parens at first too,
> > then had to figure out why I was getting "too many arguments to macro"
> > error messages)
>
> I have compiled it with GCC-15 and clang-22 (in both cases sparse enabled),
> no such a problem (It's also -Werror build). Can you elaborate?

In that patch we do:

#define MSA311_F_SOFT_RESET_I2C	REG_FIELD(MSA311_SOFT_RESET_REG, 2, 2)
...
MSA311_GENMASK(MSA311_F_SOFT_RESET_I2C)

which, without extra parens, expands like so:

GENMASK((struct reg_field){.lsb = ..., .msb = ...}.msb, (struct reg_field){lsb = ..., .msb = ...}.lsb)

which breaks because of the commas between the designated initializers.

> > BTW, I've been thinking about making reg_field store the mask directly,
> > by putting GENMASK(lsb, msb) in the REG_FIELD macro, but that's probably
> > a more involved change.
>
> But it's already there as you see in the regmap core. What you want is to make
> it at the beginning. Not sure if it's feasible and needed change.

If all the places where reg_field is used want a mask and not an msb+lsb pair
(I'm not certain that's the case, would have to check), I do think it would be a small
but worthwhile improvement to store it directly.