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