On Fri, 22 Mar 2024, Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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
