[PATCH 1/4] snd-meastro3: Add amp_gpio quirk for Compaq EVO N600C

[PATCH 1/4] snd-meastro3: Add amp_gpio quirk for Compaq EVO N600C

[Hans de Goede]

Without this quirk sound stops working after suspend resume. With this quirk,
one still needs to manually unmute the master volume control after a suspend /
/ resume cycle. That is fixed in another patch in this set.

Note that this patch was submitted to the alsa bug tracker a long time ago:
https://bugtrack.alsa-project.org/alsa-bug/view.php?id=4319

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 sound/pci/maestro3.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/sound/pci/maestro3.c b/sound/pci/maestro3.c
index 75283fb..491a2fc 100644
--- a/sound/pci/maestro3.c
+++ b/sound/pci/maestro3.c
@@ -884,6 +884,7 @@ static struct pci_device_id snd_m3_ids[] = {
 MODULE_DEVICE_TABLE(pci, snd_m3_ids);
 
 static struct snd_pci_quirk m3_amp_quirk_list[] __devinitdata = {
+	SND_PCI_QUIRK(0x0E11, 0x0094, "Compaq Evo N600c", 0x0c),
 	SND_PCI_QUIRK(0x10f7, 0x833e, "Panasonic CF-28", 0x0d),
 	SND_PCI_QUIRK(0x10f7, 0x833d, "Panasonic CF-72", 0x0d),
 	SND_PCI_QUIRK(0x1033, 0x80f1, "NEC LM800J/7", 0x03),
-- 
1.7.0.1

[PATCH 2/4] snd-meastro3: Document hardware volume control a bit

[Hans de Goede]

While working on a fix for the volume being muted on the allegro in my
Compaq EVO N600C after suspend, I've learned a few things about the hardware
volume control worth documenting. The actual fix for the suspend / resume
issue is in the next patch in this set.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 sound/pci/maestro3.c |   22 ++++++++++++++++++----
 1 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/sound/pci/maestro3.c b/sound/pci/maestro3.c
index 491a2fc..c6593cf 100644
--- a/sound/pci/maestro3.c
+++ b/sound/pci/maestro3.c
@@ -1597,6 +1597,10 @@ static void snd_m3_update_ptr(struct snd_m3 *chip, struct m3_dma *s)
 	}
 }
 
+/* The m3's 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 snd_m3_update_hw_volume(unsigned long private_data)
 {
 	struct snd_m3 *chip = (struct snd_m3 *) private_data;
@@ -1608,7 +1612,15 @@ static void snd_m3_update_hw_volume(unsigned long private_data)
 	   values. */
 	x = inb(chip->iobase + SHADOW_MIX_REG_VOICE) & 0xee;
 
-	/* Reset the volume control registers. */
+	/* Reset the volume counters to 4. Tests on the allegro integrated
+	   into a Compaq N600C laptop, have revealed that:
+	   1) Writing any value will result in the 2 counters being reset to
+	      4 so writing 0x88 is not strictly necessary
+	   2) Writing to any of the 4 involved registers will reset all 4
+	      of them (and reading them always returns the same value for all
+	      of them)
+	   It could be that a maestro deviates from this, so leave the code
+	   as is. */
 	outb(0x88, chip->iobase + SHADOW_MIX_REG_VOICE);
 	outb(0x88, chip->iobase + HW_VOL_COUNTER_VOICE);
 	outb(0x88, chip->iobase + SHADOW_MIX_REG_MASTER);
@@ -1623,7 +1635,9 @@ static void snd_m3_update_hw_volume(unsigned long private_data)
 	val = chip->ac97->regs[AC97_MASTER_VOL];
 	switch (x) {
 	case 0x88:
-		/* mute */
+		/* 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);
@@ -1632,7 +1646,7 @@ static void snd_m3_update_hw_volume(unsigned long private_data)
 			       &chip->master_switch->id);
 		break;
 	case 0xaa:
-		/* volume up */
+		/* counters increased by 1 -> volume up */
 		if ((val & 0x7f) > 0)
 			val--;
 		if ((val & 0x7f00) > 0)
@@ -1644,7 +1658,7 @@ static void snd_m3_update_hw_volume(unsigned long private_data)
 			       &chip->master_volume->id);
 		break;
 	case 0x66:
-		/* volume down */
+		/* counters decreased by 1 -> volume down */
 		if ((val & 0x7f) < 0x1f)
 			val++;
 		if ((val & 0x7f00) < 0x1f00)
