[Intel-gfx] [PATCH] drm/i915: encourage BIT() macro usage in register definitions
Chris Wilson
chris at chris-wilson.co.uk
Wed Jun 27 15:59:39 UTC 2018
Quoting Michal Wajdeczko (2018-06-27 16:51:42)
> On Wed, 27 Jun 2018 16:41:13 +0200, Jani Nikula <jani.nikula at intel.com>
> wrote:
>
> > There's already some BIT() usage here and there, embrace it.
> >
> > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > Signed-off-by: Jani Nikula <jani.nikula at 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.
-Chris
More information about the Intel-gfx
mailing list