On Thu, Jun 28, 2018 at 10:45:02AM -0700, Paulo Zanoni wrote: > Em Qui, 2018-06-28 às 15:03 +0300, Jani Nikula escreveu: > > 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@inte > > > > l.com> > > > > wrote: > > > > > > > > > There's already some BIT() usage here and there, embrace it. > > > > > > > > > > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > > Since I'm CC'd I guess my opinion counts here :) > > > > > > > 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 .. > > I'll agree with Michal and Chris here: I'm not a huge fan of mixing > BIT() and (x << y), I would prefer to keep the current standard, > especially since BIT() is easily blacklistable. Or fully embrace the > helper macros and abolish all sorts of (x << y). Consistency wins IMHO. +1 for the Consistency. > > > > > > > > > > * #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. > > Can't we create simple macros that cover our cases then? > > (yes, that would perhaps be more divergence from the Kernel coding > standards, which could be worse than using (x << y) that are not hidden > by nonstandard macros) I fully agree with them, so I'm actually dropping my ack while we don't find rough agreement on how to move forward without more consistency. > > > > > > BR, > > Jani. > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
