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

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