[alsa-devel] [PATCH 4/9 v2] ALSA: usb-audio: use usb_fill_int_urb()

[Takashi Iwai]

On Wed, 20 Jun 2018 15:56:53 +0200,
Takashi Iwai wrote:
> 
> On Wed, 20 Jun 2018 15:52:49 +0200,
> Sebastian Andrzej Siewior wrote:
> > 
> > On 2018-06-20 15:21:49 [+0200], Takashi Iwai wrote:
> > > > usb_fill_int_urb() ensures that syncinterval is within the allowed range
> > > > on HS/SS. The interval value seems to come from
> > > > snd_usb_parse_datainterval() which is bInterval - 1 and only in the rage
> > > > 1 … 4. So in order to keep the magic working I pass datainterval + 1.
> > > 
> > > This needs more explanation.  By this conversion, the interval
> > > computation becomes really tricky.
> > > 
> > > There are two interval calculations.  The first one is
> > > fp->datainterval and it's from snd_usb_parse_datainterval() as you
> > > mentioned.  And a tricky part is that fp->datainterval is 0 on
> > > FULL_SPEED.  Casually, fp->datainterval+1 will be translated to the
> > > correct value since (0 + 1) == (1 << 0).
> > > 
> > > OTOH, for the sync EP, it's taken from ep->syncinterval, which is set
> > > in snd_usb_add_endpoint().  Luckily, again, ep->syncinterval + 1 works
> > > for FULL_SPEED as well, as (1 + 1) == (1 << 1).
> > 
> > Do you want me to add your additional explanation to the description?
> 
> Yes.  It's a very implicit assumption, and needs clarification.
> In addition, the comment 1...4 is only for fp->datainterval, not about
> ep->syncinterval.
> 
> Alternatively, give some comments in the places where putting
> fp->datainterval and ep->syncinterval.

Oh, and for other patches, something about interval could be
mentioned in the change log.  You pass 1 to the macro as if it were
identical as the original code (urb->interval = 1), but this isn't
evaluated equally.

Effectively it'll result in the same, but better to be clarified.


thanks,

Takashi