Re: intel8x0 dual codec issue

[Takashi Iwai <tiwai@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 2138 bytes --]

At Thu, 10 Feb 2005 14:16:12 -0500,
Ron Cococcia wrote:
> 
> I had been looking through that block of code, and had made some of my 
> own "tweaks".  What I did was make "reg_ok" an array (size 4, all 
> initialized to 0).  I used the codec index (cidx) to index into that 
> array.  The idea is that there would be a reg_ok for each codec, instead 
> of just 1 for all codecs.  This correctly set the output on all codecs 
> after making the change.

Yeah, that's better than my last hack.  Could you create a patch?

> Unfortunately, I was still noticing some audio distortions on the 
> secondary codec.  Originally, some tones would sound "tinny".  After the 
> change, the tinny sound went away, but I could hear distinct "clicks" in 
> the audio.  What causes this is unknown to me, but I thought it might be 
> some kind of problem where both codecs are producing some information 
> (maybe interrupts) and causing some weirdness with loading the data.  If 
> I knew more about what was going on I would have looked into this more, 
> but I am still just an novice driver "tweaker".
> 
> On a hunch, I figured that if the primary codec was doing something 
> weird, I might need to make it "forget" about the primary codec.  I went 
> into intel8x0.c, and started to take a look, and was able to add a quick 
> test hack.  If the number of codecs detected in snd_intel8x0_mixer was 
> 2, set it to 1.  In snd_intel8x0_codec_{read/write/read_test}, it would 
> add 1 to ac97->num, so it would access the secondary codecs values and 
> skip the primary.
> 
> After compiling and installing, alsamixer only knows of 1 codec 
> (CS4299), and the audio playback appears to be greatly improved with no 
> clicks or tinny sounds.
> 
> For now, I will be leaving my hack in, unless there is a better fix 
> available.  A few hours after getting it working, I managed to hose my 
> development setup anyway, so it will be a little while before I can play 
> around again.

Given that overriding codecs with NCR becomes popular, it might be
nice to have a module option to specify the codecs as bitmask.
The below is a quick hack.


Takashi

[-- Attachment #2: Type: text/plain, Size: 2971 bytes --]

Index: alsa-kernel/pci/intel8x0.c
===================================================================
RCS file: /suse/tiwai/cvs/alsa/alsa-kernel/pci/intel8x0.c,v
retrieving revision 1.190
diff -u -r1.190 intel8x0.c
--- alsa-kernel/pci/intel8x0.c	4 Feb 2005 16:59:46 -0000	1.190
+++ alsa-kernel/pci/intel8x0.c	11 Feb 2005 10:16:11 -0000
@@ -70,6 +70,7 @@
 static char *ac97_quirk[SNDRV_CARDS];
 static int buggy_irq[SNDRV_CARDS];
 static int xbox[SNDRV_CARDS];
+static int codec_mask[SNDRV_CARDS];
 
 #ifdef SUPPORT_MIDI
 static int mpu_port[SNDRV_CARDS]; /* disabled */
@@ -424,6 +425,8 @@
 	ac97_bus_t *ac97_bus;
 	ac97_t *ac97[3];
 	unsigned int ac97_sdin[3];
+	int first_codec;
+	unsigned int codec_mask;
 
 	spinlock_t reg_lock;
 	
@@ -2023,8 +2026,11 @@
 	pbus->dra = 1;
 	chip->ac97_bus = pbus;
 
+	chip->first_codec = -1;
 	ac97.pci = chip->pci;
 	for (i = 0; i < codecs; i++) {
+		if (! (chip->codec_mask & (1 << i)))
+			continue;
 		ac97.num = i;
 		if ((err = snd_ac97_mixer(pbus, &ac97, &chip->ac97[i])) < 0) {
 			if (err != -EACCES)
@@ -2033,9 +2039,14 @@
 				goto __err;
 			continue;
 		}
+		if (chip->first_codec < 0)
+			chip->first_codec = i;
 	}
+	if (chip->first_codec < 0)
+		goto __err;
 	/* tune up the primary codec */
-	snd_ac97_tune_hardware(chip->ac97[0], ac97_quirks, quirk_override);
+	/* FIXME: other codecs, too? */
+	snd_ac97_tune_hardware(chip->ac97[chip->first_codec], ac97_quirks, quirk_override);
 	/* enable separate SDINs for ICH4 */
 	if (chip->device_type == DEVICE_INTEL_ICH4)
 		pbus->isdin = 1;
@@ -2098,7 +2109,8 @@
 		val = igetdword(chip, ICHREG(GLOB_CNT)) & ~ICH_PCM_SPDIF_MASK;
 		val |= ICH_PCM_SPDIF_1011;
 		iputdword(chip, ICHREG(GLOB_CNT), val);
-		snd_ac97_update_bits(chip->ac97[0], AC97_EXTENDED_STATUS, 0x03 << 4, 0x03 << 4);
+		/* FIXME: assume the first codec supports SPDIF */
+		snd_ac97_update_bits(chip->ac97[chip->first_codec], AC97_EXTENDED_STATUS, 0x03 << 4, 0x03 << 4);
 	}
 	chip->in_ac97_init = 0;
 	return 0;
@@ -2410,7 +2422,7 @@
 	ichdev->substream = NULL; /* don't process interrupts */
 
 	/* set rate */
-	if (snd_ac97_set_rate(chip->ac97[0], AC97_PCM_FRONT_DAC_RATE, 48000) < 0) {
+	if (snd_ac97_set_rate(chip->ac97[chip->first_codec], AC97_PCM_FRONT_DAC_RATE, 48000) < 0) {
 		snd_printk(KERN_ERR "cannot set ac97 rate: clock = %d\n", chip->ac97_bus->clock);
 		return;
 	}
@@ -2786,6 +2798,10 @@
 	if (xbox[dev])
 		chip->xbox = 1;
 
+	if (codec_mask[dev])
+		chip->codec_mask = codec_mask[dev];
+	else
+		chip->codec_mask = -1;
 	if ((err = snd_intel8x0_mixer(chip, ac97_clock[dev], ac97_quirk[dev])) < 0) {
 		snd_card_free(card);
 		return err;
@@ -2799,7 +2815,7 @@
 
 	snprintf(card->longname, sizeof(card->longname),
 		 "%s with %s at %#lx, irq %i", card->shortname,
-		 snd_ac97_get_short_name(chip->ac97[0]), chip->addr, chip->irq);
+		 snd_ac97_get_short_name(chip->ac97[chip->first_codec]), chip->addr, chip->irq);
 
 	if (! ac97_clock[dev])
 		intel8x0_measure_ac97_clock(chip);