At Fri, 02 May 2008 17:15:13 +0200,
Zoilo Gomez wrote:
>
> Takashi Iwai wrote:
> > 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...
> >
>
> Agreed.
>
> > 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);
> >
>
> This has no effect, since set/clear is apparently reversed here;
So, the bit 0x20 isn't set as default when you didn't write to 0x5c?
> I think you mean:
>
> > + if (!(val & 0x20))
> > + snd_ac97_write_cache(ac97, 0x5c, 0x00);
> >
No I meant as is. See below.
> But why so complicated? Why not omit the test, and write unconditionally:
> > + snd_ac97_write_cache(ac97, 0x5c, 0x00);
>
The patch was there for any good reason. It was introduced to fix
some bugs on certain machines. Removing it blindly breaks it again.
> Or perhaps even leave bit 5 in the reset state, which is 0x20 hence OK.
Hm, they why the first test didn't pass?
> In other words: drop the entire line?
No. The situation is really complicated:
- the current code is confirmed to work on some machines as is
- we don't know exactly whether the bit flip oocurs on every hardware
with VT1617A (maybe not)
- we don't know exactly whether the bit is set or cleared as default
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
