Re: [PATCH 1/3] drm/i915: Clean up hsw_audio_codec_enable()

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 11 Aug 2016, Dhinakaran Pandiyan <dhinakaran.pandiyan@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@chris-wilson.co.uk>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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.

>  	}
>  
>  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx