Re: [PATCH] drm/i915: encourage BIT() macro usage in register definitions

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

On Wed, 27 Jun 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting Michal Wajdeczko (2018-06-27 16:51:42)
>> On Wed, 27 Jun 2018 16:41:13 +0200, Jani Nikula <jani.nikula@intel.com>  
>> wrote:
>> 
>> > There's already some BIT() usage here and there, embrace it.
>> >
>> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_reg.h | 9 +++++----
>> >  1 file changed, 5 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_reg.h  
>> > b/drivers/gpu/drm/i915/i915_reg.h
>> > index 476118f46cf3..64b9c270045d 100644
>> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > @@ -65,9 +65,10 @@
>> >   * but do note that the macros may be needed to read as well as write  
>> > the
>> >   * register contents.
>> >   *
>> > - * Define bits using ``(1 << N)`` instead of ``BIT(N)``. We may change  
>> > this in
>> > - * the future, but this is the prevailing style. Do **not** add  
>> > ``_BIT`` suffix
>> > - * to the name.
>> > + * Define bits using ``BIT(N)`` instead of ``(1 << N)``. Do **not** add  
>> > ``_BIT``
>> > + * suffix to the name. Exception to ``BIT()`` usage: Value 1 for a bit  
>> > field
>> > + * should be defined using ``(1 << N)`` to be in line with other values  
>> > such as
>> > + * ``(2 << N)`` for the same field.
>> >   *
>> >   * Group the register and its contents together without blank lines,  
>> > separate
>> >   * from other registers and their contents with one blank line.
>> > @@ -105,7 +106,7 @@
>> >   *  #define _FOO_A                      0xf000
>> >   *  #define _FOO_B                      0xf001
>> >   *  #define FOO(pipe)                   _MMIO_PIPE(pipe, _FOO_A, _FOO_B)
>> > - *  #define   FOO_ENABLE                (1 << 31)
>> > + *  #define   FOO_ENABLE                BIT(31)
>> 
>> hmm, this breaks nice consistency between one- and multi-bit fields ..
>> 
>> >   *  #define   FOO_MODE_MASK             (0xf << 16)
>> 
>> .. but if you want to use macro for single bit, then maybe you should
>> also consider other existing macro for the mask definition:
>> 
>>         #define   FOO_MODE_MASK             GENMASK(19, 16)
>> 
>> >   *  #define   FOO_MODE_SHIFT            16
>> >   *  #define   FOO_MODE_BAR              (0 << 16)
>> 
>> .. but we still don't have any macro for defining multi-bit values
>> so I'm not sure if this change will make code really easier to read
>
> #include <linux/bitfield.h>
>
> I'm not sure if I'm ready to embrace that yet, but it does seem to be
> the direction we should be heading in. Primarily to check the invalid
> range checking & usage.

I guess there are two things here. Using bitfield.h macros to define our
own stuff is one thing, like so:

#define   FOO_ENABLE                BIT(31)
#define   FOO_MODE_MASK             GENMASK(19, 16)
#define   FOO_MODE_SHIFT            16
#define   FOO_MODE_BAR              FIELD_PREP(FOO_MODE_MASK, 0)
#define   FOO_MODE_BAZ              FIELD_PREP(FOO_MODE_MASK, 1)
#define   FOO_MODE_QUX_SNB          FIELD_PREP(FOO_MODE_MASK, 2)

The range checking is indeed an advantage.

Using FIELD_PREP() or FIELD_GET() in code is another, because we
currently don't define the *unshifted* field values. Everything is
defined with the shift. Defining everything unshifted and then moving
the FIELD_PREP() and FIELD_GET() in code would be quite the change.

BR,
Jani.

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