Re: [PATCH 1/2] drm/i915/lspcon: Separate function to set expected mode

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 22 Mar 2024, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote:
> LSPCON can be configured to LS or PCON mode.
> Separate the function to set the expected mode from the lspcon probe
> function during lspcon init.
>
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_lspcon.c | 47 ++++++++++++++-------
>  1 file changed, 31 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_lspcon.c b/drivers/gpu/drm/i915/display/intel_lspcon.c
> index 1d048fa98561..62159d3ead56 100644
> --- a/drivers/gpu/drm/i915/display/intel_lspcon.c
> +++ b/drivers/gpu/drm/i915/display/intel_lspcon.c
> @@ -240,18 +240,40 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon)
>  	return true;
>  }
>  
> -static bool lspcon_probe(struct intel_lspcon *lspcon)
> +static bool lspcon_set_expected_mode(struct intel_lspcon *lspcon)
>  {
> -	int retry;
> -	enum drm_dp_dual_mode_type adaptor_type;
>  	struct intel_dp *intel_dp = lspcon_to_intel_dp(lspcon);
>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> -	struct i2c_adapter *ddc = &intel_dp->aux.ddc;
>  	enum drm_lspcon_mode expected_mode;
>  
>  	expected_mode = lspcon_wake_native_aux_ch(lspcon) ?
>  			DRM_LSPCON_MODE_PCON : DRM_LSPCON_MODE_LS;

You always need to consider the functional changes when aiming to make
non-functional refactoring. This postpones lspcon_wake_native_aux_ch()
until after the probe. But the name has "wake" in it, and you're no
longer waking up as the first thing. Smells fishy.

Git blame leads to f2b667b658f9 ("drm/i915/lspcon: Ensure AUX CH is
awake while in DP Sleep state"). You should read the commit message.

BR,
Jani.


>  
> +	lspcon->mode = lspcon_wait_mode(lspcon, expected_mode);
> +
> +	/*
> +	 * In the SW state machine, lets Put LSPCON in PCON mode only.
> +	 * In this way, it will work with both HDMI 1.4 sinks as well as HDMI
> +	 * 2.0 sinks.
> +	 */
> +	if (lspcon->mode != DRM_LSPCON_MODE_PCON) {
> +		if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON) < 0) {
> +			drm_err(&i915->drm, "LSPCON mode change to PCON failed\n");
> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
> +static bool lspcon_probe(struct intel_lspcon *lspcon)
> +{
> +	int retry;
> +	enum drm_dp_dual_mode_type adaptor_type;
> +	struct intel_dp *intel_dp = lspcon_to_intel_dp(lspcon);
> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> +	struct i2c_adapter *ddc = &intel_dp->aux.ddc;
> +
>  	/* Lets probe the adaptor and check its type */
>  	for (retry = 0; retry < 6; retry++) {
>  		if (retry)
> @@ -270,19 +292,7 @@ static bool lspcon_probe(struct intel_lspcon *lspcon)
>  
>  	/* Yay ... got a LSPCON device */
>  	drm_dbg_kms(&i915->drm, "LSPCON detected\n");
> -	lspcon->mode = lspcon_wait_mode(lspcon, expected_mode);
>  
> -	/*
> -	 * In the SW state machine, lets Put LSPCON in PCON mode only.
> -	 * In this way, it will work with both HDMI 1.4 sinks as well as HDMI
> -	 * 2.0 sinks.
> -	 */
> -	if (lspcon->mode != DRM_LSPCON_MODE_PCON) {
> -		if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON) < 0) {
> -			drm_err(&i915->drm, "LSPCON mode change to PCON failed\n");
> -			return false;
> -		}
> -	}
>  	return true;
>  }
>  
> @@ -671,6 +681,11 @@ bool lspcon_init(struct intel_digital_port *dig_port)
>  		return false;
>  	}
>  
> +	if (!lspcon_set_expected_mode(lspcon)) {
> +		drm_err(&i915->drm, "LSPCON Set expected Mode failed\n");
> +		return false;
> +	}
> +
>  	if (drm_dp_read_dpcd_caps(&intel_dp->aux, intel_dp->dpcd) != 0) {
>  		drm_err(&i915->drm, "LSPCON DPCD read failed\n");
>  		return false;

-- 
Jani Nikula, Intel