[PATCH] wss_lib: do not mess mixer settings during probe

[PATCH] wss_lib: do not mess mixer settings during probe

[Krzysztof Helt]

From: Krzysztof Helt <krzysztof.h1@wp.pl>

Use the wss_dout function which does not mess
shadowed register values during chip probing.
Otherwise, user ends up with stupid mixer settings
after driver loading.

Signed-off-by: Krzysztof Helt <krzysztof.h1@wp.pl>
---

Especially, the recording settings are messed up like
left channel recording from different source than
right channel.

diff -urp linux-ref/sound/isa/wss/wss_lib.c linux-2.6/sound/isa/wss/wss_lib.c
--- linux-ref/sound/isa/wss/wss_lib.c	2008-08-14 00:05:30.000000000 +0200
+++ linux-2.6/sound/isa/wss/wss_lib.c	2008-08-25 21:13:14.000000000 +0200
@@ -1162,9 +1162,9 @@ static int snd_ad1848_probe(struct snd_w
 	spin_lock_irqsave(&chip->reg_lock, flags);
 
 	/* set CS423x MODE 1 */
-	snd_wss_out(chip, CS4231_MISC_INFO, 0);
+	snd_wss_dout(chip, CS4231_MISC_INFO, 0);
 
-	snd_wss_out(chip, CS4231_RIGHT_INPUT, 0x45); /* 0x55 & ~0x10 */
+	snd_wss_dout(chip, CS4231_RIGHT_INPUT, 0x45); /* 0x55 & ~0x10 */
 	r = snd_wss_in(chip, CS4231_RIGHT_INPUT);
 	if (r != 0x45) {
 		/* RMGE always high on AD1847 */
@@ -1174,7 +1174,7 @@ static int snd_ad1848_probe(struct snd_w
 		}
 		hardware = WSS_HW_AD1847;
 	} else {
-		snd_wss_out(chip, CS4231_LEFT_INPUT,  0xaa);
+		snd_wss_dout(chip, CS4231_LEFT_INPUT,  0xaa);
 		r = snd_wss_in(chip, CS4231_LEFT_INPUT);
 		/* L/RMGE always low on AT2320 */
 		if ((r | CS4231_ENABLE_MIC_GAIN) != 0xaa) {
@@ -1199,7 +1199,7 @@ static int snd_ad1848_probe(struct snd_w
 	r = snd_wss_in(chip, CS4231_MISC_INFO);
 
 	/* set CS423x MODE 2 */
-	snd_wss_out(chip, CS4231_MISC_INFO, CS4231_MODE2);
+	snd_wss_dout(chip, CS4231_MISC_INFO, CS4231_MODE2);
 	for (i = 0; i < 16; i++) {
 		if (snd_wss_in(chip, i) != snd_wss_in(chip, 16 + i)) {
 			/* we have more than 16 registers: check ID */
@@ -1221,7 +1221,7 @@ static int snd_ad1848_probe(struct snd_w
 	else
 		chip->hardware = WSS_HW_AD1848;
 out_mode:
-	snd_wss_out(chip, CS4231_MISC_INFO, 0);
+	snd_wss_dout(chip, CS4231_MISC_INFO, 0);
 out:
 	spin_unlock_irqrestore(&chip->reg_lock, flags);
 	return err;




----------------------------------------------------------------------
Zobacz galerie - tak wygladaja wampiry!
>> link http://link.interia.pl/f1eee

Re: [PATCH] wss_lib: do not mess mixer settings during probe

[Rene Herman]

On 24-08-08 18:08, Krzysztof Helt wrote:

> From: Krzysztof Helt <krzysztof.h1@wp.pl>
> 
> Use the wss_dout function which does not mess
> shadowed register values during chip probing.
> Otherwise, user ends up with stupid mixer settings
> after driver loading.
> 
> Signed-off-by: Krzysztof Helt <krzysztof.h1@wp.pl>

Acked-by: Rene Herman <rene.herman@gmail.com>

Must say that I only now notice that we're doing that init loop there. I 
believe it could be better to introduce an inline __snd_wss_out() with 
just the two outb()s and use that from snd_wss_out() and here directly.

And with respect to the mb() ... if anywhere, should't that be between 
the two outb's really?

Rene.

Re: [PATCH] wss_lib: do not mess mixer settings during probe

[Krzysztof Helt]

On Sun, 24 Aug 2008 20:36:56 +0200
Rene Herman <rene.herman@keyaccess.nl> wrote:

> On 24-08-08 18:08, Krzysztof Helt wrote:
> 
> > From: Krzysztof Helt <krzysztof.h1@wp.pl>
> > 
> > Use the wss_dout function which does not mess
> > shadowed register values during chip probing.
> > Otherwise, user ends up with stupid mixer settings
> > after driver loading.
> > 
> > Signed-off-by: Krzysztof Helt <krzysztof.h1@wp.pl>
> 
> Acked-by: Rene Herman <rene.herman@gmail.com>
> 
> Must say that I only now notice that we're doing that init loop there. 

I noticed it on at least three different cards.

> I 
> believe it could be better to introduce an inline __snd_wss_out() with 
> just the two outb()s and use that from snd_wss_out() and here directly.
> 

snd_wss_dout() is almost the same. The waiting loop should never trigger
there and it is safer in case some write needs few micro-secs to finish.

> And with respect to the mb() ... if anywhere, should't that be between 
> the two outb's really?
> 

>From the Documentation/memory-barriers.txt
"
(*) inX(), outX():
...
    They are guaranteed to be fully ordered with respect to each other.

    They are not guaranteed to be fully ordered with respect to other types of
     memory and I/O operation.
...
"

No barriers are needed in this case. 

Regards,
Krzysztof

----------------------------------------------------------------------
Alergia na seks?
>> http://link.interia.pl/f1eec

Re: [PATCH] wss_lib: do not mess mixer settings during probe

[Rene Herman]

On 24-08-08 21:48, Krzysztof Helt wrote:

>From the Documentation/memory-barriers.txt
> "
> (*) inX(), outX():
> ...
>     They are guaranteed to be fully ordered with respect to each other.
> 
>     They are not guaranteed to be fully ordered with respect to other types of
>      memory and I/O operation.
> ...
> "
> 
> No barriers are needed in this case. 

On x86 certainly not and I would expect these barriers are there for 
cases/architectures where inb() and outb() are redefined to memory 
mapped I/O or there would just be no point at all.

In that case, the first outb() writes an index and we'd better make sure 
that it hits the hardware before we write the value. Hence my suspicion 
that if at all, the mb() should be in between.

No idea who added those and why...

Rene

Re: [PATCH] wss_lib: do not mess mixer settings during probe

[Rene Herman]

On 25-08-08 00:37, Rene Herman wrote:

> On 24-08-08 21:48, Krzysztof Helt wrote:
> 
>> From the Documentation/memory-barriers.txt
>> "
>> (*) inX(), outX():
>> ...
>>     They are guaranteed to be fully ordered with respect to each other.
>>
>>     They are not guaranteed to be fully ordered with respect to other 
>> types of
>>      memory and I/O operation.
>> ...
>> "
>>
>> No barriers are needed in this case. 
> 
> On x86 certainly not and I would expect these barriers are there for 
> cases/architectures where inb() and outb() are redefined to memory 
> mapped I/O or there would just be no point at all.
> 
> In that case, the first outb() writes an index and we'd better make sure 
> that it hits the hardware before we write the value. Hence my suspicion 
> that if at all, the mb() should be in between.
> 
> No idea who added those and why...

Well, thinking about this more, I guess it actually makes some sense.

No need for a wmb() in between because we know that the outbs are fully 
ordered (supposedly even when redefined) but being a general accessor 
function we pretend we don't know that the next access is going to be 
through an inb/outb again -- we are supposedly being careful to allow 
combining using this accessor with direct MMIO access on other and/or 
virtualized architectures.

That is... as far as I understand this in the first place. Takashi or 
Jaroslav, do you perhaps remember barriers being added to cs4231_lib() 
and why? Could we just get rid of them?

(context was snipped, but this question is about the mb() in 
snd_cs4231_dout; snd_wss_dout in the new wss_lib form).

Rene.

Re: [PATCH] wss_lib: do not mess mixer settings during probe

[Takashi Iwai]

At Mon, 25 Aug 2008 00:37:52 +0200,
Rene Herman wrote:
> 
> On 24-08-08 21:48, Krzysztof Helt wrote:
> 
> >From the Documentation/memory-barriers.txt
> > "
> > (*) inX(), outX():
> > ...
> >     They are guaranteed to be fully ordered with respect to each other.
> > 
> >     They are not guaranteed to be fully ordered with respect to other types of
> >      memory and I/O operation.
> > ...
> > "
> > 
> > No barriers are needed in this case. 
> 
> On x86 certainly not and I would expect these barriers are there for 
> cases/architectures where inb() and outb() are redefined to memory 
> mapped I/O or there would just be no point at all.

These barriers are mostly for Alpha.  Not sure whether they are really
needed, though.


Takashi

Re: [PATCH] wss_lib: do not mess mixer settings during probe

[Takashi Iwai]

At Sun, 24 Aug 2008 18:08:03 +0200,
Krzysztof Helt wrote:
> 
> From: Krzysztof Helt <krzysztof.h1@wp.pl>
> 
> Use the wss_dout function which does not mess
> shadowed register values during chip probing.
> Otherwise, user ends up with stupid mixer settings
> after driver loading.
> 
> Signed-off-by: Krzysztof Helt <krzysztof.h1@wp.pl>
> ---
> 
> Especially, the recording settings are messed up like
> left channel recording from different source than
> right channel.

Thanks, now applied.


Takashi