| Message ID | 20180418224311.16577-8-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2018-04-18 at 15:43 -0700, José Roberto de Souza wrote: > - Doing earlier return when not busy > - using u32 instead of uint32_t > - counting from 3 to 0 as it is is the most common in the driver Hmm. I see more instances that increment the loop variable than the opposite. > - using DRM_WARN() instead of WARN() Why? WARN and WARN_ON's are common afaict > - adding aux port name to the debug message Sounds like a good idea. > - nuking last_status, it is one static variable to all DP ports > also 2 different aux transactions with the same message size would > have the same ch_ctl value, if really desired to reduce the number > of debug messages it should be implemented a per aux ch last_status Makes sense to do this. > - no need to sleep in the last try > - sleeping for 1 aux ch transaction time Was this determined based on the clock speed or is it in the spec? I see numbers for timeout, nothing for the transaction itself. This patch mixes up functional changes with non-functional ones. I'd prefer you split these changes. > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > > New patch in this series. > > drivers/gpu/drm/i915/intel_dp.c | 23 ++++++++--------------- > 1 file changed, 8 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > b/drivers/gpu/drm/i915/intel_dp.c > index a11465c62950..258e23961456 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1068,28 +1068,21 @@ static bool intel_dp_aux_is_busy(struct > intel_dp *intel_dp) > struct drm_i915_private *dev_priv = > to_i915(intel_dig_port->base.base.dev); > i915_reg_t ch_ctl; > - uint32_t status; > - int try; > + u32 status; > + unsigned int try; > > ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp); > > - for (try = 0; try < 3; try++) { > + for (try = 3; try; try--) { > status = I915_READ_NOTRACE(ch_ctl); > if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0) > - break; > - msleep(1); > + return false; > + if (try > 1) > + usleep_range(400, 500); > } > > - if (try == 3) { > - static u32 last_status = -1; > - const u32 status = I915_READ(ch_ctl); > - > - if (status != last_status) { > - WARN(1, "dp_aux_ch not started status > 0x%08x\n", > - status); > - last_status = status; > - } > - } > + DRM_WARN("DP aux %c is busy 0x%08x\n", > + aux_ch_name(intel_dp->aux_ch), status); > > return true; > }
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index a11465c62950..258e23961456 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1068,28 +1068,21 @@ static bool intel_dp_aux_is_busy(struct intel_dp *intel_dp) struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev); i915_reg_t ch_ctl; - uint32_t status; - int try; + u32 status; + unsigned int try; ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp); - for (try = 0; try < 3; try++) { + for (try = 3; try; try--) { status = I915_READ_NOTRACE(ch_ctl); if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0) - break; - msleep(1); + return false; + if (try > 1) + usleep_range(400, 500); } - if (try == 3) { - static u32 last_status = -1; - const u32 status = I915_READ(ch_ctl); - - if (status != last_status) { - WARN(1, "dp_aux_ch not started status 0x%08x\n", - status); - last_status = status; - } - } + DRM_WARN("DP aux %c is busy 0x%08x\n", + aux_ch_name(intel_dp->aux_ch), status); return true; }
- Doing earlier return when not busy - using u32 instead of uint32_t - counting from 3 to 0 as it is is the most common in the driver - using DRM_WARN() instead of WARN() - adding aux port name to the debug message - nuking last_status, it is one static variable to all DP ports also 2 different aux transactions with the same message size would have the same ch_ctl value, if really desired to reduce the number of debug messages it should be implemented a per aux ch last_status - no need to sleep in the last try - sleeping for 1 aux ch transaction time Signed-off-by: José Roberto de Souza <jose.souza@intel.com> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- New patch in this series. drivers/gpu/drm/i915/intel_dp.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-)