| Message ID | 20240322121832.4170061-2-ankit.k.nautiyal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Avoid unwanted lspcon init and probe warnings | expand |
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;
On 3/25/2024 8:47 PM, Jani Nikula wrote: > 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. 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;
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; + 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;
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(-)