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

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 28 Feb 2019, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote:
> On Wed, 27 Feb 2019 18:02:36 +0100, Jani Nikula <jani.nikula@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.

BR,
Jani.



>
> Thanks,
> Michal
>

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx