[Intel-gfx] [PATCH 1/3] drm/i915: Clean up hsw_audio_codec_enable()

[Chris Wilson]

On Thu, Aug 11, 2016 at 10:47:06AM +0300, Jani Nikula wrote:
> On Thu, 11 Aug 2016, Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com> wrote:
> > No functional change, code clean up and improved debug.
> >
> > Chris suggested this code snippet while reviewing, I just made this into a
> > patch.
> >
> > Credits-to: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_audio.c | 28 ++++++++++++----------------
> >  1 file changed, 12 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > index d32f586..3efce0e 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -282,14 +282,9 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
> >  	struct drm_i915_private *dev_priv = to_i915(connector->dev);
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> >  	enum pipe pipe = intel_crtc->pipe;
> > -	struct i915_audio_component *acomp = dev_priv->audio_component;
> >  	const uint8_t *eld = connector->eld;
> > -	struct intel_digital_port *intel_dig_port =
> > -		enc_to_dig_port(&encoder->base);
> > -	enum port port = intel_dig_port->port;
> >  	uint32_t tmp;
> >  	int len, i;
> > -	int n, rate;
> >  
> >  	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes ELD\n",
> >  		      pipe_name(pipe), drm_eld_size(eld));
> > @@ -335,19 +330,20 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
> >  
> >  	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> >  	if (audio_rate_need_prog(intel_crtc, adjusted_mode)) {
> > -		if (!acomp)
> > -			rate = 0;
> > -		else if (port >= PORT_A && port <= PORT_E)
> > +		enum port port = enc_to_dig_port(&encoder->base)->port;
> > +		struct i915_audio_component *acomp = dev_priv->audio_component;
> > +		int rate, n;
> 
> Moving these here is probably fine, although originally I was hoping we
> would always program the N, and thus wouldn't have this if
> (audio_rate_need_prog(intel_crtc, adjusted_mode)) block here at all. The
> idea was that we'd always use the same method instead of letting the hw
> set it automatically for some rates, and manually setting others.
> 
> > +
> > +		rate = 0;
> > +		if (acomp && port >= PORT_A && port <= PORT_E) {
> >  			rate = acomp->aud_sample_rate[port];
> > -		else {
> > -			DRM_ERROR("invalid port: %d\n", port);
> 
> I think I may have seen these in some dmesgs in otherwise unrelated
> bugs. I'd like to preserve this at least as a debug breadcrumb.
> 
> > -			rate = 0;
> > +
> > +			n = audio_config_get_n(adjusted_mode, rate);
> > +			DRM_DEBUG_KMS("port %d audio rate %d => N=%x\n",
> > +				      port, rate, n);
> > +			if (n)
> > +				tmp = audio_config_setup_n_reg(n, tmp);
> >  		}
> > -		n = audio_config_get_n(adjusted_mode, rate);
> > -		if (n != 0)
> > -			tmp = audio_config_setup_n_reg(n, tmp);
> > -		else
> > -			DRM_DEBUG_KMS("no suitable N value is found\n");
> 
> Same here, I'd like to preserve the debug.

The information is still there (both invalid port and unfound n), just as
a single debug.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre