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

[Takashi Iwai <tiwai@suse.de>]

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