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

[Nautiyal, Ankit K]

On 3/25/2024 8:47 PM, Jani Nikula wrote:
> On Fri, 22 Mar 2024, Ankit Nautiyal <ankit.k.nautiyal at 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 at 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.

You are right I indeed missed this part. The lspcon_wake_native_aux_ch 
was there for a reason, which I completely overlooked.

Thanks for pointing this out. Will take care of this in next version.

Thanks & Regards,

Ankit

>
> 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;