On Thu, 28 Feb 2019, Michal Wajdeczko <[email protected]> wrote: > On Wed, 27 Feb 2019 18:02:36 +0100, Jani Nikula <[email protected]> > 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. BR, Jani. > > Thanks, > Michal > -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
