Subject: port-macppc/37076: [patch] snapper(4) mixer controls, channel phase fixes and general cleanup
To: None <port-macppc-maintainer@netbsd.org, gnats-admin@netbsd.org,>
From: None <marcotrillo@gmail.com>
List: netbsd-bugs
Date: 10/07/2007 23:55:00
>Number:         37076
>Category:       port-macppc
>Synopsis:       [patch] snapper(4) mixer controls, channel phase fixes and general cleanup
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    port-macppc-maintainer
>State:          open
>Class:          change-request
>Submitter-Id:   net
>Arrival-Date:   Sun Oct 07 23:55:00 +0000 2007
>Originator:     Marco Trillo
>Release:        NetBSD 4.99.31
>Organization:
>Environment:
NetBSD 4.99.31 (sources from today)
>Description:
Hi,

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

 o The bass, treble and gain controls should have the 0 dB point centered 
at the 128 value. 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.
As they are on current sources, it's difficult to know what value corresponds
to the 0 dB point.

 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.com/darwinsource/10.4.9.ppc/AppleOnboardAudio-256.2.5/AppleLegacyAudio/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 the fixes above, 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()].

 o The "sc_vol_l" and "sc_vol_r" values can also be used to track volume
on "software codec"s to minimize logic duplication.

I hope that this is useful.

Thanks,
Marco

>How-To-Repeat:

>Fix:
Index: snapper.c
===================================================================
RCS file: /cvsroot/src/sys/arch/macppc/dev/snapper.c,v
retrieving revision 1.22
diff -u -r1.22 snapper.c
--- snapper.c	27 Sep 2007 08:49:33 -0000	1.22
+++ snapper.c	7 Oct 2007 12:25:05 -0000
@@ -1,6 +1,7 @@
 /*	$NetBSD: snapper.c,v 1.22 2007/09/27 08:49:33 dogcow Exp $	*/
 /*	Id: snapper.c,v 1.11 2002/10/31 17:42:13 tsubai Exp	*/
-/*     Id: i2s.c,v 1.12 2005/01/15 14:32:35 tsubai Exp         */
+/*	Id: i2s.c,v 1.12 2005/01/15 14:32:35 tsubai Exp		*/
+
 /*-
  * Copyright (c) 2002, 2003 Tsubai Masanari.  All rights reserved.
  *
@@ -30,8 +31,12 @@
 /*
  * Datasheet is available from
  * http://www.ti.com/sc/docs/products/analog/tas3004.html
+ * http://www.ti.com/sc/docs/products/analog/tas3001.html
  */
 
+#include <sys/cdefs.h>
+__KERNEL_RCSID(0, "$NetBSD$");
+
 #include <sys/param.h>
 #include <sys/audioio.h>
 #include <sys/device.h>
@@ -58,10 +63,14 @@
 # define DPRINTF while (0) printf
 #endif
 
+#define SNAPPER_MAXPAGES	16
+
 struct snapper_softc {
 	struct device sc_dev;
-	int sc_flags;
-#define SNAPPER_IS_TAS3001	0x01
+	int sc_mode;		  // 0 for TAS3004
+#define SNAPPER_IS_TAS3001	1 // codec is TAS3001
+#define SNAPPER_SWVOL		2 // software codec
+	
 	int sc_node;
 
 	struct audio_encoding_set *sc_encodings;
@@ -77,23 +86,23 @@
 	u_int sc_record_source;		/* recording source mask */
 	u_int sc_output_mask;		/* output source mask */
 
+	uint32_t sc_baseaddr;
 	u_char *sc_reg;
 	i2c_addr_t sc_deqaddr;
 	i2c_tag_t sc_i2c;
 
 	int sc_rate;                    /* current sampling rate */
 	int sc_bitspersample;
-	int sc_swvol;
 
-	u_int sc_vol_l;
-	u_int sc_vol_r;
-	u_int sc_treble;
-	u_int sc_bass;
-	u_int mixer[6]; /* s1_l, s2_l, an_l, s1_r, s2_r, an_r */
+	u_char sc_vol_l;
+	u_char sc_vol_r;
+	u_char sc_treble;
+	u_char sc_bass;
+	u_char mixer[6]; /* s1_l, s2_l, an_l, s1_r, s2_r, an_r */
 
 	dbdma_regmap_t *sc_odma;
 	dbdma_regmap_t *sc_idma;
-	unsigned char	dbdma_cmdspace[sizeof(struct dbdma_command) * 40 + 15];
+	
 	struct dbdma_command *sc_odmacmd;
 	struct dbdma_command *sc_idmacmd;
 };
@@ -119,10 +128,10 @@
     void *, const audio_params_t *);
 int snapper_trigger_input(void *, void *, void *, int, void (*)(void *),
     void *, const audio_params_t *);
-void snapper_set_volume(struct snapper_softc *, int, int);
+void snapper_set_volume(struct snapper_softc *, u_int, u_int);
 int snapper_set_rate(struct snapper_softc *);
