From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [PATCH 15/14] drm/i915: Add comments explaining the vdd on/off functions Date: Wed, 03 Sep 2014 14:52:41 +0300 Message-ID: <1409745161.15662.35.camel@intelbox> References: <87fvgsha80.fsf@intel.com> <1408470437-12107-1-git-send-email-ville.syrjala@linux.intel.com> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1203991584==" Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id CED7B89286 for ; Wed, 3 Sep 2014 04:52:45 -0700 (PDT) In-Reply-To: <1408470437-12107-1-git-send-email-ville.syrjala@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: ville.syrjala@linux.intel.com Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org --===============1203991584== Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-4RS5PKeDH5X/jnkCK7Yt" --=-4RS5PKeDH5X/jnkCK7Yt Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 2014-08-19 at 20:47 +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrj=C3=A4l=C3=A4 >=20 > Jani wanted some comments to explain why we call certain vdd on/off > functions in certain places. >=20 > Signed-off-by: Ville Syrj=C3=A4l=C3=A4 > --- > drivers/gpu/drm/i915/intel_dp.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) >=20 > 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; > } > =20 > +/* 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 =3D 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; > } > =20 > +/* 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 =3D > @@ -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); > } > =20 > +/* 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 =3D > @@ -1467,6 +1470,7 @@ static void edp_panel_vdd_off(struct intel_dp *inte= l_dp, bool sync) > edp_panel_vdd_schedule_off(intel_dp); > } > =20 > +/* 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 =3D > @@ -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; > =20 > + /* > + * 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 --=-4RS5PKeDH5X/jnkCK7Yt Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.14 (GNU/Linux) iQEcBAABAgAGBQJUBwEKAAoJEORIIAnNuWDFsuEH/ApY/+IKxniOQCgrktVr1r52 CYsgsoAelZXtDmeNkZIUTW/rBM+DRPpXrb6vo6LOOkYz2ybGIiSbiBmqE69hojvL 7ul72TFg+W7Xz9aEzmZtXj7Mkbrzjm2pZA79gVfXJZLRlf6U0ojqfCR/tkyuPa6A Tmddb5mR44BR8tN3vmNYZDASGUeG6L10MCL0pnDb0BohhvrCWtrUhwISjqu7JlEI G5aC7oaCxtMYSm952sNGLzWcDxDxa8+aHcWY0C0d5ypygxHUlzT5417BavzDKt+L 7PsadUeXW9Jq4OvNfSo5cqdJulrGaOXD6NByRvA5v2FgRefPra2O5ynf0xNLKoc= =gbhG -----END PGP SIGNATURE----- --=-4RS5PKeDH5X/jnkCK7Yt-- --===============1203991584== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============1203991584==--