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

["Ville Syrjälä" <syrjala@sci.fi>]

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/