Re: [PATCH treewide v2 1/3] bitfield: Add non-constant field_{prep,get}() helpers

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Vincent,

On Mon, 3 Feb 2025 at 14:37, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote:
> On 03/02/2025 at 16:44, Johannes Berg wrote:
> > On Sun, 2025-02-02 at 12:53 -0500, Yury Norov wrote:
> >>> Instead of creating another variant for
> >>> non-constant bitfields, wouldn't it be better to make the existing macro
> >>> accept both?
> >>
> >> Yes, it would definitely be better IMO.
> >
> > On the flip side, there have been discussions in the past (though I
> > think not all, if any, on the list(s)) about the argument order. Since
> > the value is typically not a constant, requiring the mask to be a
> > constant has ensured that the argument order isn't as easily mixed up as
> > otherwise.
>
> If this is a concern, then it can be checked with:
>
>   BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask) &&
>                    __builtin_constant_p(_val),
>                    _pfx "mask is not constant");
>
> It means that we forbid FIELD_PREP(non_const_mask, const_val) but allow
> any other combination.

Even that case looks valid to me. Actually there is already such a user
in drivers/iio/temperature/mlx90614.c:

    ret |= field_prep(chip_info->fir_config_mask, MLX90614_CONST_FIR);

So if you want enhanced safety, having both the safer/const upper-case
variants and the less-safe/non-const lower-case variants makes sense.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds