snapper(4) issues

Hi,

The attached patch fixes a couple of issues with the
snapper(4) driver:

 o In the bass, treble and gain controls, center the 0 dB
point at the
value 128. This is to make them easier to configure and set
up
correctly: the 128 value is the neutral value (so
bass/treble/gain is
disabled -- which is what is desired in many cases). Values
higher
than 128 emphasize bass/treble or add gain; values lesser
than 128
deemphasize bass/treble or add negative gain. Also make 128
the
default value.

 o The TAS3004 seems to have a hardware fault which causes
it to delay
the left channel by one sample, causing phase differences
between
channels. This is explained in the Apple code, as shown in
this quote
from the AppleTAS3004Audio.cpp file:

// [...] fixing the phase relationship between the
// left and right channels where a hardware fault in the
TAS3004 results in
// a one sample delay of the left channel [...]

This issue can be compensated by delaying the right channel
by one
sample. The attached patch implements this using a simple
filter.
This is a equivalent solution to what Apple does in:

<http://www.opensource.apple.co
m/darwinsource/10.4.9.ppc/AppleOnboardAudio-256.2.5/AppleLeg
acyAudio/AppleDBDMAAudio/Apple02DBDMAAudioClip.c>
(function delayRightChannel(), note that in the Apple code
there are
two TAS3004 drivers, and one of them implements another
solution which
involves programming the TAS3004 biquad filters to delay the
right
channel, but this seems more difficult to get right)

This problem does not affect the TAS3001.

 o The TAS3001 bass table is different from the table used
in the code
(which corresponds roughly to TAS3004 bass/treble and
TAS3001 treble),
so bass values are set up incorrectly. And there is a lot of
error,
for example, 0 dB is 0x3E in this table while 0x72 in the
other table.
Fix this issue using simple method (which at least gets 0 dB
right).

Apart from that, the patch also contains the following
general cleanup:

 o Don't hardcode absolute addresses for the FCR registers.
Use the
"baseaddr" parameter passed from obio(4).

 o Use dbdma_alloc() to alloc the command buffers instead of
doing the
alignment manually. Don't hardcode the "max pages"
value.

 o The "sc_swvol" and "sc_flags" members
are redundant: use a single
mode member.

 o Some mixer parameters should be disabled if a
"software codec" is used.

 o Be a bit more verbose and print if the codec is TAS3001
or TAS3004
(not only if there is a software codec).

 o The mixer members of snapper_softc can be u_char, not
u_int, since
they are 0-255 values.

 o Use TAILQ_FOREACH() and device_is_a() where appropiate
[instead of
a manual loop and strncmp()].

I hope that this is useful!

Thanks,
Marco

On Sun, Oct 07, 2007 at 01:36:10PM +0000, Marco Trillo
wrote:
> The attached patch fixes a couple of issues with the
snapper(4) driver:

Thanks, Marco.  Can you please use send-pr to submit patches
(perhaps
with a note to the mailing list referencing the PR)?  It's
possible for
mailing list mail to get overlooked, while the PR database
is more
easily searched and trackable.

Thanks again!
-allen

-- 
Allen Briggs  |  http://www.ninthw
onder.com/~briggs/  |  briggsninthwonder.com

Hi,

On 10/7/07, Allen Briggs <briggsnetbsd.org> wrote:
> On Sun, Oct 07, 2007 at 01:36:10PM +0000, Marco Trillo
wrote:
> > The attached patch fixes a couple of issues with
the snapper(4) driver:
>
> Thanks, Marco.  Can you please use send-pr to submit
patches (perhaps
> with a note to the mailing list referencing the PR)? 
It's possible for
> mailing list mail to get overlooked, while the PR
database is more
> easily searched and trackable.

Sure, I've just sent it -- should appear on the database
soon.

Thanks,
Marco

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello,

On Oct 7, 2007, at 09:36, Marco Trillo wrote:

> The attached patch fixes a couple of issues with the
snapper(4)  
> driver:
>
>  o In the bass, treble and gain controls, center the 0
dB point at the
> value 128. This is to make them easier to configure and
set up
> correctly: the 128 value is the neutral value (so
bass/treble/gain is
> disabled -- which is what is desired in many cases).
Values higher
> than 128 emphasize bass/treble or add gain; values
lesser than 128
> deemphasize bass/treble or add negative gain. Also make
128 the
> default value.

Hmm, I thought it already was that way. At least it was my
intention  
when adding bass/treble controls for TAS3004.

