On Wed, 27 Jun 2018, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote: > Quoting Michal Wajdeczko (2018-06-27 16:51:42) >> On Wed, 27 Jun 2018 16:41:13 +0200, Jani Nikula <jani.nikula@xxxxxxxxx> >> wrote: >> >> > There's already some BIT() usage here and there, embrace it. >> > >> > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> >> > Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx> >> > --- >> > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
