Re: [PATCH 3/4] drm/i915/display: Remove useless call intel_dp_mst_encoder_cleanup()

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

On Thu, 2020-01-16 at 17:58 -0800, José Roberto de Souza wrote:
> This is a eDP function and it will always returns true for non-eDP
> ports.
> 
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 4074d83b1a5f..a50b5b6dd009 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -7537,7 +7537,6 @@ intel_dp_init_connector(struct
> intel_digital_port *intel_dig_port,
>  
>  	if (!intel_edp_init_connector(intel_dp, intel_connector)) {
>  		intel_dp_aux_fini(intel_dp);
> -		intel_dp_mst_encoder_cleanup(intel_dig_port);
>  		goto fail;
>  	}
>  


Change looks fine for me(anyway better than now). 

But:

This whole thing looks kind of confusing to me. Why we are even calling
intel_edp_init_connector for
non-eDP ports, just to immediately get true returned? So returning
success means either success or that this is non-eDP..

This confuses the caller, that we have actually successfully
initialized eDP, while actually this also means here that it is not
eDP.

Why we can't just do it like:

if (intel_dp_is_edp(intel_dp)) {
	if (!intel_edp_init_connector(intel_dp, intel_connector)) {
		intel_dp_aux_fini(intel_dp);
  		goto fail;
	}
}

it looks much more understandable and less confusing, i.e eDP functions
are only called for eDP and no return value hacks are needed.

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

Stan

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel