Re: [RFC PATCH 1/3] drm/i915: move backlight enable later in vlv enable sequence

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

On Tue, Sep 03, 2013 at 02:43:37PM +0300, Jani Nikula wrote:
> Follow-up to
> commit 5004945f1d6c0282c0288afa89ad85d7f2bea4d5
> Author: Jani Nikula <jani.nikula@intel.com>
> Date:   Tue Jul 30 12:20:32 2013 +0300
> 
>     drm/i915: move encoder->enable callback later in VLV crtc enable
> 
> Reference: http://mid.gmane.org/CAKMK7uFs9EMvMW8BnS24e5UNm1D7JrfVg3SD5SDFtVEamGfOOg@mail.gmail.com
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index c192dbb..adbe7bc 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1717,11 +1717,17 @@ static void intel_enable_dp(struct intel_encoder *encoder)
>  	ironlake_edp_panel_vdd_off(intel_dp, true);
>  	intel_dp_complete_link_train(intel_dp);
>  	intel_dp_stop_link_train(intel_dp);
> -	ironlake_edp_backlight_on(intel_dp);
> +
> +	/* this function is called from vlv_pre_enable_dp() */
> +	if (!IS_VALLEYVIEW(dev))
> +		ironlake_edp_backlight_on(intel_dp);
>  }
>  
>  static void vlv_enable_dp(struct intel_encoder *encoder)
>  {
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> +
> +	ironlake_edp_backlight_on(intel_dp);
>  }

This feels a bit off to me since we've already started to use
separate functions for VLV vs. others. So how about something a bit
more in line with that design:

 	intel_enable_dp() {
 		...
-		ironlake_edp_backlight_on()
 	}
 
+	g4x_enable_dp() {
+		intel_enable_dp()
+		ironlake_edp_backlight_on()
+	}
 
 	vlv_enable_dp() {
+		ironlake_edp_backlight_on()
 	}
...
-		intel_encoder->enable = intel_enable_dp;
+		intel_encoder->enable = g4x_enable_dp;

And also perhaps s/intel_pre_enable_dp/g4x_pre_enable_dp/
and s/intel_dp_pre_pll_enable/vlv_dp_pre_pll_enable/
just to keep the naming somewhat predictable.

Also we could kill the !IS_VLV check from intel_dp_pre_pll_enable()
since we don't plug that guy in except on VLV.

>  
>  static void intel_pre_enable_dp(struct intel_encoder *encoder)
> -- 
> 1.7.9.5

-- 
Ville Syrjälä
Intel OTC