Re: [PATCH] drm/i915: simplify intel_ddi_pll_select()

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

On Tue, Mar 21, 2017 at 06:57:10PM -0300, Paulo Zanoni wrote:
> Because {hsw,skl,bxt}_ddi_pll_select all pretty much do the same thing
> in slightly different ways. Replace everything with a simple copy of
> the function and inline it inside intle_ddi_pll_select().
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 58 +++++-----------------------------------
>  1 file changed, 7 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index d8214ba..bd6fd0b 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1127,47 +1127,6 @@ void intel_ddi_clock_get(struct intel_encoder *encoder,
>  		bxt_ddi_clock_get(encoder, pipe_config);
>  }
>  
> -static bool
> -hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
> -		   struct intel_crtc_state *crtc_state,
> -		   struct intel_encoder *encoder)
> -{
> -	struct intel_shared_dpll *pll;
> -
> -	pll = intel_get_shared_dpll(intel_crtc, crtc_state,
> -				    encoder);
> -	if (!pll)
> -		DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
> -				 pipe_name(intel_crtc->pipe));
> -
> -	return pll;
> -}
> -
> -static bool
> -skl_ddi_pll_select(struct intel_crtc *intel_crtc,
> -		   struct intel_crtc_state *crtc_state,
> -		   struct intel_encoder *encoder)
> -{
> -	struct intel_shared_dpll *pll;
> -
> -	pll = intel_get_shared_dpll(intel_crtc, crtc_state, encoder);
> -	if (pll == NULL) {
> -		DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
> -				 pipe_name(intel_crtc->pipe));
> -		return false;
> -	}
> -
> -	return true;
> -}
> -
> -static bool
> -bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
> -		   struct intel_crtc_state *crtc_state,
> -		   struct intel_encoder *encoder)
> -{
> -	return !!intel_get_shared_dpll(intel_crtc, crtc_state, encoder);
> -}
> -
>  /*
>   * Tries to find a *shared* PLL for the CRTC and store it in
>   * intel_crtc->ddi_pll_sel.
> @@ -1178,19 +1137,16 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
>  bool intel_ddi_pll_select(struct intel_crtc *intel_crtc,
>  			  struct intel_crtc_state *crtc_state)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
>  	struct intel_encoder *encoder =
>  		intel_ddi_get_crtc_new_encoder(crtc_state);
> +	struct intel_shared_dpll *pll;
>  
> -	if (IS_GEN9_BC(dev_priv))
> -		return skl_ddi_pll_select(intel_crtc, crtc_state,
> -					  encoder);
> -	else if (IS_GEN9_LP(dev_priv))
> -		return bxt_ddi_pll_select(intel_crtc, crtc_state,
> -					  encoder);
> -	else
> -		return hsw_ddi_pll_select(intel_crtc, crtc_state,
> -					  encoder);
> +	pll = intel_get_shared_dpll(intel_crtc, crtc_state, encoder);

I wonder if intel_ddi_pll_select() has any good reason to even exist
anymore. It's just calling this one function, so maybe we should just
call it directly?

> +	if (!pll)
> +		DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
> +				  pipe_name(intel_crtc->pipe));
> +
> +	return pll;

I would probably write 'pll != NULL' for a bit of extra clarity.

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

>  }
>  
>  void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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