[patch] ess maestro, support for hardware volume control

[patch] ess maestro, support for hardware volume control

[Lukas Schroeder]

Hi,


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
hwv_input=1 requests the alternative HWV input pins to be used.

The maestro will generate interrupts on an input signal with bit 6 at
iobase+0x1A set. This case was already tested for, but 
a) nothing was done there and
b) this event never occurred anyway b/c the irq was turned off.
Now the master volume counter is read out and the mixer's 
master volume is changed accordingly.



regards,
  lukas



--- linux-2.4.6-pre2/drivers/sound/maestro.c	Sat Jun  9 16:55:22 2001
+++ linux/drivers/sound/maestro.c	Sat Jun  9 17:13:24 2001
@@ -115,6 +115,10 @@
  *	themselves, but we'll see.  
  *	
  * History
+ *  v0.15 - Jun 09 2001 - Lukas Schroeder <lukas@edeal.de>
+ *	enable hardware volume control (by default)
+ *	add hwv= to allow disabling of HWV (values are 0 or 1)
+ *	add hwv_input= to allow selecting the HWV input pins (values are 0 or 1)
  *  (still kind of v0.14) Nov 23 - Alan Cox <alan@redhat.com>
  *	Add clocking= for people with seriously warped hardware
  *  (still v0.14) Nov 10 2000 - Bartlomiej Zolnierkiewicz <bkz@linux-ide.org>
@@ -269,8 +273,13 @@
 	
 static int clocking=48000;
 
+/* enable hardware volume control? */
+static int hwv = 1;
+/* hardware volume input pin selection */
+static int hwv_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,18 @@
 
 	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);
+		set_mixer(c, 0, val);
 	}
 	else
 	{
@@ -3088,8 +3109,10 @@
 	w&=~(1<<14);		/* External clock */
 	
 	w&=~(1<<7);		/* HWV off */
+	if (hwv) w|=(1<<7);
 	w&=~(1<<6);		/* Debounce off */
-	w&=~(1<<5);		/* GPIO 4:5 */
+	w&=~(1<<5);		/* GPIO 4:5 ; HVI pin selection */
+	if (hwv_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 +3193,8 @@
 	outw(w, iobase+0x18);
 
 	w=inw(iobase+0x18);
-	w&=~(1<<6);		/* Harpo off */
+	w&=~(1<<6);		/* HWV irq off */
+	if (hwv) w|=(1<<6);
 	outw(w, iobase+0x18);
 	
 	w=inw(iobase+0x18);
@@ -3487,6 +3511,7 @@
 	/* now go to sleep 'till something interesting happens */
 	maestro_power(card,ACPI_D2);
 
+	printk(KERN_INFO "maestro: hardware volume control %senabled\n", (hwv) ? "" : "not ");
 	printk(KERN_INFO "maestro: %d channels configured.\n", num);
 	return 1; 
 }
@@ -3593,6 +3618,10 @@
 MODULE_PARM(dsps_order,"i");
 MODULE_PARM(use_pm,"i");
 MODULE_PARM(clocking, "i");
+
+MODULE_PARM(hwv, "i");
+MODULE_PARM(hwv_input, "i");
+
 
 void cleanup_module(void) {
 	M_printk("maestro: unloading\n");

Re: [patch] ess maestro, support for hardware volume control

[Zach Brown]

> 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.

cool.  I had support for this in the mega-patch I posted long ago, but
I never seperated and submitted those changes 'cause I'm a moron.

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

can we have a better name like 'hwvol_enable'?

> +		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.

it looks like you should just be able to spin_{,un}lock(card->lock)
around that call, but the maestro locking is so friggin' twisty.. this
gets even more exciting when the driver uses ac97_codec, which the
mega-patch also had in it.

Fix the locking (and the obscure parameter name? :)) and it looks
fine.. good work.

-- 
 zach

Re: [patch] ess maestro, support for hardware volume control

[Lukas Schroeder]

> > By giving hwv=0 to insmod one can explicitly disable it. Setting
> 
> can we have a better name like 'hwvol_enable'?
> 
> > +		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.




regards,
  lukas



--- linux-2.4.6-pre2/drivers/sound/maestro.c	Sat Jun  9 16:55:22 2001
+++ linux/drivers/sound/maestro.c	Sat Jun  9 19:48:34 2001
@@ -115,6 +115,10 @@
  *	themselves, but we'll see.  
  *	
  * History
+ *  v0.15 - Jun 09 2001 - Lukas Schroeder <lukas@edeal.de>
+ *	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 <alan@redhat.com>
  *	Add clocking= for people with seriously warped hardware
  *  (still v0.14) Nov 10 2000 - Bartlomiej Zolnierkiewicz <bkz@linux-ide.org>
@@ -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");

Re: [patch] ess maestro, support for hardware volume control

[Zach Brown]

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

fantastic, thanks lukas.

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
lattitudes), but if it does we can blacklist the lattitudes (later turning
into enabling support code for their custom hw vol wiring.. its on the
todo list :/).  This should work on inspirons.