-- 
1.7.0.1

[PATCH 3/4] snd-meastro3: Ignore spurious HV interrupts during suspend / resume

[Hans de Goede]

Ignore spurious HV interrupts during suspend / resume, this avoids
mistaking them for a mute button press. This is not very pretty but
it seems the only way to fix the master volume control gets muted
after suspend issue I'm seeing. Note that the es1968 driver is doing
exactly the same.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 sound/pci/maestro3.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/sound/pci/maestro3.c b/sound/pci/maestro3.c
index c6593cf..0ed634a 100644
--- a/sound/pci/maestro3.c
+++ b/sound/pci/maestro3.c
@@ -849,6 +849,7 @@ struct snd_m3 {
 	struct snd_kcontrol *master_switch;
 	struct snd_kcontrol *master_volume;
 	struct tasklet_struct hwvol_tq;
+	unsigned int in_suspend;
 
 #ifdef CONFIG_PM
 	u16 *suspend_mem;
@@ -1626,6 +1627,11 @@ static void snd_m3_update_hw_volume(unsigned long private_data)
 	outb(0x88, chip->iobase + SHADOW_MIX_REG_MASTER);
 	outb(0x88, chip->iobase + HW_VOL_COUNTER_MASTER);
 
+	/* Ignore spurious HV interrupts during suspend / resume, this avoids
+	   mistaking them for a mute button press. */
+	if (chip->in_suspend)
+		return;
+
 	if (!chip->master_switch || !chip->master_volume)
 		return;
 
@@ -2439,6 +2445,7 @@ static int m3_suspend(struct pci_dev *pci, pm_message_t state)
 	if (chip->suspend_mem == NULL)
 		return 0;
 
+	chip->in_suspend = 1;
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
 	snd_pcm_suspend_all(chip->pcm);
 	snd_ac97_suspend(chip->ac97);
@@ -2512,6 +2519,7 @@ static int m3_resume(struct pci_dev *pci)
 	snd_m3_hv_init(chip);
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D0);
+	chip->in_suspend = 0;
 	return 0;
 }
 #endif /* CONFIG_PM */
-- 
1.7.0.1

[PATCH 4/4] snd-maestro3: Make hardware volume buttons an input device

[Hans de Goede]

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 |  134 +++++++++++++++++++++++++++-----------------------
 1 files changed, 73 insertions(+), 61 deletions(-)

diff --git a/sound/pci/maestro3.c b/sound/pci/maestro3.c
index 0ed634a..135240d 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,42 @@ 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;
+
+	input_dev->evbit[0] = BIT_MASK(EV_KEY);
+	input_dev->keybit[BIT_WORD(KEY_MUTE)] |= BIT_MASK(KEY_MUTE);
+	input_dev->keybit[BIT_WORD(KEY_VOLUMEDOWN)] |= BIT_MASK(KEY_VOLUMEDOWN);
+	input_dev->keybit[BIT_WORD(KEY_VOLUMEUP)] |= BIT_MASK(KEY_VOLUMEUP);
+
+	err = input_register_device(input_dev);
+	if (err) {
+		input_dev->dev.parent = NULL;
+		input_free_device(input_dev);
+	} else {
+		chip->input_dev = input_dev;
+	}
+
+	return err;
+}
+#endif /* CONFIG_INPUT */
 
 /*
  */
@@ -2567,7 +2573,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 +2655,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 +2685,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);
 
-- 
1.7.0.1

Re: [PATCH 4/4] snd-maestro3: Make hardware volume buttons an input device

[Ville Syrjälä]

On Wed, Apr 21, 2010 at 11:04:09AM -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 it. The maestro2 code is nearly identical. Any chance you'd
give it the same treatment? I should be able to dig up a few laptops to
test both drivers if necessary.

<snip>
> @@ -2524,6 +2494,42 @@ 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));

What's the proper format of phys? I see gameport stuff uses
pci%s/gameport0, ir stuff uses pci-%s/ir0. I can't immediately find any
other pci input things.

> +
> +	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;
> +
> +	input_dev->evbit[0] = BIT_MASK(EV_KEY);
> +	input_dev->keybit[BIT_WORD(KEY_MUTE)] |= BIT_MASK(KEY_MUTE);
> +	input_dev->keybit[BIT_WORD(KEY_VOLUMEDOWN)] |= BIT_MASK(KEY_VOLUMEDOWN);
> +	input_dev->keybit[BIT_WORD(KEY_VOLUMEUP)] |= BIT_MASK(KEY_VOLUMEUP);

