On Mon, 25 Feb 2019, Ville Syrjala <ville.syrj...@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrj...@linux.intel.com>
>
> Make the code less repetitive by extracting a few small helpers.
>
> Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 68 +++++++++++++++++++++------------
> 1 file changed, 43 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 48c6bc44072d..b94bf475b04c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1068,16 +1068,42 @@ static void intel_sanitize_options(struct
> drm_i915_private *dev_priv)
> intel_gvt_sanitize_options(dev_priv);
> }
>
> -static int skl_get_dimm_ranks(u8 size, u32 rank)
> +static int skl_get_dimm_size(u16 val)
> {
> - if (size == 0)
> + return val & SKL_DRAM_SIZE_MASK;
> +}
> +
> +static int skl_get_dimm_width(u16 val)
> +{
> + if (skl_get_dimm_size(val) == 0)
> return 0;
> - if (rank == SKL_DRAM_RANK_SINGLE)
> - return 1;
> - else if (rank == SKL_DRAM_RANK_DUAL)
> - return 2;
>
> - return 0;
> + switch (val & SKL_DRAM_WIDTH_MASK) {
> + case SKL_DRAM_WIDTH_X8:
> + case SKL_DRAM_WIDTH_X16:
> + case SKL_DRAM_WIDTH_X32:
> + val = (val & SKL_DRAM_WIDTH_MASK) >> SKL_DRAM_WIDTH_SHIFT;
> + return 8 << val;
> + default:
> + MISSING_CASE(val);
> + return 0;
> + }
> +}
> +
> +static int skl_get_dimm_ranks(u16 val)
> +{
> + if (skl_get_dimm_size(val) == 0)
> + return 0;
> +
> + switch (val & SKL_DRAM_RANK_MASK) {
> + case SKL_DRAM_RANK_SINGLE:
> + case SKL_DRAM_RANK_DUAL:
> + val = (val & SKL_DRAM_RANK_MASK) >> SKL_DRAM_RANK_SHIFT;
> + return val + 1;
> + default:
> + MISSING_CASE(val);
> + return 0;
> + }
I don't much care for this dual use of both the macro and then the
calculation. I'd either just calculate, or return pre-calculated values
from the cases, not both. The missing cases can also never happen.
But it all checks out, so
Reviewed-by: Jani Nikula <jani.nik...@intel.com>
> }
>
> static bool
> @@ -1098,30 +1124,22 @@ skl_is_16gb_dimm(u8 ranks, u8 size, u8 width)
> static int
> skl_dram_get_channel_info(struct dram_channel_info *ch, u32 val)
> {
> - u32 tmp_l, tmp_s;
> - u32 s_val = val >> SKL_DRAM_S_SHIFT;
> + u16 tmp_l, tmp_s;
>
> - if (!val)
> - return -EINVAL;
> + tmp_l = val & 0xffff;
> + tmp_s = val >> 16;
>
> - tmp_l = val & SKL_DRAM_SIZE_MASK;
> - tmp_s = s_val & SKL_DRAM_SIZE_MASK;
> + ch->l_info.size = skl_get_dimm_size(tmp_l);
> + ch->s_info.size = skl_get_dimm_size(tmp_s);
>
> - if (tmp_l == 0 && tmp_s == 0)
> + if (ch->l_info.size == 0 && ch->s_info.size == 0)
> return -EINVAL;
>
> - ch->l_info.size = tmp_l;
> - ch->s_info.size = tmp_s;
> -
> - tmp_l = (val & SKL_DRAM_WIDTH_MASK) >> SKL_DRAM_WIDTH_SHIFT;
> - tmp_s = (s_val & SKL_DRAM_WIDTH_MASK) >> SKL_DRAM_WIDTH_SHIFT;
> - ch->l_info.width = (1 << tmp_l) * 8;
> - ch->s_info.width = (1 << tmp_s) * 8;
> + ch->l_info.width = skl_get_dimm_width(tmp_l);
> + ch->s_info.width = skl_get_dimm_width(tmp_s);
>
> - tmp_l = val & SKL_DRAM_RANK_MASK;
> - tmp_s = s_val & SKL_DRAM_RANK_MASK;
> - ch->l_info.ranks = skl_get_dimm_ranks(ch->l_info.size, tmp_l);
> - ch->s_info.ranks = skl_get_dimm_ranks(ch->s_info.size, tmp_s);
> + ch->l_info.ranks = skl_get_dimm_ranks(tmp_l);
> + ch->s_info.ranks = skl_get_dimm_ranks(tmp_s);
>
> if (ch->l_info.ranks == 2 || ch->s_info.ranks == 2)
> ch->ranks = 2;
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
