Re: [PATCH] ALSA: usb-audio - Capture and duplex support for Digidesign Mbox 1 sound card.

[Daniel Mack <daniel@zonque.org>]

Hi Damien,

On 03/29/2014 06:35 AM, Damien Zammit wrote:
> A few revisions of this patch have been made.  This patch provides support for usb cards
> which have their streams on multiple endpoints within the same interface.
> In particular, it provides duplex mode support for Digidesign Mbox 1.
> 
> I followed ALSA dev team's suggestion of making it work for N endpoints
> rather than hardcoding it to two, and improved error handling has been provided.
> An extra parameter has been added as requested to the usbaudio quirk struct 'epmulti'
> to give the number of endpoints within a multi interface.
> 

A few style nits below, which you can fix for the next version. Most
important, the error unwinder should be more in-line with other code in
the kernel.

> diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
> index 7c57f22..affbced 100644
> --- a/sound/usb/quirks.c
> +++ b/sound/usb/quirks.c
> @@ -180,6 +180,84 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
>  	return 0;
>  }
>  
> +/*
> + * create N streams for an interface without proper descriptors but with multiple endpoints
> + */
> +static int create_fixed_multi_stream_quirk(struct snd_usb_audio *chip,
> +				     struct usb_interface *iface,
> +				     struct usb_driver *driver,
> +				     const struct snd_usb_audio_quirk *quirk)
> +{
> +	struct audioformat **fp;
> +	struct usb_host_interface *alts = NULL;
> +	int *stream;
> +	int err, i;
> +	unsigned *rate_table = NULL;
> +	int rates_found;
> +	rates_found = 0;
> +
> +	stream = (int*) kmalloc(sizeof(int) * quirk->epmulti, GFP_KERNEL);

No need to cast the result of k[mzc]alloc. Same for other locations below.

> +	if (!stream)
> +		goto err_mem;

err = -ENOMEM;
goto exit_err;

> +	fp = (struct audioformat**) kmalloc(sizeof((const struct audioformat*)quirk->data) * quirk->epmulti, GFP_KERNEL);
> +	if (!(fp))

Extra parenthesis not needed.

> +		goto err_mem;
> +
> +	for (i = 0; i < quirk->epmulti; ++i) {

i++ is prefered if the operation order doesn't matter. It's not
important but a little more common in the kernel.

> +		fp[i] = kmemdup((const struct audioformat*)(quirk->data + i*sizeof(const struct audioformat)), sizeof(*fp[i]), GFP_KERNEL);

You need to do something about these overlong lines, as they're also
quite unparsable for humans. One solution is to pre-calculate the size
into a temporary variable.

> +		if (!(fp[i])) {

Extra parenthesis not needed.

> +			usb_audio_err(chip, "cannot memdup %d\n", i);

err = -ENOMEM;
goto exit_err;

> +			goto err_mem;
> +		}
> +		if (fp[i]->nr_rates > MAX_NR_RATES) {
> +			err = -EINVAL;
> +			goto err_rates;
> +		}
> +		if (fp[i]->nr_rates > 0 && !rates_found) {
> +			rate_table = kmemdup(fp[i]->rate_table,
> +				     sizeof(int) * fp[i]->nr_rates, GFP_KERNEL);
> +			rates_found = 1;

Not sure if I overlook anything, but isn't the extra variable
rates_found redundant here, as can derive the same information by
checking for (rate_table != NULL)?

> +		}
> +		if (!rate_table) {
> +			err = -ENOMEM;
> +			goto err_rates;
> +		}

This check makes more sense directly underneath the kmemdup().

> +		fp[i]->rate_table = rate_table;
> +
> +		stream[i] = (fp[i]->endpoint & USB_DIR_IN)
> +			? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
> +
> +		err = snd_usb_add_audio_stream(chip, stream[i], fp[i]);
> +		if (err < 0)
> +			goto err_ratetable;
> +
> +		if (fp[i]->iface != get_iface_desc(&iface->altsetting[0])->bInterfaceNumber ||
> +		    fp[i]->altset_idx >= iface->num_altsetting) {
> +			err = -EINVAL;
> +			goto err_ratetable;
> +		}
> +		alts = &iface->altsetting[fp[0]->altset_idx];
> +		fp[i]->datainterval = snd_usb_parse_datainterval(chip, alts);
> +		fp[i]->maxpacksize = le16_to_cpu(get_endpoint(alts, 0)->wMaxPacketSize);
> +	}
> +	usb_set_interface(chip->dev, fp[0]->iface, 0);
> +	snd_usb_init_pitch(chip, fp[0]->iface, alts, fp[0]);
> +	snd_usb_init_sample_rate(chip, fp[0]->iface, alts, fp[0], fp[0]->rate_max);
> +	return 0;
> +
> +err_mem:
> +	return -ENOMEM;

You can drop this ...

> +err_ratetable:
> +	kfree(rate_table);
> +err_rates:
> +	for (i = 0; i < quirk->epmulti; ++i)
> +		if(fp[i])
> +			kfree(fp[i]);
> +	kfree(fp);
> +	kfree(stream);

... and place another label here:

exit_err:
> +	return err;
> +}
> +


Thanks,
Daniel