Re: [PATCH] serato phono patch

[Takashi Iwai <tiwai@suse.de>]

At Mon, 12 Jan 2009 20:07:22 +0100,
LCID Fire wrote:
> 
> Add serato input phono switch

Thank you for the patch.

Could you give a more detailed patch description?
Also, don't forget to give your sign-off.  Otherwise I can't merge
it to the upstream for such an amount of patch.

Also, the patch embedded in your post seems broken due to line breaks
likely by your MUA.  Please fix MUA setting or use an attachment if
difficult to fix.

Some quick review comments below...

> diff --git a/sound/usb/usbaudio.c b/sound/usb/usbaudio.c
> index b8cfb7c..66fc115 100644
> --- a/sound/usb/usbaudio.c
> +++ b/sound/usb/usbaudio.c
> @@ -2859,7 +2859,7 @@ static int snd_usb_create_streams(struct 
> snd_usb_audio *chip, int ctrlif)
>      /* find audiocontrol interface */
>      host_iface = &usb_ifnum_to_if(dev, ctrlif)->altsetting[0];
>      if (!(p1 = snd_usb_find_csint_desc(host_iface->extra, 
> host_iface->extralen, NULL, HEADER))) {
> -        snd_printk(KERN_ERR "cannot find HEADER\n");
> +        snd_printk(KERN_ERR "cannot find HEADER for %d in %s\n", 
> ctrlif, host_iface->extra);

Is host_iface->extra really NULL-terminated, i.e. safe to use as a
string?

> diff --git a/sound/usb/usbmixer.c b/sound/usb/usbmixer.c
> index 89c63d0..8e936d1 100644
> --- a/sound/usb/usbmixer.c
> +++ b/sound/usb/usbmixer.c
> @@ -2012,6 +2012,233 @@ static void snd_audigy2nx_proc_read(struct 
> snd_info_entry *entry,
>      }
>  }
>  
> +void snd_usb_cleanup_interrupt_urb(struct urb* pUrb)

Make it static.

> +{
> +    if(pUrb->transfer_buffer_length > 0)
> +        kfree(pUrb->transfer_buffer);
> +   
> +    if(pUrb->context)
> +        kfree(pUrb->context);

No need of a NULL-check for kfree.

> +
> +    usb_free_urb(pUrb);
> +}
> +
> +/* Wrapper for setting and submitting the interrupt urb().*/
> +int snd_usb_interrupt_trans(struct usb_device *dev, unsigned int pipe, 
> void *data,
> +             __u16 size, usb_complete_t callback)

Make it static.

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

No need to initialize this.

> +
> +    if (size > 0) {
> +        buf = kmemdup(data, size, GFP_KERNEL);
> +        if (!buf)
> +            return -ENOMEM;
> +    }
> +
> +    pUrb = usb_alloc_urb(1/*int iso packets*/, GFP_KERNEL);

Missing NULL check.

> +    /*TODO: Remove hardcoded 4*/
> +    usb_fill_int_urb(pUrb, dev, pipe, buf, size, callback, NULL, 4);
> +
> +    err = usb_submit_urb(pUrb, GFP_KERNEL);
> +   
> +    return err;

Possible memory leak when error?

> +static int snd_sl_phono_put(struct snd_kcontrol *kcontrol, struct 
> snd_ctl_elem_value *ucontrol)
(snip)
> +
> +    err = snd_usb_interrupt_trans(mixer->chip->dev,
> +                usb_rcvbulkpipe(mixer->chip->dev, 0x83),
> +                buffer, bufferSize, snd_sl_phono_receive);

No error check?

Also, there are a few coding-style issues.
I'd suggest you to run scripts/checkpatch.pl once.


thanks,

Takashi