At Fri, 16 Oct 2009 21:52:48 +0200,
LCID Fire wrote:
>
> Signed-off-by: Andreas Bergmeier <andreas.bergmeier@xxxxxxx>
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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