__set_bit() perhaps

> +
> +	err = input_register_device(input_dev);
> +	if (err) {
> +		input_dev->dev.parent = NULL;

Is this nullification needed?

> +		input_free_device(input_dev);
> +	} else {
> +		chip->input_dev = input_dev;
> +	}
> +
> +	return err;
> +}
> +#endif /* CONFIG_INPUT */
>  
>  /*
>   */

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

Re: [PATCH 4/4] snd-maestro3: Make hardware volume buttons an input device

[Hans de Goede]

Hi,

On 04/21/2010 06:05 PM, Ville Syrjälä wrote:
> On Wed, Apr 21, 2010 at 11:04:09AM -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 it. The maestro2 code is nearly identical. Any chance you'd
> give it the same treatment?

Yes, I already noticed it was nearly identical, but I didn't work
on it as I have no hardware to test any changes ...

> I should be able to dig up a few laptops to
> test both drivers if necessary.

I'll do an identical patch for the meastro2, if you could test that that would
be great.

>
> <snip>
>> @@ -2524,6 +2494,42 @@ 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));
>
> What's the proper format of phys? I see gameport stuff uses
> pci%s/gameport0, ir stuff uses pci-%s/ir0. I can't immediately find any
> other pci input things.
>

I've no idea, I took the pci-%s/ part from other pci drivers registering
input devices and the input0 part is based on doing:
cat /sys/class/input/input?/phys

On my system which yields a string ending in input0 for almost all
input devices.

>> +
>> +	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;
>> +
>> +	input_dev->evbit[0] = BIT_MASK(EV_KEY);
>> +	input_dev->keybit[BIT_WORD(KEY_MUTE)] |= BIT_MASK(KEY_MUTE);
>> +	input_dev->keybit[BIT_WORD(KEY_VOLUMEDOWN)] |= BIT_MASK(KEY_VOLUMEDOWN);
>> +	input_dev->keybit[BIT_WORD(KEY_VOLUMEUP)] |= BIT_MASK(KEY_VOLUMEUP);
>
> __set_bit() perhaps

Ah yes, good one.

>
>> +
>> +	err = input_register_device(input_dev);
>> +	if (err) {
>> +		input_dev->dev.parent = NULL;
>
> Is this nullification needed?
>

I copy pasted this from gspca's input handling code, as I'm
familiar with the gspca code, but looking at what
input_free_device actually does: no.

I'll respin the patch with these 2 issues fixed and repost it
+ an identical one for the es1968 driver.

>> +		input_free_device(input_dev);
>> +	} else {
>> +		chip->input_dev = input_dev;
>> +	}
>> +
>> +	return err;
>> +}
>> +#endif /* CONFIG_INPUT */
>>
>>   /*
>>    */
>

Regards,

Hans

Re: [PATCH 1/4] snd-meastro3: Add amp_gpio quirk for Compaq EVO N600C

[Takashi Iwai]

At Wed, 21 Apr 2010 11:04:06 -0400,
Hans de Goede wrote:
> 
> Without this quirk sound stops working after suspend resume. With this quirk,
> one still needs to manually unmute the master volume control after a suspend /
> / resume cycle. That is fixed in another patch in this set.
> 
> Note that this patch was submitted to the alsa bug tracker a long time ago:
> https://bugtrack.alsa-project.org/alsa-bug/view.php?id=4319
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Thanks, applied patches 1-3.
Patch 4 needs a bit more attention.  Will follow up in another thread.


Takashi

