On Mon, Dec 08, 2014 at 03:21:02PM +0100, Daniel Vetter wrote:
> On Mon, Dec 08, 2014 at 01:59:50PM +0000, Damien Lespiau wrote:
> > On Mon, Dec 08, 2014 at 02:33:57PM +0200, Jani Nikula wrote:
> > > > #define _MASKED_BIT_ENABLE(a) (((a) << 16) | (a))
> > > > #define _MASKED_BIT_DISABLE(a) ((a) << 16)
> > > > +#define _MASKED_FIELD(value, mask) (((mask) << 16) | (value))
> > >
> > > Obligatory bikeshed, wouldn't you say _MASKED_BIT_{ENABLE,DISABLE} are
> > > special cases of _MASKED_FIELD...? ;)
> >
> > That's because we're not just enabling or disabling bits here but
> > setting a multi-bits value.
> >
> > _MASKED_FIELD(2 << 4, 0x3 << 4);
>
> #define _MASKED_BIT_ENABLE(a) _MASKED_FIELD(a, a)
> #define _MASKED_BIT_DISABLE(a) _MASKED_FIELD(a, 0)
Ok and I right away screwed up the argument ordering in the DISABLE one
because the bits we set are before the mask. All the bitmasking functions
we have in e.g. i915_irq.c ilk_update_gt_irq so for consistency I think we
should flip it in this one here, too. Otherwise that bit of inconsistency
will trip up tons of people in the future.
Jani, can you please apply that fixup if Damien acks it?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
