Em Qui, 2018-06-28 às 15:03 +0300, Jani Nikula escreveu: > On Wed, 27 Jun 2018, Chris Wilson <ch...@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@inte > > > l.com> > > > wrote: > > > > > > > There's already some BIT() usage here and there, embrace it. > > > > > > > > Cc: Paulo Zanoni <paulo.r.zan...@intel.com>
Since I'm CC'd I guess my opinion counts here :) > > > > Signed-off-by: Jani Nikula <jani.nik...@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 .. 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. > > > > > > > * #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) > > BR, > Jani. > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
