ess maestro, support for hardware volume control

this patch applies to (at least) 2.4.3 up to and including 2.4.6-pre2. It enables the hardware volume control feature of the maestro.

By giving hwv=0 to insmod one can explicitly disable it. Setting

Quoted 2 lines

By giving hwv=0 to insmod one can explicitly disable it. Setting

can we have a better name like 'hwvol_enable'?

Review

Quoted 1 line

(quoted diff) +1 -0

+		set_mixer(c, 0, val);

careful. you just used the indirect ac97 registers without holding the card's lock.. if another processor does a mixer ioctl while this is happening you'll get weird behaviour.

Fix the locking (and the obscure parameter name? :)) and it looks

below is the version with the suggested fixes, and with s/hwv/hwvol/ for hwv_input also.

drivers/sound/maestro.c +37 -6

@@ -115,6 +115,10 @@
  *	themselves, but we'll see.  
  *	
  * History
+ *  v0.15 - Jun 09 2001 - Lukas Schroeder <[email protected]>
+ *	enable hardware volume control (by default)
+ *	add hwvol_enable= to allow disabling of HWV (values are 0 or 1)
+ *	add hwvol_input= to allow selecting the HWV input pins (values are 0 or 1)
  *  (still kind of v0.14) Nov 23 - Alan Cox <[email protected]>
  *	Add clocking= for people with seriously warped hardware
  *  (still v0.14) Nov 10 2000 - Bartlomiej Zolnierkiewicz <[email protected]>

@@ -269,8 +273,13 @@
 
 static int clocking=48000;
 
+/* enable hardware volume control? */
+static int hwvol_enable = 1;
+/* hardware volume input pin selection */
+static int hwvol_input = 0;
+
 /* --------------------------------------------------------------------- */
-#define DRIVER_VERSION "0.14"
+#define DRIVER_VERSION "0.15"
 
 #ifndef PCI_VENDOR_ESS
 #define PCI_VENDOR_ESS			0x125D

@@ -312,6 +321,9 @@
 #define NR_APUS		64
 #define NR_APU_REGS	16
 
+/* steps per hardware volume count */
+#define HWV_MIXER_STEP	15
+
 /* acpi states */
 enum {
 	ACPI_D0=0,

@@ -514,6 +526,7 @@
 
 /* --------------------------------------------------------------------- */
 
+static void set_mixer(struct ess_card *card,unsigned int mixer, unsigned int val ) ;
 static void check_suspend(struct ess_card *card);
 
 static struct ess_card *devs = NULL;

@@ -1898,10 +1911,20 @@
 
 	if(event&(1<<6))
 	{
-		/* XXX if we have a hw volume control int enable
-			all the ints?  doesn't make sense.. */
+		unsigned int val;
+
 		event = inw(c->iobase+0x18);
-		outb(0xFF, c->iobase+0x1A);
+		outb((1<<6), c->iobase+0x1A);
+
+		/* read the HW Master Volume Counter
+                   Bits 7:5       Master Volume Left
+                   Bits 3:1       Master Volume Right
+                */
+		i = inb(c->iobase+0x1f);
+		val = ((HWV_MIXER_STEP * ((i>>1) & 7)) << 8) | HWV_MIXER_STEP * ((i>>5) & 7);
+		spin_lock(&s->lock);
+		set_mixer(c, 0, val);
+		spin_unlock(&s->lock);
 	}
 	else
 	{

@@ -3088,8 +3111,10 @@
 	w&=~(1<<14);		/* External clock */
 
 	w&=~(1<<7);		/* HWV off */
+	if (hwvol_enable) w|=(1<<7);
 	w&=~(1<<6);		/* Debounce off */
-	w&=~(1<<5);		/* GPIO 4:5 */
+	w&=~(1<<5);		/* GPIO 4:5 ; HVI pin selection */
+	if (hwvol_input) w|=(1<<5);
 	w|= (1<<4);             /* Disconnect from the CHI.  Enabling this made a dell 7500 work. */
 	w&=~(1<<2);		/* MIDI fix off (undoc) */
 	w&=~(1<<1);		/* reserved, always write 0 */

@@ -3170,7 +3195,8 @@
 	outw(w, iobase+0x18);
 
 	w=inw(iobase+0x18);
-	w&=~(1<<6);		/* Harpo off */
+	w&=~(1<<6);		/* HWV irq off */
+	if (hwvol_enable) w|=(1<<6);
 	outw(w, iobase+0x18);
 
 	w=inw(iobase+0x18);

@@ -3487,6 +3513,7 @@
 	/* now go to sleep 'till something interesting happens */
 	maestro_power(card,ACPI_D2);
 
+	printk(KERN_INFO "maestro: hardware volume control %senabled\n", (hwvol_enable) ? "" : "not ");
 	printk(KERN_INFO "maestro: %d channels configured.\n", num);
 	return 1; 
 }

@@ -3593,6 +3620,10 @@
 MODULE_PARM(dsps_order,"i");
 MODULE_PARM(use_pm,"i");
 MODULE_PARM(clocking, "i");
+
+MODULE_PARM(hwvol_enable, "i");
+MODULE_PARM(hwvol_input, "i");
+
 
 void cleanup_module(void) {
 	M_printk("maestro: unloading\n");

alan, can you throw this in -ac? I don't think it will cause problems for people with nonstandard wiring on the hw vol pins (read: dell

this patch applies to (at least) 2.4.3 up to and including 2.4.6-pre2. It enables the hardware volume control feature of the maestro.

Quoted 2 lines

this patch applies to (at least) 2.4.3 up to and including 2.4.6-pre2. It enables the hardware volume control feature of the maestro.

it doesnt apply to the current version of the maestro driver (2.4.5-ac) however. I think it is clashing with the docking station support

BTW, what is the officially approved way to open a device on a dynamic misc minor? Reading /proc/misc for the minor number,

then mknod'ing a device and opening it seems to me to have a nasty race condition, am I missing something here?

Quoted 2 lines

BTW, what is the officially approved way to open a device on a dynamic misc minor? Reading /proc/misc for the minor number,

Ask for minor 0 I believe, then load the module then see what you got.

Quoted 2 lines

then mknod'ing a device and opening it seems to me to have a nasty race condition, am I missing something here?

Ultimately if its a device of its own it probably wants to be part of the input device frame work - as for example the volume knob on my USB speakers is

What can happen as I see it is that userspace #2, which wants to talk to a particular misc driver, actually ends up talking to a different one because the minor gets reassigned between reading /proc/misc to find out the number and mknoding and opening the device:

Quoted 5 lines

What can happen as I see it is that userspace #2, which wants to talk to a particular misc driver, actually ends up talking to a different one because the minor gets reassigned between reading /proc/misc to find out the number and mknoding and opening the device:

Looks true to me. So get a real misc device assigned for anything you use 8)

I now have a patch that will output the hwv buttons pressed (up, down, mute) to a new dynamically allocated misc device as letters u, d, m, instead of directly modifying the mixer. Anyone want that? It's more flexible than either the patch that's currently in -ac or Lukas's patch, but you need a little userspace daemon for it to do anything useful.

Quoted 7 lines

I now have a patch that will output the hwv buttons pressed (up, down, mute) to a new dynamically allocated misc device as letters u, d, m, instead of directly modifying the mixer. Anyone want that? It's more flexible than either the patch that's currently in -ac or Lukas's patch, but you need a little userspace daemon for it to do anything useful.

hmm.. how do the alsa guys deal with hw volume? I'm not sure I like adding more stuff to the OSS API.