[alsa-devel] [Alsa-user] Jetway j7f2 via82xx volume problem: sound suddenly stops when increasing volume: "SOLVED" (well ... sort of)

[Zoilo Gomez]

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.