At Fri, 02 May 2008 19:12:57 +0200,
Zoilo Gomez wrote:
>
> Takashi Iwai wrote:
> > 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?
> >
>
> Yes it is; if I do not write to 0x5c after reset, then the register
> value reads back as 0x20.
>
> >> 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?
> >
>
> OK, now I get your point: you are assuming that the chip set on my
> machine can be recognized because it has bit 0x20 set after reset, and
> in that case we leave the value as is. However if bit 0x20 is not set,
> then we assume that it is a different chip set with "proper
> 1/0-set/clear-behaviour", and we set the bit to 0x20. Correct?
Yes, exactly.
> Sorry for the confusion on my side.
>
> > 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
> >
>
> Understood.
>
> OK, I confirm that your patch works fine on my machine, which (for the
> record) is a Jetway J7F2 Mini-ITX motherboard.
Thanks for checking. I applied the patch (with a bit more comment) to
ALSA tree now. It'll be on 2.6.26 tree, too.
> I have also cross-checked (well ... "cross-grepped") with the
> viaudiocombo OSS-driver from the Arena web site; it seems that the
> contents of register 0x5c are left unaltered after reset (hence the
> value is 0x20). So that explains why the Arena driver does not suffer
> from this problem.
The workaround was relatively new and I guess it's specific to some
EPIA board and possibly not included in other drivers...
thanks,
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
