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@xxxxxxxxx>
> ---
> @@ -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?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
