[Intel-gfx] [RFC 4/4] drm/i915/registers: define field values using FIELD_PREP()

[Joonas Lahtinen]

Quoting Chris Wilson (2018-09-27 13:35:47)
> Quoting Jani Nikula (2018-09-27 10:40:23)
> > Slightly verbose, but does away with hand rolled shifts and provides
> > static checking that the values fit the mask.
> > 
> > Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> > ---
> > @@ -4650,11 +4650,11 @@ enum {
> >  #define _PP_ON_DELAYS                  0x61208
> >  #define PP_ON_DELAYS(pps_idx)          _MMIO_PPS(pps_idx, _PP_ON_DELAYS)
> >  #define  PANEL_PORT_SELECT_MASK                GENMASK(31, 30)
> > -#define  PANEL_PORT_SELECT_LVDS                (0 << 30)
> > -#define  PANEL_PORT_SELECT_DPA         (1 << 30)
> > -#define  PANEL_PORT_SELECT_DPC         (2 << 30)
> > -#define  PANEL_PORT_SELECT_DPD         (3 << 30)
> > -#define  PANEL_PORT_SELECT_VLV(port)   ((port) << 30)
> > +#define  PANEL_PORT_SELECT_LVDS                FIELD_PREP(PANEL_PORT_SELECT_MASK, 0)
> > +#define  PANEL_PORT_SELECT_DPA         FIELD_PREP(PANEL_PORT_SELECT_MASK, 1)
> > +#define  PANEL_PORT_SELECT_DPC         FIELD_PREP(PANEL_PORT_SELECT_MASK, 2)
> > +#define  PANEL_PORT_SELECT_DPD         FIELD_PREP(PANEL_PORT_SELECT_MASK, 3)
> > +#define  PANEL_PORT_SELECT_VLV(port)   FIELD_PREP(PANEL_PORT_SELECT_MASK, port)
> 
> Maybe verbose, but it reads far better as giving each field a distinct
> name ties together all the individual options.
> 
> Before seeing this I was sceptical about FIELD_PREP, no longer.
> 
> Under this construct we aren't using masks per se, but giving a name to
> a group of bits within the register (a field). So I think
> 
> #define PANEL_PORT_SELECT       GENMASK(31, 30)
> #define PANEL_PORT_SELECT_LVDS  FIELD_PREP(PANEL_PORT_SELECT, 0)
> 
> is intuitive. Did you do a quick bloatometer to see if gcc code
> generation is affected?

+1 from me for the change as big and for the above detail suggested by
Chris.

Regards, Joonas