Re: [PATCH] drm/i915: Remove unused function intel_ddi_get_link_dpll()

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Thu, 2017-02-09 at 16:47 +0200, Jani Nikula wrote:
> On Thu, 09 Feb 2017, Ander Conselvan De Oliveira <conselvan2@gmail.com> wrote:
> > On Fri, 2017-01-13 at 14:20 +0200, Ander Conselvan de Oliveira wrote:
> > > The function intel_ddi_get_link_dpll() was added in f169660ed4e5
> > > ("drm/i915/dp: Add a standalone function to obtain shared dpll for
> > > HSW/BDW/SKL/BXT") to "allow for the implementation of a platform
> > > neutral upfront link training function", but such implementation
> > > never landed.
> > 
> > Ping? Is there any plan of using that function?
> 
> Just get rid of it. If it's needed, it can be resurrected. One of the
> great advantages of kernel work is that you can look at all the users of
> the kernel internal code, and refactor based on that. How do you
> refactor code that is not used? How do you know it even works? How do
> you figure out what ideas there were for the use of such code? Just
> remove it.
> 
> Acked-by: Jani Nikula <jani.nikula@intel.com>

Thanks. Patch pushed.

Ander

> 
> 
> > 
> > Ander
> > 
> > > 
> > > So remove that function and clean up the exported shared DPLL interface.
> > > 
> > > Fixes: f169660ed4e5 ("drm/i915/dp: Add a standalone function to obtain
> > > shared dpll for HSW/BDW/SKL/BXT")
> > > Cc: Durgadoss R <durgadoss.r@intel.com>
> > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > Cc: Jim Bride <jim.bride@linux.intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Cc: intel-gfx@lists.freedesktop.org
> > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@in
> > > tel.
> > > com>
> > > 
> > > --
> > > 
> > > I omitted "Cc: <stable@vger.kernel.org> # v4.9+" from dim fixes output,
> > > since this just deletes dead code.
> > > ---
> > >  drivers/gpu/drm/i915/intel_ddi.c      | 39 --------------------------
> > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 52 ++++++----------------------
> > > ----
> > > ---
> > >  drivers/gpu/drm/i915/intel_dpll_mgr.h | 16 -----------
> > >  3 files changed, 8 insertions(+), 99 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > index 66b367d..3eba5bf 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -2126,45 +2126,6 @@ intel_ddi_init_hdmi_connector(struct
> > > intel_digital_port
> > > *intel_dig_port)
> > >  	return connector;
> > >  }
> > >  
> > > -struct intel_shared_dpll *
> > > -intel_ddi_get_link_dpll(struct intel_dp *intel_dp, int clock)
> > > -{
> > > -	struct intel_connector *connector = intel_dp->attached_connector;
> > > -	struct intel_encoder *encoder = connector->encoder;
> > > -	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > -	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > > -	struct intel_shared_dpll *pll = NULL;
> > > -	struct intel_shared_dpll_state tmp_pll_state;
> > > -	enum intel_dpll_id dpll_id;
> > > -
> > > -	if (IS_GEN9_LP(dev_priv)) {
> > > -		dpll_id =  (enum intel_dpll_id)dig_port->port;
> > > -		/*
> > > -		 * Select the required PLL. This works for platforms
> > > where
> > > -		 * there is no shared DPLL.
> > > -		 */
> > > -		pll = &dev_priv->shared_dplls[dpll_id];
> > > -		if (WARN_ON(pll->active_mask)) {
> > > -
> > > -			DRM_ERROR("Shared DPLL in use. active_mask:%x\n",
> > > -				  pll->active_mask);
> > > -			return NULL;
> > > -		}
> > > -		tmp_pll_state = pll->state;
> > > -		if (!bxt_ddi_dp_set_dpll_hw_state(clock,
> > > -						  &pll->state.hw_state))
> > > {
> > > -			DRM_ERROR("Could not setup DPLL\n");
> > > -			pll->state = tmp_pll_state;
> > > -			return NULL;
> > > -		}
> > > -	} else if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) {
> > > -		pll = skl_find_link_pll(dev_priv, clock);
> > > -	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> > > -		pll = hsw_ddi_dp_get_dpll(encoder, clock);
> > > -	}
> > > -	return pll;
> > > -}
> > > -
> > >  void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
> > >  {
> > >  	struct intel_digital_port *intel_dig_port;
> > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > index c92a255..4388c21 100644
> > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > @@ -42,44 +42,6 @@
> > >   * commit phase.
> > >   */
> > >  
> > > -struct intel_shared_dpll *
> > > -skl_find_link_pll(struct drm_i915_private *dev_priv, int clock)
> > > -{
> > > -	struct intel_shared_dpll *pll = NULL;
> > > -	struct intel_dpll_hw_state dpll_hw_state;
> > > -	enum intel_dpll_id i;
> > > -	bool found = false;
> > > -
> > > -	if (!skl_ddi_dp_set_dpll_hw_state(clock, &dpll_hw_state))
> > > -		return pll;
> > > -
> > > -	for (i = DPLL_ID_SKL_DPLL1; i <= DPLL_ID_SKL_DPLL3; i++) {
> > > -		pll = &dev_priv->shared_dplls[i];
> > > -
> > > -		/* Only want to check enabled timings first */
> > > -		if (pll->state.crtc_mask == 0)
> > > -			continue;
> > > -
> > > -		if (memcmp(&dpll_hw_state, &pll->state.hw_state,
> > > -			   sizeof(pll->state.hw_state)) == 0) {
> > > -			found = true;
> > > -			break;
> > > -		}
> > > -	}
> > > -
> > > -	/* Ok no matching timings, maybe there's a free one? */
> > > -	for (i = DPLL_ID_SKL_DPLL1;
> > > -	     ((found == false) && (i <= DPLL_ID_SKL_DPLL3)); i++) {
> > > -		pll = &dev_priv->shared_dplls[i];
> > > -		if (pll->state.crtc_mask == 0) {
> > > -			pll->state.hw_state = dpll_hw_state;
> > > -			break;
> > > -		}
> > > -	}
> > > -
> > > -	return pll;
> > > -}
> > > -
> > >  static void
> > >  intel_atomic_duplicate_dpll_state(struct drm_i915_private *dev_priv,
> > >  				  struct intel_shared_dpll_state
> > > *shared_dpll)
> > > @@ -811,8 +773,8 @@ static struct intel_shared_dpll
> > > *hsw_ddi_hdmi_get_dpll(int
> > > clock,
> > >  	return pll;
> > >  }
> > >  
> > > -struct intel_shared_dpll *hsw_ddi_dp_get_dpll(struct intel_encoder
> > > *encoder,
> > > -					      int clock)
> > > +static struct intel_shared_dpll *
> > > +hsw_ddi_dp_get_dpll(struct intel_encoder *encoder, int clock)
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > >  	struct intel_shared_dpll *pll;
> > > @@ -1360,8 +1322,9 @@ static bool skl_ddi_hdmi_pll_dividers(struct
> > > intel_crtc
> > > *crtc,
> > >  }
> > >  
> > >  
> > > -bool skl_ddi_dp_set_dpll_hw_state(int clock,
> > > -				  struct intel_dpll_hw_state
> > > *dpll_hw_state)
> > > +static bool
> > > +skl_ddi_dp_set_dpll_hw_state(int clock,
> > > +			     struct intel_dpll_hw_state *dpll_hw_state)
> > >  {
> > >  	uint32_t ctrl1;
> > >  
> > > @@ -1816,8 +1779,9 @@ static bool bxt_ddi_set_dpll_hw_state(int clock,
> > >  	return true;
> > >  }
> > >  
> > > -bool bxt_ddi_dp_set_dpll_hw_state(int clock,
> > > -			  struct intel_dpll_hw_state *dpll_hw_state)
> > > +static bool
> > > +bxt_ddi_dp_set_dpll_hw_state(int clock,
> > > +			     struct intel_dpll_hw_state *dpll_hw_state)
> > >  {
> > >  	struct bxt_clk_div clk_div = {0};
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > > b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > > index af1497e..f8d13a9 100644
> > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > > @@ -282,20 +282,4 @@ void intel_shared_dpll_init(struct drm_device *dev);
> > >  void intel_dpll_dump_hw_state(struct drm_i915_private *dev_priv,
> > >  			      struct intel_dpll_hw_state *hw_state);
> > >  
> > > -/* BXT dpll related functions */
> > > -bool bxt_ddi_dp_set_dpll_hw_state(int clock,
> > > -			  struct intel_dpll_hw_state *dpll_hw_state);
> > > -
> > > -
> > > -/* SKL dpll related functions */
> > > -bool skl_ddi_dp_set_dpll_hw_state(int clock,
> > > -				  struct intel_dpll_hw_state
> > > *dpll_hw_state);
> > > -struct intel_shared_dpll *skl_find_link_pll(struct drm_i915_private
> > > *dev_priv,
> > > -					    int clock);
> > > -
> > > -
> > > -/* HSW dpll related functions */
> > > -struct intel_shared_dpll *hsw_ddi_dp_get_dpll(struct intel_encoder
> > > *encoder,
> > > -					      int clock);
> > > -
> > >  #endif /* _INTEL_DPLL_MGR_H_ */
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx