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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
