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

[Hans de Goede <hdegoede@redhat.com>]

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