[alsa-devel] [PATCH v3] stop setup_dig_out_stream() causing clicks

[Takashi Iwai]

At Sat, 3 Nov 2012 17:00:06 +0000,
Laurence Darby wrote:
> 
> Starting audio or seeking in various music players causes
> setup_dig_out_stream() to be called, which resets the SPDIF stream,
> which caused one DAC (but not another) to make a clicking noise every
> time.
> 
> This patch ensures the reset only happens when it needs to, which is
> when the format changes, and makes the code a little more readable.
> 
> Signed-off-by: Laurence Darby <ldarby at tuffmail.com>
> ---
> 
> Takashi Iwai wrote:
> 
> > > +	reset = codec->spdif_status_reset &&
> > > +		spdif->ctls & AC_DIG1_ENABLE &&
> > > +		curr_fmt != format;
> > 
> > Better to put parentheses around the bit operations when combined with
> > logical operations.  gcc would suggest it, too, I thought.
> > 
> 
> gcc doesn't warn about it, but it does on code like this:
> 
> 	reset = codec->spdif_status_reset &&
> 		spdif->ctls & AC_DIG1_ENABLE + 0 &&
> 		curr_fmt != format;
> 
> which suggests putting them around the +. But since you spotted it, I've put them back in.

Thanks, applied now.


Takashi

> 
> Regards,
> Laurence
> 
> 
>  sound/pci/hda/hda_codec.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 38fdefc..2b21906 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -4718,10 +4718,20 @@ EXPORT_SYMBOL_HDA(snd_hda_input_mux_put);
>  static void setup_dig_out_stream(struct hda_codec *codec, hda_nid_t nid,
>  				 unsigned int stream_tag, unsigned int format)
>  {
> -	struct hda_spdif_out *spdif = snd_hda_spdif_out_of_nid(codec, nid);
> -
> -	/* turn off SPDIF once; otherwise the IEC958 bits won't be updated */
> -	if (codec->spdif_status_reset && (spdif->ctls & AC_DIG1_ENABLE))
> +	struct hda_spdif_out *spdif;
> +	unsigned int curr_fmt;
> +	bool reset;
> +
> +	spdif = snd_hda_spdif_out_of_nid(codec, nid);
> +	curr_fmt = snd_hda_codec_read(codec, nid, 0,
> +				      AC_VERB_GET_STREAM_FORMAT, 0);
> +	reset = codec->spdif_status_reset &&
> +		(spdif->ctls & AC_DIG1_ENABLE) &&
> +		curr_fmt != format;
> +	
> +	/* turn off SPDIF if needed; otherwise the IEC958 bits won't be
> +	   updated */
> +	if (reset)
>  		set_dig_out_convert(codec, nid,
>  				    spdif->ctls & ~AC_DIG1_ENABLE & 0xff,
>  				    -1);
> @@ -4733,7 +4743,7 @@ static void setup_dig_out_stream(struct hda_codec *codec, hda_nid_t nid,
>  						   format);
>  	}
>  	/* turn on again (if needed) */
> -	if (codec->spdif_status_reset && (spdif->ctls & AC_DIG1_ENABLE))
> +	if (reset)
>  		set_dig_out_convert(codec, nid,
>  				    spdif->ctls & 0xff, -1);
>  }
> -- 
> 1.7.12.1
>