-void snapper_set_treble(struct snapper_softc *, int);
-void snapper_set_bass(struct snapper_softc *, int);
+void snapper_set_treble(struct snapper_softc *, u_int);
+void snapper_set_bass(struct snapper_softc *, u_int);
 void snapper_write_mixers(struct snapper_softc *);
 
 int tas3004_write(struct snapper_softc *, u_int, const void *);
@@ -136,11 +145,18 @@
 
 struct snapper_codecvar {
 	stream_filter_t	base;
+
+#ifdef DIAGNOSTIC
+# define SNAPPER_CODECVAR_MAGIC		0xC0DEC
+	uint32_t	magic;
+#endif // DIAGNOSTIC
+	
+	int16_t		rval; // for snapper_fixphase
 };
 
-static stream_filter_t *snapper_factory
+static stream_filter_t *snapper_filter_factory
 	(int (*)(stream_fetcher_t *, audio_stream_t *, int));
-static void snapper_dtor(stream_filter_t *);
+static void snapper_filter_dtor(stream_filter_t *);
 
 
 /* XXX We can't access the hw device softc from our audio
@@ -158,7 +174,7 @@
 name(struct audio_softc *sc, const audio_params_t *from, \
      const audio_params_t *to) \
 { \
-	return snapper_factory(name##_fetch_to); \
+	return snapper_filter_factory(name##_fetch_to); \
 } \
 static int \
 name##_fetch_to(stream_fetcher_t *self, audio_stream_t *dst, int max_used)
@@ -185,21 +201,63 @@
 	return 0;
 }
 
+/*
+ * A hardware bug in the TAS3004 I2S transport
+ * produces phase differences between channels
+ * (left channel appears delayed by one sample).
+ * Fix the phase difference by delaying the right channel
+ * by one sample.
+ */
+DEFINE_FILTER(snapper_fixphase)
+{
+	struct snapper_codecvar *cv = (struct snapper_codecvar *) self;
+	stream_filter_t *this = &cv->base;
+	int err, m;
+	const int16_t *rp;
+	int16_t *wp, rval = cv->rval;
+
+#ifdef DIAGNOSTIC	
+	if (cv->magic != SNAPPER_CODECVAR_MAGIC)
+		panic("snapper_fixphase");
+#endif
+	max_used = (max_used + 3) & ~2;
+	if ((err = this->prev->fetch_to(this->prev, this->src, max_used)))
+		return err;
+
+	/* work in stereo frames (4 bytes) */
+	m = (dst->end - dst->start) & ~2;
+	m = min(m, max_used);
+	FILTER_LOOP_PROLOGUE(this->src, 4, dst, 4, m) {
+		rp = (const int16_t *) s;
+		wp = (int16_t *) d;
+		wp[0] = rp[0];
+		wp[1] = rval;
+		rval = rp[1];
+	} FILTER_LOOP_EPILOGUE(this->src, dst);
+	cv->rval = rval;
+
+	return 0;
+}
+
 static stream_filter_t *
-snapper_factory(int (*fetch_to)(stream_fetcher_t *, audio_stream_t *, int))
+snapper_filter_factory(int (*fetch_to)(stream_fetcher_t *, audio_stream_t *, int))
 {
 	struct snapper_codecvar *this;
 
 	this = malloc(sizeof(*this), M_DEVBUF, M_WAITOK | M_ZERO);
 	this->base.base.fetch_to = fetch_to;
-	this->base.dtor = snapper_dtor;
+	this->base.dtor = snapper_filter_dtor;
 	this->base.set_fetcher = stream_filter_set_fetcher;
 	this->base.set_inputbuffer = stream_filter_set_inputbuffer;
-	return &this->base;
+
+#ifdef DIAGNOSTIC
+	this->magic = SNAPPER_CODECVAR_MAGIC;
+#endif
+	return (stream_filter_t *) this;
 }
 
 static void
-snapper_dtor(stream_filter_t *this)
+snapper_filter_dtor(stream_filter_t *this)
 {
 	if (this != NULL)
 		free(this, M_DEVBUF);
@@ -235,6 +293,7 @@
 	snapper_get_props,
 	snapper_trigger_output,
 	snapper_trigger_input,
+	NULL,
 	NULL
 };
 
@@ -244,6 +303,7 @@
 	"snapper"
 };
 
+#define SNAPPER_BASSTAB_0DB	18
 const uint8_t snapper_basstab[] = {
 	0x96,	/* -18dB */
 	0x94,	/* -17dB */
@@ -284,6 +344,7 @@
 	0x01,	/* 18dB */
 };
 
+#define SNAPPER_MIXER_GAIN_0DB		36
 const uint8_t snapper_mixer_gain[178][3] = {
 	{ 0x7f, 0x17, 0xaf }, /* 18.0 dB */
 	{ 0x77, 0xfb, 0xaa }, /* 17.5 dB */
@@ -476,7 +537,7 @@
 #define TUMBLER_NFORMATS	1
 static const struct audio_format tumbler_formats[TUMBLER_NFORMATS] = {
 	{NULL, AUMODE_PLAY | AUMODE_RECORD, AUDIO_ENCODING_SLINEAR_BE, 16, 16,
-	 2, AUFMT_STEREO, 3, {32000, 44100, 48000}},
+	 2, AUFMT_STEREO, 4, {32000, 44100, 48000, 96000}},
 };
 
 static u_char *amp_mute;
