[alsa-devel] [PATCH 01/15] ALSA: line6: Make driver configuration more generic.

[Takashi Iwai]

On Thu, 11 Aug 2016 21:02:13 +0200,
Andrej Krutak wrote:
> 
> The main reasons are different settings for USB low/high speed and possible
> different channel counts for in/out; required by POD X3.
> 
> This includes:
> * iso_buffers (count of iso buffers depends on USB speed, 2 is not enough
>   for high speed)
> * bytes_per_frame -> bytes_per_channel
> * max_packet_size -> max_packet_size_in/out
> * USB_INTERVALS_PER_SECOND -> LOW/HIGH settings
>   (high needs 8000, instead of 1000)

The changes are slightly too many done in a shot.  It'd be great if
you can split to a few each logical change if possible.


> --- a/sound/usb/line6/capture.c
> +++ b/sound/usb/line6/capture.c
.....
> @@ -173,17 +175,26 @@ static void audio_in_callback(struct urb *urb)
>  		fbuf = urb->transfer_buffer + fin->offset;
>  		fsize = fin->actual_length;
>  
> -		if (fsize > line6pcm->max_packet_size) {
> +		if (fsize > line6pcm->max_packet_size_in) {
>  			dev_err(line6pcm->line6->ifcdev,
>  				"driver and/or device bug: packet too large (%d > %d)\n",
> -				fsize, line6pcm->max_packet_size);
> +				fsize, line6pcm->max_packet_size_in);
>  		}
>  
>  		length += fsize;
>  
> -		/* the following assumes LINE6_ISO_PACKETS == 1: */
> +#if LINE6_ISO_PACKETS != 1
> +# error "The following assumes LINE6_ISO_PACKETS == 1"
> +/* TODO:
> +   Also, if iso_buffers != 2, the prev frame is almost random at playback side.
> +   This needs to be redesigned. It should be "stable", but we may experience
> +   sync problems on such high-speed configs.
> +*/
> +#endif

You can use BUILD_BUG_ON().



> --- a/sound/usb/line6/driver.h
> +++ b/sound/usb/line6/driver.h
> @@ -18,39 +18,45 @@
>  
>  #include "midi.h"
>  
> -#define USB_INTERVALS_PER_SECOND 1000
> +/* USB 1.1 speed configuration */
> +#define USB_LOW_INTERVALS_PER_SECOND (1000)
> +#define USB_LOW_ISO_BUFFERS (2)
> +
> +/* USB 2.0+ speed configuration */
> +#define USB_HIGH_INTERVALS_PER_SECOND (8000)
> +#define USB_HIGH_ISO_BUFFERS (16)

Superfluous parentheses (all other places, too).


> --- a/sound/usb/line6/pcm.c
> +++ b/sound/usb/line6/pcm.c
....
> @@ -433,24 +438,26 @@ static struct snd_kcontrol_new line6_controls[] = {
>  /*
>  	Cleanup the PCM device.
>  */
> -static void cleanup_urbs(struct line6_pcm_stream *pcms)
> +static void cleanup_urbs(struct line6_pcm_stream *pcms, int iso_buffers)
>  {
>  	int i;
>  
> -	for (i = 0; i < LINE6_ISO_BUFFERS; i++) {
> +	for (i = 0; i < iso_buffers; i++) {
>  		if (pcms->urbs[i]) {

This may cause NULL-dereference when pcms->urbs wasn't allocated.


thanks,

Takashi