Quoting Jani Nikula (2019-02-28 10:07:06) > On Thu, 28 Feb 2019, Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx> wrote: > > On Wed, 27 Feb 2019 18:02:36 +0100, Jani Nikula <jani.nikula@xxxxxxxxx> > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
