At Wed, 30 Apr 2008 18:49:52 +0200,
Zoilo Gomez wrote:
>
> Takashi Iwai wrote:
> > At Tue, 29 Apr 2008 15:25:57 +0200,
> > Zoilo Gomez wrote:
> >
> >>>> This is consistent with the apparent introduction of this bug in kernel
> >>>> 2.6.19 (includes alsa-driver version 1.0.12rc1): no such problem
> >>>> occurred until 2.6.18, but all kernels since 2.6.19 do suffer from this
> >>>> problem. The line of code above first shows up in linux-2.6.19.
> >>>>
> >>>> Unfortunately, since I do not have a datasheet for the VT1617A chip set,
> >>>> I cannot verify the exact semantics, or suggest an improvement.
> >>>>
> >>>> Can anyone with a datasheet please suggest a proper patch to this line
> >>>> of code?
> >>>>
> >>>>
> >>> The register 0x5c is the VIA specific one. I have a VT1617 (without
> >>> A) datasheet, and it suggests that the bit corresponds to the
> >>> "headphone amplifier temperature sensing control". And setting this
> >>> bit means to _disable_ the temperature sensing control. This sounds
> >>> rather the correct to set.
> >>>
> >>> However, I don't know whether any difference exists betweeen VIA1617
> >>> and 1617A although the codec id of both are identical.
> >>>
> >>> Andrey, any comments about your patch?
> >>>
> >>>
> >>> In anyway, it'd be helpful if we can know which ac97 registers work
> >>> and wich not. Please take /proc/asound/card0/codec97#*/ac97#*-regs
> >>> file in both working and non-working cases to compare. Especially,
> >>> the registers 0x5a and 0x5c look interesting.
> >>>
> >>>
> >> Code containing "snd_ac97_write_cache(a97, 0x5c, 0x20)":
> >> 0x5a = 8300
> >> 0x5c = 0000
> >>
> >> Code with "snd_ac97_write_cache(a97, 0x5c, 0x20)" commented out:
> >> 0x5a = 8301
> >> 0x5c = 0020
> >>
> >> Quite a surprise to me, I would have expected exactly the opposite .....!?
> >>
> >
> > I would, too. Could you double-check, e.g. by adding a printk?
> >
>
> I modified the driver code to:
>
> > int patch_vt1617a(struct snd_ac97 * ac97)
> > {
> > int err = 0;
> >
> > /* we choose to not fail out at this point, but we tell the
> > caller when we return */
> >
> > err = patch_build_controls(ac97, &snd_ac97_controls_vt1617a[0],
> > ARRAY_SIZE(snd_ac97_controls_vt1617a));
> >
> > /* bring analog power consumption to normal by turning off the
> > * headphone amplifier, like WinXP driver for EPIA SP
> > */
> > printk("before: snd_ac97_read(ac97, 0x5c) = %04x\n",
> > snd_ac97_read(ac97, 0x5c));
> > snd_ac97_write_cache(ac97, 0x5c, 0x20);
> > printk("after: snd_ac97_read(ac97, 0x5c) = %04x\n",
> > snd_ac97_read(ac97, 0x5c));
> > ac97->ext_id |= AC97_EI_SPDIF; /* force the detection of spdif */
> > ac97->rates[AC97_RATES_SPDIF] = SNDRV_PCM_RATE_44100 |
> > SNDRV_PCM_RATE_48000;
> > ac97->build_ops = &patch_vt1616_ops;
> >
> > return err;
> > }
>
> Surprise surprise ... the printk output in dmesg reads:
>
> > before: snd_ac97_read(ac97, 0x5c) = 0020
> > after: snd_ac97_read(ac97, 0x5c) = 0000
>
> If I write 0x00 (i.o. 0x20) into 0x5c, then the register content remains
> unchanged, and returns: 0x20.
> If I write 0x20 into 0x5c twice, then the register content is also 0x00,
> so no toggling.
>
> So it seems that the bit-value is inverted: write "1" clears the bit,
> whereas writing "0" sets the bit.
Interesting.
> And the code is trying to set the bit (with good intentions), but the
> result is in fact the opposite.
>
> What do you think?
This sounds like a hardware bug to me...
How about the patch below then?
Takashi
---
diff --git a/pci/ac97/ac97_patch.c b/pci/ac97/ac97_patch.c
index 726320b..e3e4dac 100644
--- a/pci/ac97/ac97_patch.c
+++ b/pci/ac97/ac97_patch.c
@@ -3448,6 +3448,7 @@ static const struct snd_kcontrol_new snd_ac97_controls_vt1617a[] = {
int patch_vt1617a(struct snd_ac97 * ac97)
{
int err = 0;
+ int val;
/* we choose to not fail out at this point, but we tell the
caller when we return */
@@ -3458,7 +3459,10 @@ int patch_vt1617a(struct snd_ac97 * ac97)
/* bring analog power consumption to normal by turning off the
* headphone amplifier, like WinXP driver for EPIA SP
*/
- snd_ac97_write_cache(ac97, 0x5c, 0x20);
+ val = snd_ac97_read(ac97, 0x5c);
+ if (!(val & 0x20))
+ snd_ac97_write_cache(ac97, 0x5c, 0x20);
+
ac97->ext_id |= AC97_EI_SPDIF; /* force the detection of spdif */
ac97->rates[AC97_RATES_SPDIF] = SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000;
ac97->build_ops = &patch_vt1616_ops;
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