> Apart from that, the patch also contains the following
general  
> cleanup:
>
>  o Don't hardcode absolute addresses for the FCR
registers. Use the
> "baseaddr" parameter passed from obio(4).

Yeah, that had to go away.

>  o Some mixer parameters should be disabled if a
"software codec"  
> is used.

Indeed.

> I hope that this is useful!

I'll have a more detailed look later today, thanks for
working on this!

have fun
Michael
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)

iQEVAwUBRwkOLMpnzkX8Yg2nAQKMmQgAi3tJEQuyT509VYuZzC5dSYMGO52B
lL9e
AxnjhaR7uGW96rneL9DQLCjYi1BBKN7SXv4sTnyksMrcGIHlLxO92hOpshL2
UOWS
NkntJX31f0jB9anS+VAEmPBN79iOLPRdlMylcqu2jn+sCrCZQhCRvSf65bju
vEos
LR8QWyiPtGO9eDbl+dGRS8xa/WOjg61NsvM0DA4MQTj+sU6hPTbuCKModSI5
gbcu
kBrfQdrMeYwJyt6pZsYRCjmcVKQzccX70mB5vTb3j0H+ibDUPCcK9nsqB67Q
8Edq
N6VywAW9IdeagWngKJxZ6xG4/LbiEAB8qTL1vJJXipzBus3XKrqkgg==
=Pant
-----END PGP SIGNATURE-----

Hi,

On 10/7/07, Michael Lorenz <macallannetbsd.org> wrote:
> >  o In the bass, treble and gain controls, center
the 0 dB point at the
> > value 128. This is to make them easier to
configure and set up
> > correctly: the 128 value is the neutral value (so
bass/treble/gain is
> > disabled -- which is what is desired in many
cases). Values higher
> > than 128 emphasize bass/treble or add gain; values
lesser than 128
> > deemphasize bass/treble or add negative gain. Also
make 128 the
> > default value.
>
> Hmm, I thought it already was that way. At least it was
my intention
> when adding bass/treble controls for TAS3004.

Yeah, it's the mixer gain setting (the
"monitor.dac" setting in
mixerctl) the one that has the problem (0 dB corresponds
non-exactly
at the 208 value). I erroneously  extrapolated that to the
bass/treble
ones too, but they are correct. Sorry for mentioning them.

By the way, the patch is already at PR database with id
37076...

Thanks,
Marco.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello,

On Oct 8, 2007, at 09:33, Marco Trillo wrote:

> On 10/7/07, Michael Lorenz <macallannetbsd.org> wrote:
>>>  o In the bass, treble and gain controls,
center the 0 dB point  
>>> at the
>>> value 128. This is to make them easier to
configure and set up
>>> correctly: the 128 value is the neutral value
(so bass/treble/ 
>>> gain is
>>> disabled -- which is what is desired in many
cases). Values higher
>>> than 128 emphasize bass/treble or add gain;
values lesser than 128
>>> deemphasize bass/treble or add negative gain.
Also make 128 the
>>> default value.
>>
>> Hmm, I thought it already was that way. At least it
was my intention
>> when adding bass/treble controls for TAS3004.
>
> Yeah, it's the mixer gain setting (the
"monitor.dac" setting in
> mixerctl) the one that has the problem (0 dB
corresponds non-exactly
> at the 208 value). I erroneously  extrapolated that to
the bass/treble
> ones too, but they are correct. Sorry for mentioning
them.

Ah, that explains it. I didn't do the gain table, only the
bass/ 
treble one.

have fun
Michael
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)

iQEVAwUBRwqAnspnzkX8Yg2nAQLZ7QgAtlWrNsfPcmUdVhbCaBN9qa2ziu87
YRoy
BH54GEhIhqi8XLjh1VembFEKkY0KbtLkXVi4LJkNRdDLthsrq/Sqg05qhQ2z
C4DI
YthfK2V2MqjrOev9Bmu8UG9Nx/x7ROxuvxNrdmMvbCy0/IQntmyqbnQzGIYk
71kE
1r8iiyFA91FDTeDsz0kcc97/1sqJKoaG63h7/Q1/adKwlKZrOxjnokT+W825
D2RP
6c8TYYFAEwe1BdUX02l6n/mk4xcQe6k17LIyZGpi//kboAcfvNT8mtcdC17D
ct+p
wmmGntK/EkXA0FNAxnh7zgAr3HXz9wZtrT6WhrZiaQlsM5s11jqp1g==
=0qqS
-----END PGP SIGNATURE-----