| Message ID | 1408470437-12107-1-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2014-08-19 at 20:47 +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Jani wanted some comments to explain why we call certain vdd on/off > functions in certain places. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 1067082..1233a10 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1315,6 +1315,7 @@ static u32 ironlake_get_pp_control(struct intel_dp *intel_dp) > return control; > } > > +/* should be paired with edp_panel_vdd_off() */ It would be useful to clarify here the purpose of the edp_ vs. intel_edp_* versions. Iiuc you need to hold pps_lock around the whole edp_panel_vdd_on/off() sequence and these can be nested within an intel_edp_panel_vdd_on/off() sequence. Otoh, you can't nest intel_edp_panel_vdd_on() calls. > static bool edp_panel_vdd_on(struct intel_dp *intel_dp) > { > struct drm_device *dev = intel_dp_to_dev(intel_dp); > @@ -1365,6 +1366,7 @@ static bool edp_panel_vdd_on(struct intel_dp *intel_dp) > return need_to_disable; > } > > +/* should be paired with intel_edp_panel_vdd_off() */ > void intel_edp_panel_vdd_on(struct intel_dp *intel_dp) > { > struct drm_i915_private *dev_priv = > @@ -1447,6 +1449,7 @@ static void edp_panel_vdd_schedule_off(struct intel_dp *intel_dp) > schedule_delayed_work(&intel_dp->panel_vdd_work, delay); > } > > +/* should be paired with edp_panel_vdd_on() */ > static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync) > { > struct drm_i915_private *dev_priv = > @@ -1467,6 +1470,7 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync) > edp_panel_vdd_schedule_off(intel_dp); > } > > +/* should be paired with intel_edp_panel_vdd_on() */ > static void intel_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync) > { > struct drm_i915_private *dev_priv = > @@ -4307,6 +4311,10 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder) > drm_encoder_cleanup(encoder); > if (is_edp(intel_dp)) { > cancel_delayed_work_sync(&intel_dp->panel_vdd_work); > + /* > + * vdd might still be enabled do to the delayed vdd off. s/do/due/ > + * Make sure vdd is actually turned off here. > + */ > mutex_lock(&dev_priv->pps_mutex); > edp_panel_vdd_off_sync(intel_dp); > mutex_unlock(&dev_priv->pps_mutex); > @@ -4327,6 +4335,10 @@ static void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder) > if (!is_edp(intel_dp)) > return; > > + /* > + * vdd might still be enabled do to the delayed vdd off. > + * Make sure vdd is actually turned off here. > + */ > mutex_lock(&dev_priv->pps_mutex); > edp_panel_vdd_off_sync(intel_dp); > mutex_unlock(&dev_priv->pps_mutex); > @@ -5025,6 +5037,10 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > drm_dp_aux_unregister(&intel_dp->aux); > if (is_edp(intel_dp)) { > cancel_delayed_work_sync(&intel_dp->panel_vdd_work); > + /* > + * vdd might still be enabled do to the delayed vdd off. > + * Make sure vdd is actually turned off here. > + */ > mutex_lock(&dev_priv->pps_mutex); > edp_panel_vdd_off_sync(intel_dp); > mutex_unlock(&dev_priv->pps_mutex); edp_panel_vdd_off_sync() could also use a clarification similar to the the rest of the API. With or without the above changes: Reviewed-by: Imre Deak <imre.deak@intel.com>
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 1067082..1233a10 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1315,6 +1315,7 @@ static u32 ironlake_get_pp_control(struct intel_dp *intel_dp) return control; } +/* should be paired with edp_panel_vdd_off() */ static bool edp_panel_vdd_on(struct intel_dp *intel_dp) { struct drm_device *dev = intel_dp_to_dev(intel_dp); @@ -1365,6 +1366,7 @@ static bool edp_panel_vdd_on(struct intel_dp *intel_dp) return need_to_disable; } +/* should be paired with intel_edp_panel_vdd_off() */ void intel_edp_panel_vdd_on(struct intel_dp *intel_dp) { struct drm_i915_private *dev_priv = @@ -1447,6 +1449,7 @@ static void edp_panel_vdd_schedule_off(struct intel_dp *intel_dp) schedule_delayed_work(&intel_dp->panel_vdd_work, delay); } +/* should be paired with edp_panel_vdd_on() */ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync) { struct drm_i915_private *dev_priv = @@ -1467,6 +1470,7 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync) edp_panel_vdd_schedule_off(intel_dp); } +/* should be paired with intel_edp_panel_vdd_on() */ static void intel_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync) { struct drm_i915_private *dev_priv = @@ -4307,6 +4311,10 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder) drm_encoder_cleanup(encoder); if (is_edp(intel_dp)) { cancel_delayed_work_sync(&intel_dp->panel_vdd_work); + /* + * vdd might still be enabled do to the delayed vdd off. + * Make sure vdd is actually turned off here. + */ mutex_lock(&dev_priv->pps_mutex); edp_panel_vdd_off_sync(intel_dp); mutex_unlock(&dev_priv->pps_mutex); @@ -4327,6 +4335,10 @@ static void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder) if (!is_edp(intel_dp)) return; + /* + * vdd might still be enabled do to the delayed vdd off. + * Make sure vdd is actually turned off here. + */ mutex_lock(&dev_priv->pps_mutex); edp_panel_vdd_off_sync(intel_dp); mutex_unlock(&dev_priv->pps_mutex); @@ -5025,6 +5037,10 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, drm_dp_aux_unregister(&intel_dp->aux); if (is_edp(intel_dp)) { cancel_delayed_work_sync(&intel_dp->panel_vdd_work); + /* + * vdd might still be enabled do to the delayed vdd off. + * Make sure vdd is actually turned off here. + */ mutex_lock(&dev_priv->pps_mutex); edp_panel_vdd_off_sync(intel_dp); mutex_unlock(&dev_priv->pps_mutex);