From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH 01/15] ALSA: line6: Make driver configuration more generic. Date: Fri, 12 Aug 2016 10:24:12 +0200 Message-ID: References: <1470942147-19848-1-git-send-email-dev@andree.sk> <1470942147-19848-2-git-send-email-dev@andree.sk> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id 37D96266F55 for ; Fri, 12 Aug 2016 10:24:13 +0200 (CEST) In-Reply-To: <1470942147-19848-2-git-send-email-dev@andree.sk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Andrej Krutak Cc: alsa-devel@alsa-project.org List-Id: alsa-devel@alsa-project.org 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