- z

> --- linux-2.4.6-pre2/drivers/sound/maestro.c	Sat Jun  9 16:55:22 2001
> +++ linux/drivers/sound/maestro.c	Sat Jun  9 19:48:34 2001
> @@ -115,6 +115,10 @@
>   *	themselves, but we'll see.  
>   *	
>   * History
> + *  v0.15 - Jun 09 2001 - Lukas Schroeder <lukas@edeal.de>
> + *	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 <alan@redhat.com>
>   *	Add clocking= for people with seriously warped hardware
>   *  (still v0.14) Nov 10 2000 - Bartlomiej Zolnierkiewicz <bkz@linux-ide.org>
> @@ -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");
> 

Re: [patch] ess maestro, support for hardware volume control

[Alan Cox]

> 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

Send me a version against the -ac tree where the patch wasnt mangled by
the forward

Re: [patch] ess maestro, support for hardware volume control

[Alan Cox]

> 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

Re: [patch] ess maestro, support for hardware volume control

[Ben Pfaff]

Alan Cox <alan@redhat.com> writes:

> > 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

Yeah, it does--I included support for the hwv control in the
docking patch.  I used a different technique though because I
couldn't get the technique used by Lukas's patch to work properly
for me.

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.

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?

Re: [patch] ess maestro, support for hardware volume control

[Zach Brown]

> 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.  

-- 
 zach

Re: [patch] ess maestro, support for hardware volume control

[Abramo Bagnara]

Zach Brown wrote:
> 
> > 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.

It's a control separated from master volume. Often there is another
control that control if the two are linked.

Application may ask notification for controls changes (like the hw
volume one). This imply that an interrupt is related to this event.

-- 
Abramo Bagnara                       mailto:abramo@alsa-project.org

Opera Unica                          Phone: +39.546.656023
Via Emilia Interna, 140
48014 Castel Bolognese (RA) - Italy

ALSA project               http://www.alsa-project.org
It sounds good!

Re: [patch] ess maestro, support for hardware volume control

[Alan Cox]

> 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.

> 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

Re: [patch] ess maestro, support for hardware volume control

[Ben Pfaff]

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

> > 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.
> 
> > then mknod'ing a device and opening it seems to me to have a
> > nasty race condition, am I missing something here?

I think that's not what I was thinking of, here's an outline.
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:

kernel			userspace #1		userspace #2
------			------------		------------
			insmod maestro
grab a misc minor
						read /proc/misc
						mknod device
			rmmod maestro
release misc minor
			insmod something else
grab same misc minor
						open device
						read from device

Is this just something where you say "don't let that happen",
or...?

I didn't really want to use a dynamic minor but Linus doesn't
want to give out any more fixed assignments, right?

> 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

Any idea what code I should be looking at to do something like
that, drivers/usb/audio.c is a big file (is it in there or
somewhere else?).

Thanks,

Ben.
-- 
Regarding a Microsoft/Xerox agreement:
	"This is a match made in heaven. 
	 Both companies excel at copying other people's work."
--douglas@min.net <URL:http://slashdot.org/article.pl?sid=99/05/16/2211252>

Re: [patch] ess maestro, support for hardware volume control

[Alan Cox]

> 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)

Re: [patch] ess maestro, support for hardware volume control

[Riley Williams]

Hi Alan.

 >> 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)

Wasn't the basis for devfs that it would effectively do the mknod'ing
for you? Or have I completely misunderstood the situation?

Best wishes from Riley.

Re: [patch] ess maestro, support for hardware volume control

[Lukas Schroeder]

the little patch below (vs. 2.4.5-ac13) fixes a freeze which happens
when the maestro module was unloaded and a HWV button gets pushed 
generating the interrupt. so this disables all irqs of that chip 
on remove...


regards,
  lukas

--- linux-2.4.5-ac13/drivers/sound/maestro.c    Tue Jun 12 13:41:24 2001
+++ linux/drivers/sound/maestro.c       Tue Jun 12 14:13:40 2001
@@ -3575,6 +3575,12 @@
        struct ess_card *card = pci_get_drvdata(pcidev);
        int i;
 
+       /* turn off all irqs; _especially_ the one for hardware volume
+          control (bit 6), which locks the machine dead when occurring after
+          the maestro module was removed.
+        */
+       outw(0x0, card->iobase+0x18);
+
        /* XXX maybe should force stop bob, but should be all 
                stopped by _release by now */
        free_irq(card->irq, card);