[Intel-gfx] [PATCH v3 1/3] drm/i915: introduce REG_BIT() and REG_GENMASK() to define register contents

[Chris Wilson]

Quoting Jani Nikula (2019-02-28 10:07:06)
> On Thu, 28 Feb 2019, Michal Wajdeczko <michal.wajdeczko at intel.com> wrote:
> > On Wed, 27 Feb 2019 18:02:36 +0100, Jani Nikula <jani.nikula at intel.com>  
> > wrote:
> >
> >> @@ -116,6 +116,34 @@
> >>   *  #define GEN8_BAR                    _MMIO(0xb888)
> >>   */
> >> +/**
> >> + * REG_BIT() - Prepare a u32 bit value
> >> + * @__n: 0-based bit number
> >> + *
> >> + * Local wrapper for BIT() to force u32, with compile time checks.
> >> + *
> >> + * @return: Value with bit @__n set.
> >> + */
> >> +#define REG_BIT(__n)                                                        \
> >> +    ((u32)(BIT(__n) +                                               \
> >> +           BUILD_BUG_ON_ZERO(__builtin_constant_p(__n) &&           \
> >> +                             ((__n) < 0 || (__n) > 31))))
> >
> > Maybe to simplify the code we can define this macro using macro below:
> >
> > #define REG_BIT(__n) REG_GENMASK(__n, __n)
> 
> I don't want to limit the macro to constant expressions (even if that's
> the most common use for us), and in non-constant expressions the simple
> shift becomes unnecessarily complicated with GENMASK. Plus there's the
> double evaluation of __n.
> 
> >
> >> +
> >> +/**
> >> + * REG_GENMASK() - Prepare a continuous u32 bitmask
> >> + * @__high: 0-based high bit
> >> + * @__low: 0-based low bit
> >> + *
> >> + * Local wrapper for GENMASK() to force u32, with compile time checks.
> >> + *
> >> + * @return: Continuous bitmask from @__high to @__low, inclusive.
> >> + */
> >> +#define REG_GENMASK(__high, __low)                                  \
> >> +    ((u32)(GENMASK(__high, __low) +                                 \
> >> +           BUILD_BUG_ON_ZERO(__builtin_constant_p(__high) &&        \
> >> +                             __builtin_constant_p(__low) &&         \
> >> +                             ((__low) < 0 || (__high) > 31 || (__low) > (__high)))))
> >> +
> >
> > nit: Since we are defining new set of macros, do we really have to follow
> > naming of the underlying macros? maybe we can can have clear new names:
> >
> >       REG_BIT(n)
> >       REG_BITS(hi,low)
> 
> We've pretty much been having this conversation ever since the first RFC
> was posted. It could be BITS, MASK, GENMASK, FIELD (except that clashes
> with REG_FIELD from regmap.h), BITFIELD, whatnot. And next thing you
> know, we look at REG_FIELD_PREP and REG_FIELD_GET and wonder if we
> should have our own names for them too. REG_BITS_PREP? REG_BITS_VALUE?
> REG_BITS_GET?
> 
> We *might* be able to come up with internally consistent naming
> everyone's happy with, and have those names grow on people, but based on
> the discussion so far I'm not optimistic.
> 
> So basically I gave up on that, and with the current proposal, the names
> are the same as the widely used kernel macros, with REG_ prefix
> added. If you know what they do, you know what these do. It's still
> consistent, just in a different way.
> 
> Also, I pretty much expect our code outside of i915_reg.h to use a mix
> of our versions and the underlying ones. And I'm not sure I want to
> start policing people to use our versions exclusively. If the names
> differ only with the REG_ part, I think the mix will be much easier to
> live with.

+1

An alternative naming scheme might be:

BIT_U32
GENMASK_U32
FIELD_GET_U32
FIELD_PREP_U32

(which extend naturally to 16b registers etc) and perhaps more obvious
what the nature of the change over the common macros. The U is in all
likelihood redundant, so even

BIT32
GENMASK32
FIELD32_GET
FIELD32_PREP

?

But I think the central tenant is that we want to use the common naming
scheme so that we can trivially go from GENMASK to REG_GENMASK/GENMASK32
and that other people reading our code already know the language (plus
we want these to be part of include/linux and out of our hair!)
-Chris