@@ -497,16 +558,17 @@
 #define I2S_INT_CLKSTOPPEND 0x01000000  /* clock-stop interrupt pending */
 
 /* FCR(0x3c) bits */
-#define I2S0CLKEN      0x1000
-#define I2S0EN         0x2000
-#define I2S1CLKEN      0x080000
-#define I2S1EN         0x100000
+#define KEYLARGO_FCR1   0x3c
+#define  I2S0CLKEN      0x1000
+#define  I2S0EN         0x2000
+#define  I2S1CLKEN      0x080000
+#define  I2S1EN         0x100000
 #define FCR3C_BITMASK "\020\25I2S1EN\24I2S1CLKEN\16I2S0EN\15I2S0CLKEN"
 
-/* TAS3004 registers */
+/* TAS3004/TAS3001 registers */
 #define DEQ_MCR1	0x01	/* Main control register 1 (1byte) */
 #define DEQ_DRC		0x02	/* Dynamic range compression (6bytes?)
-					2 bytes on the TAS 3001 */
+                            	   2 bytes (reserved) on the TAS 3001 */
 #define DEQ_VOLUME	0x04	/* Volume (6bytes) */
 #define DEQ_TREBLE	0x05	/* Treble control (1byte) */
 #define DEQ_BASS	0x06	/* Bass control (1byte) */
@@ -530,9 +592,8 @@
 #define DEQ_RLB		0x22	/* Right loudness biquad (15bytes) */
 #define DEQ_LLB_GAIN	0x23	/* Left loudness biquad gain (3bytes) */
 #define DEQ_RLB_GAIN	0x24	/* Right loudness biquad gain (3bytes) */
-#define DEQ_ACR		0x40	/* Analog control register (1byte) */
-#define DEQ_MCR2	0x43	/* Main control register 2 (1byte) */
-
+#define DEQ_ACR		0x40	/* [TAS3004] Analog control register (1byte) */
+#define DEQ_MCR2	0x43	/* [TAS3004] Main control register 2 (1byte) */
 #define DEQ_MCR1_FL	0x80	/* Fast load */
 #define DEQ_MCR1_SC	0x40	/* SCLK frequency */
 #define  DEQ_MCR1_SC_32	0x00	/*  32fs */
@@ -541,10 +602,10 @@
 #define  DEQ_MCR1_SM_L	0x00	/*  Left justified */
 #define  DEQ_MCR1_SM_R	0x10	/*  Right justified */
 #define  DEQ_MCR1_SM_I2S 0x20	/*  I2S */
-#define DEQ_MCR1_ISM	0x0c	/* Input serial port mode */
-#define  DEQ_MCR1_ISM_L	0x00	/*  Left justified */
-#define  DEQ_MCR1_ISM_R	0x04	/*  Right justified */
-#define  DEQ_MCR1_ISM_I2S 0x08	/*  I2S */
+#define DEQ_MCR1_ISM	0x0c	/* [TAS3001] Input serial port mode */
+#define  DEQ_MCR1_ISM_L	0x00	/*           Left justified */
+#define  DEQ_MCR1_ISM_R	0x04	/*           Right justified */
+#define  DEQ_MCR1_ISM_I2S 0x08	/*           I2S */
 #define DEQ_MCR1_W	0x03	/* Serial port word length */
 #define  DEQ_MCR1_W_16	0x00	/*  16 bit */
 #define  DEQ_MCR1_W_18	0x01	/*  18 bit */
@@ -643,7 +704,7 @@
 	if (strcmp(compat, "AOAK2") == 0)
 		return 1;
 		
