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; I think
you mean:
> + if (!(val & 0x20))
> + snd_ac97_write_cache(ac97, 0x5c, 0x00);
>
But why so complicated? Why not omit the test, and write unconditionally:
> + snd_ac97_write_cache(ac97, 0x5c, 0x00);
Or perhaps even leave bit 5 in the reset state, which is 0x20 hence OK.
In other words: drop the entire line?
Z.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
