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

[Takashi Iwai]

At Thu, 1 Nov 2012 02:53:43 +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:
> 
> > At Thu, 4 Oct 2012 21:20:08 +0100,
> > 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 turns off codec->spdif_status_reset after one reset which
> > > stops further clicks.  One reset is still necessary to initialise
> > > the codec properly.
> > 
> > The flag isn't supposed to be changed in such a dynamic way.
> > Certain codecs seem requiring resetting the SPDIF status bits, and
> > this flag indicates that.
> > 
> > That is, blindly clearing this flag is risky (and likely broken).
> > 
> > One better option would be to do the SPDIF status reset only when the
> > value is really changed.
> > 
> 
> Hi Takashi,
> 
> Thanks, you are right, if another audio player used a different format,
> only noise was produced...  So v2 of the patch does the reset if the
> format changes, (so changing players still results in a click, but that
> seems to be unavoidable) and has some other changes which I think make
> the code a bit more readable.  Could you please review this?

Thanks, this looks much better now.
Just a nitpicking:

> 
> It's against the 3.6.4 stable branch (which is what I'm running), or
> does it need to be against Linus's tree?
> 
> Thanks,
> 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..6f7800a 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;

Better to put parentheses around the bit operations when combined with
logical operations.  gcc would suggest it, too, I thought.



Takashi

> +
> +	/* 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
> 
>