On Tue, 02 Oct 2018, José Roberto de Souza <[email protected]> wrote: > Moving all the error handling to the end of the function and ealier > returning for connected MST ports make this function way more easy to > read. > No functional changed is intended here.
Honestly, I disagree that the end result would be more clear to read. I'm all for improving clarity, but there does have to be a clear improvement to justify changing this. I'm especially suspicious of duplicating the intel_display_power_put() calls and the returns. It sets a precedent to duplicate them again, and someone's bound to forget to put. BR, Jani, > > Cc: Jani Nikula <[email protected]> > Cc: Ville Syrjälä <[email protected]> > Cc: Dhinakaran Pandiyan <[email protected]> > Signed-off-by: José Roberto de Souza <[email protected]> > --- > drivers/gpu/drm/i915/intel_dp.c | 55 +++++++++++++++++++-------------- > 1 file changed, 31 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 15a981ef5966..5a84a929bc7d 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -5077,28 +5077,20 @@ intel_dp_detect(struct drm_connector *connector, > > intel_display_power_get(dev_priv, intel_dp->aux_power_domain); > > - /* Can't disconnect eDP */ > + /* Is port connected? eDP can't be disconnected */ > + if (!intel_dp_is_edp(intel_dp) && > + !intel_digital_port_connected(encoder)) { > + status = connector_status_disconnected; > + goto port_disconnected; > + } > + > if (intel_dp_is_edp(intel_dp)) > status = edp_detect(intel_dp); > - else if (intel_digital_port_connected(encoder)) > - status = intel_dp_detect_dpcd(intel_dp); > else > - status = connector_status_disconnected; > - > - if (status == connector_status_disconnected) { > - memset(&intel_dp->compliance, 0, sizeof(intel_dp->compliance)); > - > - if (intel_dp->is_mst) { > - DRM_DEBUG_KMS("MST device may have disappeared %d vs > %d\n", > - intel_dp->is_mst, > - intel_dp->mst_mgr.mst_state); > - intel_dp->is_mst = false; > - drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, > - intel_dp->is_mst); > - } > + status = intel_dp_detect_dpcd(intel_dp); > > - goto out; > - } > + if (status == connector_status_disconnected) > + goto port_not_detected; > > if (intel_dp->reset_link_params) { > /* Initial max link lane count */ > @@ -5123,8 +5115,8 @@ intel_dp_detect(struct drm_connector *connector, > * won't appear connected or have anything > * with EDID on it > */ > - status = connector_status_disconnected; > - goto out; > + intel_display_power_put(dev_priv, intel_dp->aux_power_domain); > + return connector_status_disconnected; > } > > /* > @@ -5151,14 +5143,29 @@ intel_dp_detect(struct drm_connector *connector, > intel_dp->aux.i2c_defer_count = 0; > > intel_dp_set_edid(intel_dp); > - if (intel_dp_is_edp(intel_dp) || > - to_intel_connector(connector)->detect_edid) > + /* > + * intel_dp_detect_dpcd() will return connector_status_unknown for some > + * type of DP devices, if edid can be read set status as connected > + */ > + if (to_intel_connector(connector)->detect_edid) > status = connector_status_connected; > > intel_dp_check_service_irq(intel_dp); > > -out: > - if (status != connector_status_connected && !intel_dp->is_mst) > + intel_display_power_put(dev_priv, intel_dp->aux_power_domain); > + return status; > + > +port_not_detected: > +port_disconnected: > + memset(&intel_dp->compliance, 0, sizeof(intel_dp->compliance)); > + > + if (intel_dp->is_mst) { > + DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", > + intel_dp->is_mst, intel_dp->mst_mgr.mst_state); > + intel_dp->is_mst = false; > + drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, > + intel_dp->is_mst); > + } else > intel_dp_unset_edid(intel_dp); > > intel_display_power_put(dev_priv, intel_dp->aux_power_domain); -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
