On Wed, 17 Aug 2016 09:33:26 -0700, Linus Torvalds wrote:
> On Wed, Aug 17, 2016 at 3:31 AM, Kalle Valo <kvalo@xxxxxxxxxxxxxx> wrote:
> >
> > Are people ok with this? I think they are useful and I can take these
> > through my tree, but I would prefer to get an ack from other maintainers
> > first. Dave? Andrew?
>
> I'm not a huge fan, since the interface fundamentally seems to be
> oddly designed (why pass in the mask rather than "start bit +
> length"?).
Would that not require start and length to have separate defines?
I assume doing:
#define REG_BLA_FIELD_FOO 3, 4
val = FIELD_GET(REG_BLA_FIELD_FOO, reg);
is not acceptable. Attempts to define a single value brought us to the
shifted mask.
> I also don't like how this very much would match the GENMASK() macros
> we have, but then it clashes with them in other ways
>
> - it's in a different header file
I'll move to bitops.h.
> - it has completely different naming (GENMASK_ULL vs FIELD_GET64}.
>
> I actually think the naming could/should be fixed by just
> automatically doing the right thing based on sizes. For example,
> GENMASK could just have something like
>
> #define GENMASK(end,start) __builtin_choose_expr((end)>31,
> __GENMASK_64(end,start), __GENMASK_32(end,start))
>
> and doing similar things with the FIELD_GET/SET ops.
>
> So I'm not entirely happy about this all.
Seems feasible.
> But if people love the interface, and think the above kind of cleanups
> might be possible, I'd just want to make sure that there is also a
>
> BUILD_BUG_ON(!__builtin_constant_p(_mask));
>
> because if the mask isn't a build-time constant, the code ends up
> being *complete* shit. Also preferably something like
>
> BUILD_BUG_ON((_mask) > (typeof(_val)~0ull);
>
> to make sure you can't try to mask bits that don't exist in the value.
>
> Or something like that to make mis-uses *really* obvious.
OK!
> The FIELD_PUT macro also seems misnamed. It doesn't "put" anything
> (unlike the GET macro). It just prepares the field for inserting
> later. As exemplified by how you actually have to put things:
>
> First, "GET" - yeah, that looks like a "get" operation:
>
> * Get:
> * a = FIELD_GET(REG_FIELD_A, reg);
>
> But then "PUT" isn't actually a PUT operation at all, but the comments
> kind of gloss over it by talking about "Modify" instead:
>
> * Modify:
> * reg &= ~REG_FIELD_C;
> * reg |= FIELD_PUT(REG_FIELD_C, c);
>
> so I'm not entirely sure about the naming.
>
> I can live with the FIELD_PUT naming, because I see how it comes
> about, even if I think it's a bit odd. I might have called it
> "FIELD_PREP" instead, but I'm not really sure that's all that much
> better.
Yes, it used to be called FIELD_SET, which was even worse. I think
PREP sounds good.
Thanks!
