[alsa-devel] [Fwd: Scratch Live amplifier switch]

[Takashi Iwai]

At Fri, 16 Oct 2009 21:52:48 +0200,
LCID Fire wrote:
> 
> Signed-off-by: Andreas Bergmeier <andreas.bergmeier at gmx.net>

Thanks for the renewed patch.  Below is a quick review.


> diff --git a/sound/usb/usbmixer.c b/sound/usb/usbmixer.c
> index 00397c8..532d02d 100644
> --- a/sound/usb/usbmixer.c
> +++ b/sound/usb/usbmixer.c
> @@ -2013,6 +2013,281 @@ static void snd_audigy2nx_proc_read(struct snd_info_entry *entry,
>  	}
>  }
>  
> +static void snd_usb_cleanup_interrupt_urb(struct urb *pUrb)

Avoid Windows-style variable names as much as possible.

> +{
> +	if (pUrb->transfer_buffer_length > 0)
> +		kfree(pUrb->transfer_buffer);
> +
> +	kfree(pUrb->context);
> +
> +	usb_free_urb(pUrb);
> +}
> +
> +/* Wrapper for setting and submitting the interrupt urb().*/
> +static int snd_usb_interrupt_trans(struct usb_device *dev,
> +	unsigned int pipe, void *data, __u16 size, usb_complete_t callback)

No need to use __u16.  Here it can be simply unsigned int.

> +{
> +	int err = 0;
> +	void *buf = NULL;
> +	struct urb *pUrb = NULL;

No need for initializations of all of these.

(snip)
> +/* gets called multiple times! */
> +static int snd_sl_controls_create(struct usb_mixer_interface *mixer)
> +{
> +	int i, err;
> +
> +	for (i = 0; i < ARRAY_SIZE(snd_sl_controls); ++i) {
> +		struct snd_kcontrol *kcontrol =
> +			snd_ctl_new1(&snd_sl_controls[i], mixer);

Missing NULL check here.

> +
> +		/* Since we only need one control and the routine
> +		 * is called multiple times we have to ignore all
> +		 * attempts to attach controls multiple times*/

Hm, why is it called so many times?
Can't it be limited with a check of iface or altsetting?

> +		if (snd_ctl_find_id(mixer->chip->card, &kcontrol->id)) {
> +			/* we found the control - it is already present
> +			 * so just continue*/
> +			snd_ctl_free_one(kcontrol);
> +			continue;
> +		}
> +
> +		/* add frees the control if on err < 0! */
> +		err = snd_ctl_add(mixer->chip->card,
> +			  kcontrol);
> +		if (err < 0)
> +			return err;
> +
> +		/* Make sure device is set to default */
> +		err = snd_sl_phono_set(kcontrol, kcontrol->private_value);
> +
> +		if (err < 0) {
> +			snd_ctl_free_one(kcontrol);

Don't free this after adding via snd_ctl_add().  Just keep it.
The final destructor will clean up everything eventually.


thanks,

Takashi