diff mbox

[27/27,v2] media: uvcvideo: use usb_fill_int_urb() for the ->intarval value

Message ID 20180620152122.ebstiwvdpbhgsbrs@linutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Sebastian Andrzej Siewior June 20, 2018, 3:21 p.m. UTC
uvc_init_video_isoc() assigns
	urb->interval = p->desc.bInterval;

for the interval. This is correct for FS/LS. For HS/SS the bInterval
value is using a logarithmic encoding. The usb_fill_int_urb() function
takes this into account while settings the ->interval member.
->start_frame is set to -1 on init and should be filled by the HC on
completion of the URB.

Use usb_fill_int_urb() to fill the members of the struct urb.

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/uvc/uvc_video.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Comments

Alan Stern June 20, 2018, 3:40 p.m. UTC | #1
On Wed, 20 Jun 2018, Sebastian Andrzej Siewior wrote:

> uvc_init_video_isoc() assigns
> 	urb->interval = p->desc.bInterval;
> 
> for the interval. This is correct for FS/LS.

That's a strange thing to say.  For one thing, LS devices don't support 
isochronous transfers at all.  And while this assignment would be 
correct for FS interrupt URBs, it is wrong for FS isochronous URBs.

> For HS/SS the bInterval
> value is using a logarithmic encoding. The usb_fill_int_urb() function
> takes this into account while settings the ->interval member.
> ->start_frame is set to -1 on init and should be filled by the HC on
> completion of the URB.
> 
> Use usb_fill_int_urb() to fill the members of the struct urb.

Please don't do this.  If you instead on using an inline routine to 
save on source code, create an explicit usb_fill_isoc_urb() function.

Alan Stern

> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index a88b2e51a666..79e7a827ed44 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1619,21 +1619,19 @@ static int uvc_init_video_isoc(struct uvc_streaming *stream,
>  			return -ENOMEM;
>  		}
>  
> -		urb->dev = stream->dev->udev;
> -		urb->context = stream;
> -		urb->pipe = usb_rcvisocpipe(stream->dev->udev,
> -				ep->desc.bEndpointAddress);
> +		usb_fill_int_urb(urb, stream->dev->udev,
> +				 usb_rcvisocpipe(stream->dev->udev,
> +						 ep->desc.bEndpointAddress),
> +				 stream->urb_buffer[i], size,
> +				 uvc_video_complete, stream,
> +				 ep->desc.bInterval);
>  #ifndef CONFIG_DMA_NONCOHERENT
>  		urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
>  		urb->transfer_dma = stream->urb_dma[i];
>  #else
>  		urb->transfer_flags = URB_ISO_ASAP;
>  #endif
> -		urb->interval = ep->desc.bInterval;
> -		urb->transfer_buffer = stream->urb_buffer[i];
> -		urb->complete = uvc_video_complete;
>  		urb->number_of_packets = npackets;
> -		urb->transfer_buffer_length = size;
>  
>  		for (j = 0; j < npackets; ++j) {
>  			urb->iso_frame_desc[j].offset = j * psize;
>
diff mbox

Patch

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index a88b2e51a666..79e7a827ed44 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1619,21 +1619,19 @@  static int uvc_init_video_isoc(struct uvc_streaming *stream,
 			return -ENOMEM;
 		}
 
-		urb->dev = stream->dev->udev;
-		urb->context = stream;
-		urb->pipe = usb_rcvisocpipe(stream->dev->udev,
-				ep->desc.bEndpointAddress);
+		usb_fill_int_urb(urb, stream->dev->udev,
+				 usb_rcvisocpipe(stream->dev->udev,
+						 ep->desc.bEndpointAddress),
+				 stream->urb_buffer[i], size,
+				 uvc_video_complete, stream,
+				 ep->desc.bInterval);
 #ifndef CONFIG_DMA_NONCOHERENT
 		urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
 		urb->transfer_dma = stream->urb_dma[i];
 #else
 		urb->transfer_flags = URB_ISO_ASAP;
 #endif
-		urb->interval = ep->desc.bInterval;
-		urb->transfer_buffer = stream->urb_buffer[i];
-		urb->complete = uvc_video_complete;
 		urb->number_of_packets = npackets;
-		urb->transfer_buffer_length = size;
 
 		for (j = 0; j < npackets; ++j) {
 			urb->iso_frame_desc[j].offset = j * psize;