[alsa-devel] [PATCH 1/2] snd-maestro3: Make hardware volume buttons an input device
While working on the sound suspend / resume problems with my laptop I noticed that the hardware volume handling code in essence just detects key presses, and then does some hardcoded modification of the master volume based on which key is pressed.
This made me think that clearly the right thing to do here is just report these keypresses to userspace as keypresses using an input device and let userspace decide what to with them.
This patch does this, getting rid of the ugly direct ac97 writes from the tasklet, the ac97lock and the need for using a tasklet in general.
As an added bonus the keys now work identical to volume keys on a (usb) keyboard with multimedia keys, providing visual feedback of the volume level change, and a better range of the volume control (with a properly configured desktop environment).
Signed-off-by: Hans de Goede hdegoede@redhat.com --- sound/pci/maestro3.c | 133 +++++++++++++++++++++++++++----------------------- 1 files changed, 72 insertions(+), 61 deletions(-)
diff --git a/sound/pci/maestro3.c b/sound/pci/maestro3.c index 0ed634a..4ee1b67 100644 --- a/sound/pci/maestro3.c +++ b/sound/pci/maestro3.c @@ -41,6 +41,7 @@ #include <linux/vmalloc.h> #include <linux/moduleparam.h> #include <linux/firmware.h> +#include <linux/input.h> #include <sound/core.h> #include <sound/info.h> #include <sound/control.h> @@ -806,6 +807,10 @@ struct m3_dma { struct snd_m3 { struct snd_card *card; +#ifdef CONFIG_INPUT + struct input_dev *input_dev; + char phys[64]; /* physical device path */ +#endif
unsigned long iobase;
@@ -844,11 +849,7 @@ struct snd_m3 { struct m3_dma *substreams;
spinlock_t reg_lock; - spinlock_t ac97_lock;
- struct snd_kcontrol *master_switch; - struct snd_kcontrol *master_volume; - struct tasklet_struct hwvol_tq; unsigned int in_suspend;
#ifdef CONFIG_PM @@ -1602,11 +1603,9 @@ static void snd_m3_update_ptr(struct snd_m3 *chip, struct m3_dma *s) (without wrap around) in response to volume button presses and then generating an interrupt. The pair of counters is stored in bits 1-3 and 5-7 of a byte wide register. The meaning of bits 0 and 4 is unknown. */ -static void snd_m3_update_hw_volume(unsigned long private_data) +static void snd_m3_update_hw_volume(struct snd_m3 *chip) { - struct snd_m3 *chip = (struct snd_m3 *) private_data; - int x, val; - unsigned long flags; + int x, key = 0;
/* Figure out which volume control button was pushed, based on differences from the default register @@ -1632,51 +1631,34 @@ static void snd_m3_update_hw_volume(unsigned long private_data) if (chip->in_suspend) return;
- if (!chip->master_switch || !chip->master_volume) +#ifdef CONFIG_INPUT + if (!chip->input_dev) return;
- /* FIXME: we can't call snd_ac97_* functions since here is in tasklet. */ - spin_lock_irqsave(&chip->ac97_lock, flags); - - val = chip->ac97->regs[AC97_MASTER_VOL]; switch (x) { case 0x88: /* The counters have not changed, yet we've received a HV interrupt. According to tests run by various people this happens when pressing the mute button. */ - val ^= 0x8000; - chip->ac97->regs[AC97_MASTER_VOL] = val; - outw(val, chip->iobase + CODEC_DATA); - outb(AC97_MASTER_VOL, chip->iobase + CODEC_COMMAND); - snd_ctl_notify(chip->card, SNDRV_CTL_EVENT_MASK_VALUE, - &chip->master_switch->id); + key = KEY_MUTE; break; case 0xaa: /* counters increased by 1 -> volume up */ - if ((val & 0x7f) > 0) - val--; - if ((val & 0x7f00) > 0) - val -= 0x0100; - chip->ac97->regs[AC97_MASTER_VOL] = val; - outw(val, chip->iobase + CODEC_DATA); - outb(AC97_MASTER_VOL, chip->iobase + CODEC_COMMAND); - snd_ctl_notify(chip->card, SNDRV_CTL_EVENT_MASK_VALUE, - &chip->master_volume->id); + key = KEY_VOLUMEUP; break; case 0x66: /* counters decreased by 1 -> volume down */ - if ((val & 0x7f) < 0x1f) - val++; - if ((val & 0x7f00) < 0x1f00) - val += 0x0100; - chip->ac97->regs[AC97_MASTER_VOL] = val; - outw(val, chip->iobase + CODEC_DATA); - outb(AC97_MASTER_VOL, chip->iobase + CODEC_COMMAND); - snd_ctl_notify(chip->card, SNDRV_CTL_EVENT_MASK_VALUE, - &chip->master_volume->id); + key = KEY_VOLUMEDOWN; break; } - spin_unlock_irqrestore(&chip->ac97_lock, flags); + + if (key) { + input_report_key(chip->input_dev, key, 1); + input_sync(chip->input_dev); + input_report_key(chip->input_dev, key, 0); + input_sync(chip->input_dev); + } +#endif }
static irqreturn_t snd_m3_interrupt(int irq, void *dev_id) @@ -1691,7 +1673,7 @@ static irqreturn_t snd_m3_interrupt(int irq, void *dev_id) return IRQ_NONE;
if (status & HV_INT_PENDING) - tasklet_schedule(&chip->hwvol_tq); + snd_m3_update_hw_volume(chip);
/* * ack an assp int if its running @@ -1957,18 +1939,14 @@ static unsigned short snd_m3_ac97_read(struct snd_ac97 *ac97, unsigned short reg) { struct snd_m3 *chip = ac97->private_data; - unsigned long flags; unsigned short data = 0xffff;
if (snd_m3_ac97_wait(chip)) goto fail; - spin_lock_irqsave(&chip->ac97_lock, flags); snd_m3_outb(chip, 0x80 | (reg & 0x7f), CODEC_COMMAND); if (snd_m3_ac97_wait(chip)) - goto fail_unlock; + goto fail; data = snd_m3_inw(chip, CODEC_DATA); -fail_unlock: - spin_unlock_irqrestore(&chip->ac97_lock, flags); fail: return data; } @@ -1977,14 +1955,11 @@ static void snd_m3_ac97_write(struct snd_ac97 *ac97, unsigned short reg, unsigned short val) { struct snd_m3 *chip = ac97->private_data; - unsigned long flags;
if (snd_m3_ac97_wait(chip)) return; - spin_lock_irqsave(&chip->ac97_lock, flags); snd_m3_outw(chip, val, CODEC_DATA); snd_m3_outb(chip, reg & 0x7f, CODEC_COMMAND); - spin_unlock_irqrestore(&chip->ac97_lock, flags); }
@@ -2091,7 +2066,6 @@ static int __devinit snd_m3_mixer(struct snd_m3 *chip) { struct snd_ac97_bus *pbus; struct snd_ac97_template ac97; - struct snd_ctl_elem_id elem_id; int err; static struct snd_ac97_bus_ops ops = { .write = snd_m3_ac97_write, @@ -2111,15 +2085,6 @@ static int __devinit snd_m3_mixer(struct snd_m3 *chip) schedule_timeout_uninterruptible(msecs_to_jiffies(100)); snd_ac97_write(chip->ac97, AC97_PCM, 0);
- memset(&elem_id, 0, sizeof(elem_id)); - elem_id.iface = SNDRV_CTL_ELEM_IFACE_MIXER; - strcpy(elem_id.name, "Master Playback Switch"); - chip->master_switch = snd_ctl_find_id(chip->card, &elem_id); - memset(&elem_id, 0, sizeof(elem_id)); - elem_id.iface = SNDRV_CTL_ELEM_IFACE_MIXER; - strcpy(elem_id.name, "Master Playback Volume"); - chip->master_volume = snd_ctl_find_id(chip->card, &elem_id); - return 0; }
@@ -2398,6 +2363,11 @@ static int snd_m3_free(struct snd_m3 *chip) struct m3_dma *s; int i;
+#ifdef CONFIG_INPUT + if (chip->input_dev) + input_unregister_device(chip->input_dev); +#endif + if (chip->substreams) { spin_lock_irq(&chip->reg_lock); for (i = 0; i < chip->num_substreams; i++) { @@ -2524,6 +2494,41 @@ static int m3_resume(struct pci_dev *pci) } #endif /* CONFIG_PM */
+#ifdef CONFIG_INPUT +static int __devinit snd_m3_input_register(struct snd_m3 *chip) +{ + struct input_dev *input_dev; + int err; + + input_dev = input_allocate_device(); + if (!input_dev) + return -ENOMEM; + + snprintf(chip->phys, sizeof(chip->phys), "pci-%s/input0", + pci_name(chip->pci)); + + input_dev->name = chip->card->driver; + input_dev->phys = chip->phys; + input_dev->id.bustype = BUS_PCI; + input_dev->id.vendor = chip->pci->vendor; + input_dev->id.product = chip->pci->device; + input_dev->dev.parent = &chip->pci->dev; + + __set_bit(EV_KEY, input_dev->evbit); + __set_bit(KEY_MUTE, input_dev->keybit); + __set_bit(KEY_VOLUMEDOWN, input_dev->keybit); + __set_bit(KEY_VOLUMEUP, input_dev->keybit); + + err = input_register_device(input_dev); + if (err) { + input_free_device(input_dev); + return err; + } + + chip->input_dev = input_dev; + return 0; +} +#endif /* CONFIG_INPUT */
/* */ @@ -2567,7 +2572,6 @@ snd_m3_create(struct snd_card *card, struct pci_dev *pci, }
spin_lock_init(&chip->reg_lock); - spin_lock_init(&chip->ac97_lock);
switch (pci->device) { case PCI_DEVICE_ID_ESS_ALLEGRO: @@ -2650,8 +2654,6 @@ snd_m3_create(struct snd_card *card, struct pci_dev *pci,
snd_m3_hv_init(chip);
- tasklet_init(&chip->hwvol_tq, snd_m3_update_hw_volume, (unsigned long)chip); - if (request_irq(pci->irq, snd_m3_interrupt, IRQF_SHARED, card->driver, chip)) { snd_printk(KERN_ERR "unable to grab IRQ %d\n", pci->irq); @@ -2682,7 +2684,16 @@ snd_m3_create(struct snd_card *card, struct pci_dev *pci,
if ((err = snd_m3_pcm(chip, 0)) < 0) return err; - + +#ifdef CONFIG_INPUT + if (chip->hv_config & HV_CTRL_ENABLE) { + err = snd_m3_input_register(chip); + if (err) + snd_printk(KERN_WARNING "Input device registration " + "failed with error %i", err); + } +#endif + snd_m3_enable_ints(chip); snd_m3_assp_continue(chip);
The hardware volume handling code in essence just detects key presses, and then does some hardcoded modification of the master volume based on which key is pressed.
Clearly the right thing to do here is just report these keypresses to userspace as keypresses using an input device and let userspace decide what to with them.
This patch does this, getting rid of the ugly direct ac97 writes from the tasklet, the ac97lock and the need for using a tasklet in general.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- sound/pci/es1968.c | 144 ++++++++++++++++++++++++++++----------------------- 1 files changed, 79 insertions(+), 65 deletions(-)
diff --git a/sound/pci/es1968.c b/sound/pci/es1968.c index a11f453..ca44ac5 100644 --- a/sound/pci/es1968.c +++ b/sound/pci/es1968.c @@ -104,6 +104,7 @@ #include <linux/gameport.h> #include <linux/moduleparam.h> #include <linux/mutex.h> +#include <linux/input.h>
#include <sound/core.h> #include <sound/pcm.h> @@ -517,14 +518,9 @@ struct es1968 {
/* ALSA Stuff */ struct snd_ac97 *ac97; - struct snd_kcontrol *master_switch; /* for h/w volume control */ - struct snd_kcontrol *master_volume; - struct snd_rawmidi *rmidi;
spinlock_t reg_lock; - spinlock_t ac97_lock; - struct tasklet_struct hwvol_tq; unsigned int in_suspend;
/* Maestro Stuff */ @@ -547,6 +543,11 @@ struct es1968 { #ifdef SUPPORT_JOYSTICK struct gameport *gameport; #endif + +#ifdef CONFIG_INPUT + struct input_dev *input_dev; + char phys[64]; /* physical device path */ +#endif };
static irqreturn_t snd_es1968_interrupt(int irq, void *dev_id); @@ -632,28 +633,23 @@ static int snd_es1968_ac97_wait_poll(struct es1968 *chip) static void snd_es1968_ac97_write(struct snd_ac97 *ac97, unsigned short reg, unsigned short val) { struct es1968 *chip = ac97->private_data; - unsigned long flags;
snd_es1968_ac97_wait(chip);
/* Write the bus */ - spin_lock_irqsave(&chip->ac97_lock, flags); outw(val, chip->io_port + ESM_AC97_DATA); /*msleep(1);*/ outb(reg, chip->io_port + ESM_AC97_INDEX); /*msleep(1);*/ - spin_unlock_irqrestore(&chip->ac97_lock, flags); }
static unsigned short snd_es1968_ac97_read(struct snd_ac97 *ac97, unsigned short reg) { u16 data = 0; struct es1968 *chip = ac97->private_data; - unsigned long flags;
snd_es1968_ac97_wait(chip);
- spin_lock_irqsave(&chip->ac97_lock, flags); outb(reg | 0x80, chip->io_port + ESM_AC97_INDEX); /*msleep(1);*/
@@ -661,7 +657,6 @@ static unsigned short snd_es1968_ac97_read(struct snd_ac97 *ac97, unsigned short data = inw(chip->io_port + ESM_AC97_DATA); /*msleep(1);*/ } - spin_unlock_irqrestore(&chip->ac97_lock, flags);
return data; } @@ -1874,13 +1869,13 @@ static void snd_es1968_update_pcm(struct es1968 *chip, struct esschan *es) } }
-/* - */ -static void es1968_update_hw_volume(unsigned long private_data) +/* The hardware volume works by incrementing / decrementing 2 counters + (without wrap around) in response to volume button presses and then + generating an interrupt. The pair of counters is stored in bits 1-3 and 5-7 + of a byte wide register. The meaning of bits 0 and 4 is unknown. */ +static void es1968_update_hw_volume(struct es1968 *chip) { - struct es1968 *chip = (struct es1968 *) private_data; - int x, val; - unsigned long flags; + int x, key = 0;
/* Figure out which volume control button was pushed, based on differences from the default register @@ -1895,48 +1890,34 @@ static void es1968_update_hw_volume(unsigned long private_data) if (chip->in_suspend) return;
- if (! chip->master_switch || ! chip->master_volume) +#ifdef CONFIG_INPUT + if (!chip->input_dev) return;
- /* FIXME: we can't call snd_ac97_* functions since here is in tasklet. */ - spin_lock_irqsave(&chip->ac97_lock, flags); - val = chip->ac97->regs[AC97_MASTER]; switch (x) { case 0x88: - /* mute */ - val ^= 0x8000; - chip->ac97->regs[AC97_MASTER] = val; - outw(val, chip->io_port + ESM_AC97_DATA); - outb(AC97_MASTER, chip->io_port + ESM_AC97_INDEX); - snd_ctl_notify(chip->card, SNDRV_CTL_EVENT_MASK_VALUE, - &chip->master_switch->id); + /* The counters have not changed, yet we've received a HV + interrupt. According to tests run by various people this + happens when pressing the mute button. */ + key = KEY_MUTE; break; case 0xaa: - /* volume up */ - if ((val & 0x7f) > 0) - val--; - if ((val & 0x7f00) > 0) - val -= 0x0100; - chip->ac97->regs[AC97_MASTER] = val; - outw(val, chip->io_port + ESM_AC97_DATA); - outb(AC97_MASTER, chip->io_port + ESM_AC97_INDEX); - snd_ctl_notify(chip->card, SNDRV_CTL_EVENT_MASK_VALUE, - &chip->master_volume->id); + /* counters increased by 1 -> volume up */ + key = KEY_VOLUMEUP; break; case 0x66: - /* volume down */ - if ((val & 0x7f) < 0x1f) - val++; - if ((val & 0x7f00) < 0x1f00) - val += 0x0100; - chip->ac97->regs[AC97_MASTER] = val; - outw(val, chip->io_port + ESM_AC97_DATA); - outb(AC97_MASTER, chip->io_port + ESM_AC97_INDEX); - snd_ctl_notify(chip->card, SNDRV_CTL_EVENT_MASK_VALUE, - &chip->master_volume->id); + /* counters decreased by 1 -> volume down */ + key = KEY_VOLUMEDOWN; break; } - spin_unlock_irqrestore(&chip->ac97_lock, flags); + + if (key) { + input_report_key(chip->input_dev, key, 1); + input_sync(chip->input_dev); + input_report_key(chip->input_dev, key, 0); + input_sync(chip->input_dev); + } +#endif }
/* @@ -1953,7 +1934,7 @@ static irqreturn_t snd_es1968_interrupt(int irq, void *dev_id) outw(inw(chip->io_port + 4) & 1, chip->io_port + 4);
if (event & ESM_HWVOL_IRQ) - tasklet_schedule(&chip->hwvol_tq); /* we'll do this later */ + es1968_update_hw_volume(chip);
/* else ack 'em all, i imagine */ outb(0xFF, chip->io_port + 0x1A); @@ -1993,7 +1974,6 @@ snd_es1968_mixer(struct es1968 *chip) { struct snd_ac97_bus *pbus; struct snd_ac97_template ac97; - struct snd_ctl_elem_id elem_id; int err; static struct snd_ac97_bus_ops ops = { .write = snd_es1968_ac97_write, @@ -2009,16 +1989,6 @@ snd_es1968_mixer(struct es1968 *chip) if ((err = snd_ac97_mixer(pbus, &ac97, &chip->ac97)) < 0) return err;
- /* attach master switch / volumes for h/w volume control */ - memset(&elem_id, 0, sizeof(elem_id)); - elem_id.iface = SNDRV_CTL_ELEM_IFACE_MIXER; - strcpy(elem_id.name, "Master Playback Switch"); - chip->master_switch = snd_ctl_find_id(chip->card, &elem_id); - memset(&elem_id, 0, sizeof(elem_id)); - elem_id.iface = SNDRV_CTL_ELEM_IFACE_MIXER; - strcpy(elem_id.name, "Master Playback Volume"); - chip->master_volume = snd_ctl_find_id(chip->card, &elem_id); - return 0; }
@@ -2474,8 +2444,49 @@ static inline int snd_es1968_create_gameport(struct es1968 *chip, int dev) { ret static inline void snd_es1968_free_gameport(struct es1968 *chip) { } #endif
+#ifdef CONFIG_INPUT +static int __devinit snd_es1968_input_register(struct es1968 *chip) +{ + struct input_dev *input_dev; + int err; + + input_dev = input_allocate_device(); + if (!input_dev) + return -ENOMEM; + + snprintf(chip->phys, sizeof(chip->phys), "pci-%s/input0", + pci_name(chip->pci)); + + input_dev->name = chip->card->driver; + input_dev->phys = chip->phys; + input_dev->id.bustype = BUS_PCI; + input_dev->id.vendor = chip->pci->vendor; + input_dev->id.product = chip->pci->device; + input_dev->dev.parent = &chip->pci->dev; + + __set_bit(EV_KEY, input_dev->evbit); + __set_bit(KEY_MUTE, input_dev->keybit); + __set_bit(KEY_VOLUMEDOWN, input_dev->keybit); + __set_bit(KEY_VOLUMEUP, input_dev->keybit); + + err = input_register_device(input_dev); + if (err) { + input_free_device(input_dev); + return err; + } + + chip->input_dev = input_dev; + return 0; +} +#endif /* CONFIG_INPUT */ + static int snd_es1968_free(struct es1968 *chip) { +#ifdef CONFIG_INPUT + if (chip->input_dev) + input_unregister_device(chip->input_dev); +#endif + if (chip->io_port) { if (chip->irq >= 0) synchronize_irq(chip->irq); @@ -2486,8 +2497,6 @@ static int snd_es1968_free(struct es1968 *chip) if (chip->irq >= 0) free_irq(chip->irq, chip); snd_es1968_free_gameport(chip); - chip->master_switch = NULL; - chip->master_volume = NULL; pci_release_regions(chip->pci); pci_disable_device(chip->pci); kfree(chip); @@ -2558,9 +2567,7 @@ static int __devinit snd_es1968_create(struct snd_card *card, spin_lock_init(&chip->substream_lock); INIT_LIST_HEAD(&chip->buf_list); INIT_LIST_HEAD(&chip->substream_list); - spin_lock_init(&chip->ac97_lock); mutex_init(&chip->memory_mutex); - tasklet_init(&chip->hwvol_tq, es1968_update_hw_volume, (unsigned long)chip); chip->card = card; chip->pci = pci; chip->irq = -1; @@ -2713,6 +2720,13 @@ static int __devinit snd_es1968_probe(struct pci_dev *pci,
snd_es1968_create_gameport(chip, dev);
+#ifdef CONFIG_INPUT + err = snd_es1968_input_register(chip); + if (err) + snd_printk(KERN_WARNING "Input device registration " + "failed with error %i", err); +#endif + snd_es1968_start_irq(chip);
chip->clock = clock[dev];
At Thu, 22 Apr 2010 04:37:04 -0400, Hans de Goede wrote:
While working on the sound suspend / resume problems with my laptop I noticed that the hardware volume handling code in essence just detects key presses, and then does some hardcoded modification of the master volume based on which key is pressed.
This made me think that clearly the right thing to do here is just report these keypresses to userspace as keypresses using an input device and let userspace decide what to with them.
This patch does this, getting rid of the ugly direct ac97 writes from the tasklet, the ac97lock and the need for using a tasklet in general.
As an added bonus the keys now work identical to volume keys on a (usb) keyboard with multimedia keys, providing visual feedback of the volume level change, and a better range of the volume control (with a properly configured desktop environment).
I like the basic idea. However, two points to be fixed:
- We need a kconfig to keep the old behavior. We are not allowed to give any regression in general.
- CONFIG_INPUT is tristate. It can be a module, thus the dependency is a bit messy. Imagine you want to build snd-maestro3 into kernel while CONFIG_INPUT=m is given.
A hack to avoid to avoid the dependency is to create a bool Kconfig to specify whether input feature is added or not; this is anyway needed for the first reason above. Suppose it CONFIG_SND_MAESTRO3_INPUT.
Then it looks like:
config SND_MAESTRO3_INPUT bool "enable input device for maestro3 volume switches" depends on SND_MAESTRO3 depends on INPUT=y || INPUT=SND_MAESTRO3 help ....
Then you can use "#ifdef CONFIG_SND_MAESTRO3_INPUT" to determine compile condition. If this isn't defined, keep the old behavior, i.e. controlling ac97 registers directly.
thanks,
Takashi
Hi,
On 04/22/2010 05:02 PM, Takashi Iwai wrote:
At Thu, 22 Apr 2010 04:37:04 -0400, Hans de Goede wrote:
While working on the sound suspend / resume problems with my laptop I noticed that the hardware volume handling code in essence just detects key presses, and then does some hardcoded modification of the master volume based on which key is pressed.
This made me think that clearly the right thing to do here is just report these keypresses to userspace as keypresses using an input device and let userspace decide what to with them.
This patch does this, getting rid of the ugly direct ac97 writes from the tasklet, the ac97lock and the need for using a tasklet in general.
As an added bonus the keys now work identical to volume keys on a (usb) keyboard with multimedia keys, providing visual feedback of the volume level change, and a better range of the volume control (with a properly configured desktop environment).
I like the basic idea. However, two points to be fixed:
- We need a kconfig to keep the old behavior. We are not allowed to give any regression in general.
Yes, I already thought about this, and sort of expected this comment, but I opted to start with the straight forward patch.
- CONFIG_INPUT is tristate. It can be a module, thus the dependency is a bit messy. Imagine you want to build snd-maestro3 into kernel while CONFIG_INPUT=m is given.
Ugh, I would say just don't do that, but I understand that that is not an acceptable answer, and I see you've already provided a solution, thanks.
A hack to avoid to avoid the dependency is to create a bool Kconfig to specify whether input feature is added or not; this is anyway needed for the first reason above. Suppose it CONFIG_SND_MAESTRO3_INPUT.
Then it looks like:
config SND_MAESTRO3_INPUT bool "enable input device for maestro3 volume switches" depends on SND_MAESTRO3 depends on INPUT=y || INPUT=SND_MAESTRO3 help ....
Then you can use "#ifdef CONFIG_SND_MAESTRO3_INPUT" to determine compile condition. If this isn't defined, keep the old behavior, i.e. controlling ac97 registers directly.
Ok, I'll respin this patches taking the above comments into account, I'll do this either tomorrow or sometime next week.
Regards,
Hans
participants (2)
-
Hans de Goede -
Takashi Iwai