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