> ---
>  sound/pci/maestro3.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/sound/pci/maestro3.c b/sound/pci/maestro3.c
> index 75283fb..491a2fc 100644
> --- a/sound/pci/maestro3.c
> +++ b/sound/pci/maestro3.c
> @@ -884,6 +884,7 @@ static struct pci_device_id snd_m3_ids[] = {
>  MODULE_DEVICE_TABLE(pci, snd_m3_ids);
>  
>  static struct snd_pci_quirk m3_amp_quirk_list[] __devinitdata = {
> +	SND_PCI_QUIRK(0x0E11, 0x0094, "Compaq Evo N600c", 0x0c),
>  	SND_PCI_QUIRK(0x10f7, 0x833e, "Panasonic CF-28", 0x0d),
>  	SND_PCI_QUIRK(0x10f7, 0x833d, "Panasonic CF-72", 0x0d),
>  	SND_PCI_QUIRK(0x1033, 0x80f1, "NEC LM800J/7", 0x03),
> -- 
> 1.7.0.1
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH 4/4] snd-maestro3: Make hardware volume buttons an input device

[Ville Syrjälä]

On Thu, Apr 22, 2010 at 09:57:42AM +0200, Hans de Goede wrote:
> On 04/21/2010 06:05 PM, Ville Syrjälä wrote:
> > On Wed, Apr 21, 2010 at 11:04:09AM -0400, Hans de Goede wrote:
> >> @@ -2524,6 +2494,42 @@ 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));
> >
> > What's the proper format of phys? I see gameport stuff uses
> > pci%s/gameport0, ir stuff uses pci-%s/ir0. I can't immediately find any
> > other pci input things.
> >
> 
> I've no idea, I took the pci-%s/ part from other pci drivers registering
> input devices and the input0 part is based on doing:
> cat /sys/class/input/input?/phys
> 
> On my system which yields a string ending in input0 for almost all
> input devices.

Same for me. Unfortunately none start with "pci". Adding Dmitry and
linux-input in cc. Dmitry, any official statement about this phys
string?

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

Re: [alsa-devel] [PATCH 4/4] snd-maestro3: Make hardware volume buttons an input device

[Dmitry Torokhov]

On Thursday 22 April 2010 08:11:12 am Ville Syrjälä wrote:
> On Thu, Apr 22, 2010 at 09:57:42AM +0200, Hans de Goede wrote:
> > On 04/21/2010 06:05 PM, Ville Syrjälä wrote:
> > > On Wed, Apr 21, 2010 at 11:04:09AM -0400, Hans de Goede wrote:
> > >> @@ -2524,6 +2494,42 @@ 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));
> > > 
> > > What's the proper format of phys? I see gameport stuff uses
> > > pci%s/gameport0, ir stuff uses pci-%s/ir0. I can't immediately find any
> > > other pci input things.
> > 
> > I've no idea, I took the pci-%s/ part from other pci drivers registering
> > input devices and the input0 part is based on doing:
> > cat /sys/class/input/input?/phys
> > 
> > On my system which yields a string ending in input0 for almost all
> > input devices.
> 
> Same for me. Unfortunately none start with "pci". Adding Dmitry and
> linux-input in cc. Dmitry, any official statement about this phys
> string?

Phys can be pretty much anything, it is supposed to aid in identifying
particular device by physical connection. DVB guys like phys ending in
"irX", everywhere else we put "inputX" (normally input0 but if there
several input devices on the same physical device, like some USB HID
devices, there could be xxx/input1, xxx/intput2 and so on).

The piece in the quoted patch segment seems to be just fine.

Hope this helps.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH 4/4] snd-maestro3: Make hardware volume buttons an input device

[Ville Syrjälä]

On Thu, Apr 22, 2010 at 09:59:00AM -0700, Dmitry Torokhov wrote:
> On Thursday 22 April 2010 08:11:12 am Ville Syrjälä wrote:
> > On Thu, Apr 22, 2010 at 09:57:42AM +0200, Hans de Goede wrote:
> > > On 04/21/2010 06:05 PM, Ville Syrjälä wrote:
> > > > On Wed, Apr 21, 2010 at 11:04:09AM -0400, Hans de Goede wrote:
> > > >> @@ -2524,6 +2494,42 @@ 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));
> > > > 
> > > > What's the proper format of phys? I see gameport stuff uses
> > > > pci%s/gameport0, ir stuff uses pci-%s/ir0. I can't immediately find any
> > > > other pci input things.
> > > 
> > > I've no idea, I took the pci-%s/ part from other pci drivers registering
> > > input devices and the input0 part is based on doing:
> > > cat /sys/class/input/input?/phys
> > > 
> > > On my system which yields a string ending in input0 for almost all
> > > input devices.
> > 
> > Same for me. Unfortunately none start with "pci". Adding Dmitry and
> > linux-input in cc. Dmitry, any official statement about this phys
> > string?
> 
> Phys can be pretty much anything, it is supposed to aid in identifying
> particular device by physical connection. DVB guys like phys ending in
> "irX", everywhere else we put "inputX" (normally input0 but if there
> several input devices on the same physical device, like some USB HID
> devices, there could be xxx/input1, xxx/intput2 and so on).

OK. I was mainly worried that perhaps some tool might use it and would
need some specific format. In this case the dash between the bus type
and the device name caught my eye as some drivers have it and some
don't.

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html