-	if (OF_getprop(soundchip,"platform-tas-codec-ref",
+	if (OF_getprop(soundchip, "platform-tas-codec-ref",
 	    &soundcodec, sizeof soundcodec) == sizeof soundcodec)
 		return 1;
 
@@ -655,23 +716,21 @@
 {
 	struct snapper_softc *sc;
 	struct confargs *ca;
-	unsigned long v;
 	int cirq, oirq, iirq, cirq_type, oirq_type, iirq_type;
 	int soundbus, intr[6];
 	char compat[32];
 
-	sc = (struct snapper_softc *)self;
+	sc = device_private(self);
 	ca = aux;
 
 	soundbus = OF_child(ca->ca_node);
 	bzero(compat, sizeof compat);
 	OF_getprop(OF_child(soundbus), "compatible", compat, sizeof compat);
 
-	sc->sc_flags = 0;
 	if (strcmp(compat, "tumbler") == 0)
-		sc->sc_flags |= SNAPPER_IS_TAS3001;
+		sc->sc_mode = SNAPPER_IS_TAS3001;
 
-	if (sc->sc_flags & SNAPPER_IS_TAS3001) {
+	if (sc->sc_mode == SNAPPER_IS_TAS3001) {
 		if (auconv_create_encodings(tumbler_formats, TUMBLER_NFORMATS,
 				&sc->sc_encodings) != 0) {
 			aprint_normal("can't create encodings\n");
@@ -685,17 +744,12 @@
 		}
 	}
 
-	v = (((unsigned long) &sc->dbdma_cmdspace[0]) + 0xf) & ~0xf;
-	sc->sc_odmacmd = (struct dbdma_command *) v;
-	sc->sc_idmacmd = sc->sc_odmacmd + 20;
-
-#ifdef DIAGNOSTIC
-	if ((vaddr_t)sc->sc_odmacmd & 0x0f) {
-		printf(": bad dbdma alignment\n");
-		return;
-	}
-#endif
+	sc->sc_odmacmd = dbdma_alloc((SNAPPER_MAXPAGES + 4) * 
+				     sizeof(struct dbdma_command));
+	sc->sc_idmacmd = dbdma_alloc((SNAPPER_MAXPAGES + 4) * 
+				     sizeof(struct dbdma_command));
 
+	sc->sc_baseaddr = ca->ca_baseaddr;
 	ca->ca_reg[0] += ca->ca_baseaddr;
 	ca->ca_reg[2] += ca->ca_baseaddr;
 	ca->ca_reg[4] += ca->ca_baseaddr;
@@ -729,24 +783,32 @@
 	struct device *dv;
 	struct deq_softc *deq;
 	
-	sc = (struct snapper_softc *)dev;
-	for (dv = alldevs.tqh_first; dv; dv=dv->dv_list.tqe_next)
-		if (strncmp(dv->dv_xname, "deq", 3) == 0 &&
-		    strncmp(device_parent(dv)->dv_xname, "ki2c", 4) == 0) {
-		    	deq=(struct deq_softc *)dv;
+	sc = device_private(dev);
+	TAILQ_FOREACH(dv, &alldevs, dv_list) {
+		if (device_is_a(dv, "deq")) {
+			deq = device_private(dv);
 			sc->sc_i2c = deq->sc_i2c;
-			sc->sc_deqaddr=deq->sc_address;
+			sc->sc_deqaddr = deq->sc_address;
 		}
+	}
 
 	/* If we don't find a codec, it's not the end of the world;
 	 * we can control the volume in software in this case.
 	 */
-	if (sc->sc_i2c == NULL) {
-		aprint_verbose("%s: software codec\n",
-		    sc->sc_dev.dv_xname);
-		sc->sc_swvol = 1;
-	} else
-		sc->sc_swvol = 0;
+	if (sc->sc_i2c == NULL)
+		sc->sc_mode = SNAPPER_SWVOL;
+
+	switch (sc->sc_mode) {
+	case SNAPPER_SWVOL:
+		aprint_verbose("%s: software codec\n", device_xname(dev));
+		break;
+	case SNAPPER_IS_TAS3001:
+		aprint_verbose("%s: codec: TAS3001\n", device_xname(dev));
+		break;
+	case 0:
+		aprint_verbose("%s: codec: TAS3004\n", device_xname(dev));
+		break;
+	}
 
 	audio_attach_mi(&snapper_hw_if, sc, &sc->sc_dev);
 
@@ -838,14 +900,8 @@
 			continue;
 
 		p = mode == AUMODE_PLAY ? play : rec;
-		if (p->sample_rate < 4000 || p->sample_rate > 50000) {
-			DPRINTF("snapper_set_params: invalid rate %d\n", 
-			    p->sample_rate);
-			return EINVAL;
-		}
-
 		fil = mode == AUMODE_PLAY ? pfil : rfil;
-		if (sc->sc_flags & SNAPPER_IS_TAS3001) {
+		if (sc->sc_mode == SNAPPER_IS_TAS3001) {
 			if (auconv_set_converter(tumbler_formats,
 				    TUMBLER_NFORMATS, mode, p, true, fil) < 0) {
 				DPRINTF("snapper_set_params: "
@@ -861,10 +917,20 @@
 			}
 		}
 
-		if (sc->sc_swvol)
-			fil->append(fil, snapper_volume, p);
 		if (fil->req_size > 0)
 			p = &fil->filters[0].param;
+		if (p->precision == 16) {
+			if (sc->sc_mode == SNAPPER_SWVOL)
+				fil->prepend(fil, snapper_volume, p);
+			else if (sc->sc_mode == 0 && p->channels == 2) {
+				/*
+				 * Fix phase problems on TAS3004.
+				 * This filter must go last on the chain,
+				 * so prepend it, not append it.
+				 */
+				fil->prepend(fil, snapper_fixphase, p);
+			}
+		}
 	}
 
 	/* Set the speed. p points HW encoding. */
@@ -967,7 +1033,7 @@
 		return 0;
 
 	case SNAPPER_INPUT_SELECT:
-		if (sc->sc_flags & SNAPPER_IS_TAS3001)
+		if (sc->sc_mode != 0)
 			return ENXIO;
 
 		/* no change necessary? */
@@ -995,31 +1061,41 @@
 		return 0;
 
 	case SNAPPER_BASS:
-		snapper_set_bass(sc,l);
+		if (sc->sc_mode == SNAPPER_SWVOL)
+			return ENXIO;
+		snapper_set_bass(sc, l);
 		return 0;
 	case SNAPPER_TREBLE:
-		snapper_set_treble(sc,l);
+		if (sc->sc_mode == SNAPPER_SWVOL)
+			return ENXIO;
+		snapper_set_treble(sc, l);
 		return 0;
 	case SNAPPER_DIGI1:
-		sc->mixer[0]=l;
-		sc->mixer[3]=r;
+		if (sc->sc_mode == SNAPPER_SWVOL)
+			return ENXIO;
+
+		sc->mixer[0] = l;
+		sc->mixer[3] = r;
 		snapper_write_mixers(sc);
 		return 0;
 	case SNAPPER_DIGI2:
-		if (sc->sc_flags & SNAPPER_IS_TAS3001)
+		if (sc->sc_mode == SNAPPER_SWVOL)
+			return ENXIO;
+
+		if (sc->sc_mode == SNAPPER_IS_TAS3001)
 			sc->mixer[3] = l;
 		else {
-			sc->mixer[1]=l;
-			sc->mixer[4]=r;
+			sc->mixer[1] = l;
+			sc->mixer[4] = r;
 		}
 		snapper_write_mixers(sc);
 		return 0;
 	case SNAPPER_ANALOG:
-		if (sc->sc_flags & SNAPPER_IS_TAS3001)
+		if (sc->sc_mode != 0)
 			return ENXIO;
 
-		sc->mixer[2]=l;
-		sc->mixer[5]=r;
+		sc->mixer[2] = l;
+		sc->mixer[5] = r;
 		snapper_write_mixers(sc);
 		return 0;
 	}	
@@ -1039,17 +1115,12 @@
 		return 0;
 
 	case SNAPPER_VOL_OUTPUT:
-		if (sc->sc_swvol) {
-			mc->un.value.level[AUDIO_MIXER_LEVEL_LEFT] = snapper_vol_l;
-			mc->un.value.level[AUDIO_MIXER_LEVEL_RIGHT] = snapper_vol_r;
-		} else {
-			mc->un.value.level[AUDIO_MIXER_LEVEL_LEFT] = sc->sc_vol_l;
-			mc->un.value.level[AUDIO_MIXER_LEVEL_RIGHT] = sc->sc_vol_r;
-		}
+		mc->un.value.level[AUDIO_MIXER_LEVEL_LEFT] = sc->sc_vol_l;
+		mc->un.value.level[AUDIO_MIXER_LEVEL_RIGHT] = sc->sc_vol_r;
 		return 0;
 
 	case SNAPPER_INPUT_SELECT:
-		if (sc->sc_flags & SNAPPER_IS_TAS3001)
+		if (sc->sc_mode != 0)
 			return ENXIO;
 
 		mc->un.mask = sc->sc_record_source;
@@ -1060,22 +1131,34 @@
 		mc->un.value.level[AUDIO_MIXER_LEVEL_LEFT] = 0;
 		mc->un.value.level[AUDIO_MIXER_LEVEL_RIGHT] = 0;
 		return 0;
+
 	case SNAPPER_TREBLE:
+		if (sc->sc_mode == SNAPPER_SWVOL)
+			return ENXIO;
 		mc->un.value.level[AUDIO_MIXER_LEVEL_MONO] = sc->sc_treble;
 		return 0;
 	case SNAPPER_BASS:
+		if (sc->sc_mode == SNAPPER_SWVOL)
+			return ENXIO;
 		mc->un.value.level[AUDIO_MIXER_LEVEL_MONO] = sc->sc_bass;
 		return 0;
+
 	case SNAPPER_DIGI1:
+		if (sc->sc_mode == SNAPPER_SWVOL)
+			return ENXIO;
+
 		mc->un.value.level[AUDIO_MIXER_LEVEL_LEFT] = sc->mixer[0];
 		mc->un.value.level[AUDIO_MIXER_LEVEL_RIGHT] = sc->mixer[3];
 		return 0;
 	case SNAPPER_DIGI2:
+		if (sc->sc_mode == SNAPPER_SWVOL)
+			return ENXIO;
+
 		mc->un.value.level[AUDIO_MIXER_LEVEL_LEFT] = sc->mixer[1];
 		mc->un.value.level[AUDIO_MIXER_LEVEL_RIGHT] = sc->mixer[4];
 		return 0;
 	case SNAPPER_ANALOG:
-		if (sc->sc_flags & SNAPPER_IS_TAS3001)
+		if (sc->sc_mode != 0)
 			return ENXIO;
 
 		mc->un.value.level[AUDIO_MIXER_LEVEL_LEFT] = sc->mixer[2];
@@ -1117,7 +1200,7 @@
 		return 0;
 
 	case SNAPPER_INPUT_SELECT:
-		if (sc->sc_flags & SNAPPER_IS_TAS3001)
+		if (sc->sc_mode != 0)
 			return ENXIO;
 
 		dip->mixer_class = SNAPPER_RECORD_CLASS;
@@ -1162,6 +1245,9 @@
 		return 0;
 
 	case SNAPPER_TREBLE:
+		if (sc->sc_mode == SNAPPER_SWVOL)
+			return ENXIO;
+
 		dip->mixer_class = SNAPPER_MONITOR_CLASS;
 		strcpy(dip->label.name, AudioNtreble);
 		dip->type = AUDIO_MIXER_VALUE;
@@ -1170,6 +1256,9 @@
 		return 0;
 
 	case SNAPPER_BASS:
+		if (sc->sc_mode == SNAPPER_SWVOL)
+			return ENXIO;
+
 		dip->mixer_class = SNAPPER_MONITOR_CLASS;
 		strcpy(dip->label.name, AudioNbass);
 		dip->type = AUDIO_MIXER_VALUE;
@@ -1178,23 +1267,29 @@
 		return 0;
 
 	case SNAPPER_DIGI1:
+		if (sc->sc_mode == SNAPPER_SWVOL)
+			return ENXIO;
+
 		dip->mixer_class = SNAPPER_MONITOR_CLASS;
 		strcpy(dip->label.name, AudioNdac);
 		dip->type = AUDIO_MIXER_VALUE;
 		dip->prev = dip->next = AUDIO_MIXER_LAST;
 		dip->un.v.num_channels =
-			sc->sc_flags & SNAPPER_IS_TAS3001? 1 : 2;
+			sc->sc_mode == SNAPPER_IS_TAS3001? 1 : 2;
 		return 0;
 	case SNAPPER_DIGI2:
+		if (sc->sc_mode == SNAPPER_SWVOL)
+			return ENXIO;
+
 		dip->mixer_class = SNAPPER_MONITOR_CLASS;
 		strcpy(dip->label.name, AudioNline);
 		dip->type = AUDIO_MIXER_VALUE;
 		dip->prev = dip->next = AUDIO_MIXER_LAST;
 		dip->un.v.num_channels =
-			sc->sc_flags & SNAPPER_IS_TAS3001? 1 : 2;
+			sc->sc_mode == SNAPPER_IS_TAS3001? 1 : 2;
 		return 0;
 	case SNAPPER_ANALOG:
-		if (sc->sc_flags & SNAPPER_IS_TAS3001)
+		if (sc->sc_mode != 0)
 			return ENXIO;
 
 		dip->mixer_class = SNAPPER_MONITOR_CLASS;
@@ -1255,7 +1350,7 @@
 	sc->sc_opages = ((char *)end - (char *)start) / NBPG;
 
 #ifdef DIAGNOSTIC
-	if (sc->sc_opages > 16)
+	if (sc->sc_opages > SNAPPER_MAXPAGES)
 		panic("snapper_trigger_output");
 #endif
 
@@ -1310,7 +1405,7 @@
 	sc->sc_ipages = ((char *)end - (char *)start) / NBPG;
 
 #ifdef DIAGNOSTIC
-	if (sc->sc_ipages > 16)
+	if (sc->sc_ipages > SNAPPER_MAXPAGES)
 		panic("snapper_trigger_input");
 #endif
 
@@ -1343,31 +1438,31 @@
 }
 
 void
-snapper_set_volume(struct snapper_softc *sc, int left, int right)
+snapper_set_volume(struct snapper_softc *sc, u_int left, u_int right)
 {
 	u_char regs[6];
 	int l, r;
 
-	if (sc->sc_swvol) {
+	left &= 0xFF;
+	right &= 0xFF;
+
+	if (sc->sc_mode == SNAPPER_SWVOL) {
 		snapper_vol_l = left;
 		snapper_vol_r = right;
-		return;
-	}
-
-	/*
-	 * for some insane reason the gain table for master volume and the
-	 * mixer channels is almost identical - just shifted by 4 bits
-	 * so we use the mixer_gain table and bit-twiddle it... 
-	 */
-	if ((left >= 0) && (left < 256) && (right >= 0) && (right < 256)) {
-		l = 177 - (left * 177 / 255);
+	} else {
+		/*
+		 * for some insane reason the gain table for master volume and the
+		 * mixer channels is almost identical - just shifted by 4 bits
+		 * so we use the mixer_gain table and bit-twiddle it... 
+		 */
+		l = 177 - (left * 178 / 256);
 		regs[0] =  (snapper_mixer_gain[l][0] >> 4);
 		regs[1] = ((snapper_mixer_gain[l][0] & 0x0f) << 4) | 
 			   (snapper_mixer_gain[l][1] >> 4);
 		regs[2] = ((snapper_mixer_gain[l][1] & 0x0f) << 4) | 
 			   (snapper_mixer_gain[l][2] >> 4);
 
-		r = 177 - (right * 177 / 255);
+		r = 177 - (right * 178 / 256);
 		regs[3] =  (snapper_mixer_gain[r][0] >> 4);
 		regs[4] = ((snapper_mixer_gain[r][0] & 0x0f) << 4) | 
 			   (snapper_mixer_gain[r][1] >> 4);
@@ -1376,53 +1471,97 @@
 
 		tas3004_write(sc, DEQ_VOLUME, regs);
 		
-		sc->sc_vol_l = left;
-		sc->sc_vol_r = right;
-
 		DPRINTF("%d %02x %02x %02x : %d %02x %02x %02x\n", l, regs[0], 	
 		    regs[1], regs[2], r, regs[3], regs[4], regs[5]);
 	}
+
+	sc->sc_vol_l = left;
+	sc->sc_vol_r = right;
 }
 
-void snapper_set_treble(struct snapper_softc *sc, int stuff)
+static void
+snapper_set_basstreble(struct snapper_softc *sc, u_int val, u_int mode)
 {
+	int i = val & 0xFF;
 	uint8_t reg;
-	if ((stuff >= 0) && (stuff <= 255) && (sc->sc_treble != stuff)) {
-		reg = snapper_basstab[(stuff >> 3) + 2];
-		sc->sc_treble = stuff;
-		tas3004_write(sc, DEQ_TREBLE, &reg);
+
+	/*
+	 * Make 128 match the 0 dB point
+	 */
+	i = (i - (128 - (SNAPPER_BASSTAB_0DB << 2))) >> 2;
+	if (i < 0)
+		i = 0;
+	else if (i >= sizeof(snapper_basstab))
+		i = sizeof(snapper_basstab) - 1;
+	reg = snapper_basstab[i];
+
+	if (sc->sc_mode == SNAPPER_IS_TAS3001 && 
+	    mode == DEQ_BASS) {
+	    /*
+	     * XXX -- The TAS3001 bass table is different 
+	     *        than the other tables.
+	     */
+	    reg = (reg >> 1) + 5; // map 0x72 -> 0x3E (0 dB)
 	}
+	
+	tas3004_write(sc, mode, &reg);
 }
 
-void snapper_set_bass(struct snapper_softc *sc, int stuff)
+void
+snapper_set_treble(struct snapper_softc *sc, u_int val)
 {
-	uint8_t reg;
-	if ((stuff >= 0) && (stuff <= 255) && (stuff != sc->sc_bass)) {
-		reg = snapper_basstab[(stuff >> 3) + 2];
-		sc->sc_bass = stuff;
-		tas3004_write(sc, DEQ_BASS, &reg);
+	if (sc->sc_treble != (u_char)val) {
+		sc->sc_treble = val;
+		snapper_set_basstreble(sc, val, DEQ_TREBLE);
 	}
 }
 
-void snapper_write_mixers(struct snapper_softc *sc)
+void
+snapper_set_bass(struct snapper_softc *sc, u_int val)
+{
+	if (sc->sc_bass != (u_char)val) {
+		sc->sc_bass = val;
+		snapper_set_basstreble(sc, val, DEQ_BASS);
+	}
+}
+
+
+/*
+ * In the mixer gain setting, make 128 correspond to
+ * the 0dB value from the table.
+ * Note that the table values are complemented.
+ */
+#define SNAPPER_MIXER_GAIN_SIZE	(sizeof(snapper_mixer_gain) / \
+                               	 sizeof(snapper_mixer_gain[0]))
+#define NORMALIZE(i)	((~(i) & 0xff) - ((~128 & 0xff) - SNAPPER_MIXER_GAIN_0DB))
+#define ADJUST(v, i)	do { \
+                		(v) = NORMALIZE(i);\
+				if ((v) < 0) \
+					(v) = 0; \
+				else if ((v) >= SNAPPER_MIXER_GAIN_SIZE) \
+					(v) = SNAPPER_MIXER_GAIN_SIZE - 1; \
+				\
+			} while (0)
+void
+snapper_write_mixers(struct snapper_softc *sc)
 {
 	uint8_t regs[9] = {0, 0, 0, 0, 0, 0, 0, 0, 0};
 	int i;
 
 	/* Left channel of SDIN1 */
-	i = 177 - (sc->mixer[0] * 177 / 255);
+	ADJUST(i, sc->mixer[0]);
 	regs[0] = snapper_mixer_gain[i][0];
 	regs[1] = snapper_mixer_gain[i][1];
 	regs[2] = snapper_mixer_gain[i][2];
 
 	/* Left channel of SDIN2 */
-	i = 177 - (sc->mixer[1] * 177 / 255);
+	ADJUST(i, sc->mixer[1]);
 	regs[3] = snapper_mixer_gain[i][0];
 	regs[4] = snapper_mixer_gain[i][1];
 	regs[5] = snapper_mixer_gain[i][2];
 
 	/* Left channel of analog input */
-	i = 177 - (sc->mixer[2] * 177 / 255);
+	ADJUST(i, sc->mixer[2]);
 	regs[6] = snapper_mixer_gain[i][0];
 	regs[7] = snapper_mixer_gain[i][1];
 	regs[8] = snapper_mixer_gain[i][2];
@@ -1430,19 +1569,19 @@
 	tas3004_write(sc, DEQ_MIXER_L, regs);
 
 	/* Right channel of SDIN1 */
-	i = 177 - (sc->mixer[3] * 177 / 255);
+	ADJUST(i, sc->mixer[3]);
 	regs[0] = snapper_mixer_gain[i][0];
 	regs[1] = snapper_mixer_gain[i][1];
 	regs[2] = snapper_mixer_gain[i][2];
 
 	/* Right channel of SDIN2 */
-	i = 177 - (sc->mixer[4] * 177 / 255);
+	ADJUST(i, sc->mixer[4]);
 	regs[3] = snapper_mixer_gain[i][0];
 	regs[4] = snapper_mixer_gain[i][1];
 	regs[5] = snapper_mixer_gain[i][2];
 
 	/* Right channel of analog input */
-	i = 177 - (sc->mixer[5] * 177 / 255);
+	ADJUST(i, sc->mixer[5]);
 	regs[6] = snapper_mixer_gain[i][0];
 	regs[7] = snapper_mixer_gain[i][1];
 	regs[8] = snapper_mixer_gain[i][2];
@@ -1500,6 +1639,7 @@
 
 	case 32000:
 	case 48000:
+	case 96000:
 		clksrc = 49152000;		/* 49MHz */
 		reg = CLKSRC_49MHz;
 		mclk_fs = 256;
@@ -1563,7 +1703,7 @@
 			return EINVAL;
 	}
 
-	if (sc->sc_flags & SNAPPER_IS_TAS3001)
+	if (sc->sc_mode == SNAPPER_IS_TAS3001)
 		mcr1 |= DEQ_MCR1_ISM_I2S;
 
 	ows = in32rb(sc->sc_reg + I2S_WORDSIZE);
@@ -1571,7 +1711,8 @@
 	    ows, wordsize);
 	if (ows != wordsize) {
 		out32rb(sc->sc_reg + I2S_WORDSIZE, wordsize);
-		tas3004_write(sc, DEQ_MCR1, &mcr1);
+		if (sc->sc_mode != SNAPPER_SWVOL)
+			tas3004_write(sc, DEQ_MCR1, &mcr1);
 	}
 
 	x = in32rb(sc->sc_reg + I2S_FORMAT);
@@ -1584,9 +1725,9 @@
 	/* Clear CLKSTOPPEND. */
 	out32rb(sc->sc_reg + I2S_INT, I2S_INT_CLKSTOPPEND);
 
-	x = in32rb(0x8000003c);                /* FCR */
+	x = in32rb(sc->sc_baseaddr + KEYLARGO_FCR1);
 	x &= ~I2S0CLKEN;                /* XXX I2S0 */
-	out32rb(0x8000003c, x);
+	out32rb(sc->sc_baseaddr + KEYLARGO_FCR1, x);
 
 	/* Wait until clock is stopped. */
 	for (timo = 1000; timo > 0; timo--) {
@@ -1598,9 +1739,9 @@
 done:
 	out32rb(sc->sc_reg + I2S_FORMAT, reg);
 
-	x = in32rb(0x8000003c);
+	x = in32rb(sc->sc_baseaddr + KEYLARGO_FCR1);
 	x |= I2S0CLKEN;
-	out32rb(0x8000003c, x);
+	out32rb(sc->sc_baseaddr + KEYLARGO_FCR1, x);
 
 	return 0;
 }
@@ -1693,13 +1834,13 @@
 
 	regblock[0] = reg;
 	memcpy(&regblock[1], data, size);
-	if (sc->sc_flags & SNAPPER_IS_TAS3001) {
+	if (sc->sc_mode == SNAPPER_IS_TAS3001) {
 		if (reg == DEQ_MIXER_L || reg == DEQ_MIXER_R)
 			size = 3;
-		else if (reg == DEQ_DRC) {
-			size = 2;
-			regblock[1] = 0;
-			regblock[2] = 0;
+		else if (reg == DEQ_DRC || reg == DEQ_ACR || 
+			 reg == DEQ_MCR2) {
+			/* these registers are not available on TAS3001 */
+			return 0;
 		}
 	}
 	iic_acquire_bus(sc->sc_i2c, 0);
@@ -1859,7 +2000,7 @@
 #ifdef SNAPPER_DEBUG
 	char fcr[32];
 
-	bitmask_snprintf(in32rb(0x8000003c), FCR3C_BITMASK, fcr, sizeof fcr);
+	bitmask_snprintf(in32rb(sc->sc_baseaddr + KEYLARGO_FCR1), FCR3C_BITMASK, fcr, sizeof fcr);
 	printf("FCR(0x3c) 0x%s\n", fcr);
 #endif
 	headphone_detect_intr = -1;
@@ -1927,9 +2068,8 @@
 	snapper_cint(sc);
 
 	snapper_set_volume(sc, 128, 128);
-	
-	sc->sc_bass = 128;
-	sc->sc_treble = 128;
+	snapper_set_bass(sc, 128);
+	snapper_set_treble(sc, 128);
 
 	/* Record source defaults to microphone.  This reflects the
 	 * default value for the ACR (see tas3004_initdata).
@@ -1937,11 +2077,12 @@
 	sc->sc_record_source = 1 << 0;
 	
 	/* We mute the analog input for now */
-	sc->mixer[0] = 80;
-	sc->mixer[1] = 80;
+	sc->mixer[0] = 128;
+	sc->mixer[1] = 128;
 	sc->mixer[2] = 0;
-	sc->mixer[3] = 80;
-	sc->mixer[4] = 80;
+	sc->mixer[3] = 128;
+	sc->mixer[4] = 128;
 	sc->mixer[5] = 0;
 	snapper_write_mixers(sc);
 }
+