[alsa-devel] [FT C400, PATCH RFC, v4 02/10] usb-audio: correct sync ep init

[Eldad Zack]

Hi Takashi,

On Thu, 29 Nov 2012, Takashi Iwai wrote:
> At Wed, 28 Nov 2012 23:55:33 +0100,
> Eldad Zack wrote:
> >  /*
> > + * Configure the sync ep using the rate and pcm format of the data ep.
> > + */
> > +static int configure_sync_endpoint(struct snd_usb_substream *subs)
> > +{
> > +	int ret;
> > +	struct list_head *p;
> > +	struct audioformat *sync_fp;
> > +	int sync_channels = 0;
> > +	int sync_period_bytes = subs->period_bytes;
> > +	int direction = subs->direction ^ 1;
> 
> Better to rename for avoiding misunderstanding.
> "direction" sounds like the direction of this substream.

Thanks for the pointer. I ended up getting rid rid of it 
altogether, since the following is redundant:

> > +	struct snd_usb_substream *sync_subs =
> > +		&subs->stream->substream[direction];
> > +
> > +	if (sync_subs->ep_num != subs->sync_endpoint->ep_num) {
> > +		snd_printk(KERN_ERR "%s: cannot find the sync ep (direction %d, found %x, wanted %x).\n",
> > +			__func__, direction,
> > +				subs->stream->substream[direction].ep_num,
> > +				subs->sync_endpoint->ep_num);
> > +		return -EINVAL;
> > +	}
> 
> This sanity check would suit better where the implicit_fb is checked.
> It doesn't have to be performed at each time.
> 
> And, the whole restriction you introduced is only for implicit fb
> case, no?

Yes, I took a closer look and saw that the only way for the 
sync_endpoint member to be at all initialized is when it is 1) implicit 
fb, 2) in the same stream as the data and in the opposite direction,
so there's no point in checking that at all.

> > +	/*
> > +	 * Try to find a format with a number of channels matching
> > +	 * the data substream. If no such format was found, pick one
> > +	 * with non zero channel number at random.
> > +	 */
> > +	list_for_each(p, &sync_subs->fmt_list) {
> Cleaner to use list_for_each_entry().
> (Yes, there are other codes using list_for_each() there, but remember
>  how old the usb-audio driver code is :)

Thanks again, it looks more concise that way. I'll see if it is possible 
to clean up other spots the same way and post later.

> Can be simplified like
> 
> 		if (!sync_channels || fp->channels == sub->channels) {
> 			sync_channels = fp->channels;
> 			sync_fp = fp;
> 		}
> 
> But, I wonder whether you don't have to check other parameters such as
> rate, format, etc?

Yes, especially since both rate and format will influence the period 
bytes count and then the usb host driver will error out. It is unlikely
that such a configuration exist, which doesn't support the same rate
and PCM format in both direction, but it is worth checking.

Patch follows.

Cheers,
Eldad