Re: [PATCH v2] stop setup_dig_out_stream() causing clicks

[Laurence Darby <ldarby@tuffmail.com>]

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@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?

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