Re: [PATCH 5/8] drm/i915: Consolidate {bxt, cnl, icl}_uninit_cdclk

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Sep 06, 2019 at 05:21:40PM -0700, Matt Roper wrote:
> The uninitialize flow is the same on all of these platforms, aside from
> calculating a different frequency level.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 48 +++++++---------------
>  1 file changed, 14 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index f8c2a706990b..a70fec82d2bc 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1692,7 +1692,18 @@ static void bxt_uninit_cdclk(struct drm_i915_private *dev_priv)
>  
>  	cdclk_state.cdclk = cdclk_state.bypass;
>  	cdclk_state.vco = 0;
> -	cdclk_state.voltage_level = bxt_calc_voltage_level(cdclk_state.cdclk);
> +	if (IS_ELKHARTLAKE(dev_priv))
> +		cdclk_state.voltage_level =
> +			ehl_calc_voltage_level(cdclk_state.cdclk);
> +	else if (INTEL_GEN(dev_priv) >= 11)
> +		cdclk_state.voltage_level =
> +			icl_calc_voltage_level(cdclk_state.cdclk);
> +	else if (INTEL_GEN(dev_priv) >= 10)
> +		cdclk_state.voltage_level =
> +			cnl_calc_voltage_level(cdclk_state.cdclk);
> +	else
> +		cdclk_state.voltage_level =
> +			bxt_calc_voltage_level(cdclk_state.cdclk);
>  
>  	bxt_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
>  }
> @@ -1738,22 +1749,6 @@ static void icl_init_cdclk(struct drm_i915_private *dev_priv)
>  	bxt_set_cdclk(dev_priv, &sanitized_state, INVALID_PIPE);
>  }
>  
> -static void icl_uninit_cdclk(struct drm_i915_private *dev_priv)
> -{
> -	struct intel_cdclk_state cdclk_state = dev_priv->cdclk.hw;
> -
> -	cdclk_state.cdclk = cdclk_state.bypass;
> -	cdclk_state.vco = 0;
> -	if (IS_ELKHARTLAKE(dev_priv))
> -		cdclk_state.voltage_level =
> -			ehl_calc_voltage_level(cdclk_state.cdclk);
> -	else
> -		cdclk_state.voltage_level =
> -			icl_calc_voltage_level(cdclk_state.cdclk);
> -
> -	bxt_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
> -}
> -
>  static void cnl_init_cdclk(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_cdclk_state cdclk_state;
> @@ -1773,17 +1768,6 @@ static void cnl_init_cdclk(struct drm_i915_private *dev_priv)
>  	bxt_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
>  }
>  
> -static void cnl_uninit_cdclk(struct drm_i915_private *dev_priv)
> -{
> -	struct intel_cdclk_state cdclk_state = dev_priv->cdclk.hw;
> -
> -	cdclk_state.cdclk = cdclk_state.bypass;
> -	cdclk_state.vco = 0;
> -	cdclk_state.voltage_level = cnl_calc_voltage_level(cdclk_state.cdclk);
> -
> -	bxt_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE);
> -}
> -
>  /**
>   * intel_cdclk_init - Initialize CDCLK
>   * @i915: i915 device
> @@ -1814,14 +1798,10 @@ void intel_cdclk_init(struct drm_i915_private *i915)
>   */
>  void intel_cdclk_uninit(struct drm_i915_private *i915)
>  {
> -	if (INTEL_GEN(i915) >= 11)
> -		icl_uninit_cdclk(i915);
> -	else if (IS_CANNONLAKE(i915))
> -		cnl_uninit_cdclk(i915);
> +	if (IS_GEN9_LP(i915) || INTEL_GEN(i915) >= 10)

I think the general concensus has been to list platforms
from new to old. Might be good to stick to that even
within conditions like this. There may have been other cases like this
in these patches that I didn't spot.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +		bxt_uninit_cdclk(i915);
>  	else if (IS_GEN9_BC(i915))
>  		skl_uninit_cdclk(i915);
> -	else if (IS_GEN9_LP(i915))
> -		bxt_uninit_cdclk(i915);
>  }
>  
>  /**
> -- 
> 2.20.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx