Message ID | 5a6b4702bd9da438a0635901d2e44ca737842655.1541534872.git-series.kieran.bingham@ideasonboard.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Asynchronous UVC | expand |
Hi Kieran, Thank you for the patch. On Tuesday, 6 November 2018 23:27:18 EET Kieran Bingham wrote: > From: Kieran Bingham <kieran.bingham@ideasonboard.com> > > uvc_video_enable() is used both to start and stop the video stream > object, however the single function entry point shares no code between > the two operations. > > Split the function into two distinct calls, and rename to > uvc_video_start_streaming() and uvc_video_stop_streaming() as > appropriate. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > drivers/media/usb/uvc/uvc_queue.c | 4 +- > drivers/media/usb/uvc/uvc_video.c | 56 +++++++++++++++----------------- > drivers/media/usb/uvc/uvcvideo.h | 3 +- > 3 files changed, 31 insertions(+), 32 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_queue.c > b/drivers/media/usb/uvc/uvc_queue.c index cd8c03341de0..682698ec1118 100644 > --- a/drivers/media/usb/uvc/uvc_queue.c > +++ b/drivers/media/usb/uvc/uvc_queue.c > @@ -176,7 +176,7 @@ static int uvc_start_streaming(struct vb2_queue *vq, > unsigned int count) > > queue->buf_used = 0; > > - ret = uvc_video_enable(stream, 1); > + ret = uvc_video_start_streaming(stream); > if (ret == 0) > return 0; > > @@ -194,7 +194,7 @@ static void uvc_stop_streaming(struct vb2_queue *vq) > lockdep_assert_irqs_enabled(); > > if (vq->type != V4L2_BUF_TYPE_META_CAPTURE) > - uvc_video_enable(uvc_queue_to_stream(queue), 0); > + uvc_video_stop_streaming(uvc_queue_to_stream(queue)); > > spin_lock_irq(&queue->irqlock); > uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR); > diff --git a/drivers/media/usb/uvc/uvc_video.c > b/drivers/media/usb/uvc/uvc_video.c index ce9e40444507..0d35e933856a 100644 > --- a/drivers/media/usb/uvc/uvc_video.c > +++ b/drivers/media/usb/uvc/uvc_video.c > @@ -2082,38 +2082,10 @@ int uvc_video_init(struct uvc_streaming *stream) > return 0; > } > > -/* > - * Enable or disable the video stream. > - */ > -int uvc_video_enable(struct uvc_streaming *stream, int enable) > +int uvc_video_start_streaming(struct uvc_streaming *stream) > { > int ret; > > - if (!enable) { > - uvc_uninit_video(stream, 1); > - if (stream->intf->num_altsetting > 1) { > - usb_set_interface(stream->dev->udev, > - stream->intfnum, 0); > - } else { > - /* UVC doesn't specify how to inform a bulk-based device > - * when the video stream is stopped. Windows sends a > - * CLEAR_FEATURE(HALT) request to the video streaming > - * bulk endpoint, mimic the same behaviour. > - */ > - unsigned int epnum = stream->header.bEndpointAddress > - & USB_ENDPOINT_NUMBER_MASK; > - unsigned int dir = stream->header.bEndpointAddress > - & USB_ENDPOINT_DIR_MASK; > - unsigned int pipe; > - > - pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir; > - usb_clear_halt(stream->dev->udev, pipe); > - } > - > - uvc_video_clock_cleanup(stream); > - return 0; > - } > - > ret = uvc_video_clock_init(stream); > if (ret < 0) > return ret; > @@ -2136,3 +2108,29 @@ int uvc_video_enable(struct uvc_streaming *stream, > int enable) > > return ret; > } > + > +int uvc_video_stop_streaming(struct uvc_streaming *stream) > +{ > + uvc_uninit_video(stream, 1); > + if (stream->intf->num_altsetting > 1) { > + usb_set_interface(stream->dev->udev, > + stream->intfnum, 0); This now holds on a single line. > + } else { > + /* UVC doesn't specify how to inform a bulk-based device Let's fix the checkpatch.pl warning here. > + * when the video stream is stopped. Windows sends a > + * CLEAR_FEATURE(HALT) request to the video streaming > + * bulk endpoint, mimic the same behaviour. > + */ > + unsigned int epnum = stream->header.bEndpointAddress > + & USB_ENDPOINT_NUMBER_MASK; > + unsigned int dir = stream->header.bEndpointAddress > + & USB_ENDPOINT_DIR_MASK; > + unsigned int pipe; > + > + pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir; > + usb_clear_halt(stream->dev->udev, pipe); > + } > + > + uvc_video_clock_cleanup(stream); > + return 0; As this always return 0 you can make it a void function. Apart from that, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I'll take the patch in my tree with the above changes. > +} > diff --git a/drivers/media/usb/uvc/uvcvideo.h > b/drivers/media/usb/uvc/uvcvideo.h index 0953e2e59a79..c0a120496a1f 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -784,7 +784,8 @@ void uvc_mc_cleanup_entity(struct uvc_entity *entity); > int uvc_video_init(struct uvc_streaming *stream); > int uvc_video_suspend(struct uvc_streaming *stream); > int uvc_video_resume(struct uvc_streaming *stream, int reset); > -int uvc_video_enable(struct uvc_streaming *stream, int enable); > +int uvc_video_start_streaming(struct uvc_streaming *stream); > +int uvc_video_stop_streaming(struct uvc_streaming *stream); > int uvc_probe_video(struct uvc_streaming *stream, > struct uvc_streaming_control *probe); > int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
Hi Laurent, On 06/11/2018 23:08, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Tuesday, 6 November 2018 23:27:18 EET Kieran Bingham wrote: >> From: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >> uvc_video_enable() is used both to start and stop the video stream >> object, however the single function entry point shares no code between >> the two operations. >> >> Split the function into two distinct calls, and rename to >> uvc_video_start_streaming() and uvc_video_stop_streaming() as >> appropriate. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> drivers/media/usb/uvc/uvc_queue.c | 4 +- >> drivers/media/usb/uvc/uvc_video.c | 56 +++++++++++++++----------------- >> drivers/media/usb/uvc/uvcvideo.h | 3 +- >> 3 files changed, 31 insertions(+), 32 deletions(-) >> >> diff --git a/drivers/media/usb/uvc/uvc_queue.c >> b/drivers/media/usb/uvc/uvc_queue.c index cd8c03341de0..682698ec1118 100644 >> --- a/drivers/media/usb/uvc/uvc_queue.c >> +++ b/drivers/media/usb/uvc/uvc_queue.c >> @@ -176,7 +176,7 @@ static int uvc_start_streaming(struct vb2_queue *vq, >> unsigned int count) >> >> queue->buf_used = 0; >> >> - ret = uvc_video_enable(stream, 1); >> + ret = uvc_video_start_streaming(stream); >> if (ret == 0) >> return 0; >> >> @@ -194,7 +194,7 @@ static void uvc_stop_streaming(struct vb2_queue *vq) >> lockdep_assert_irqs_enabled(); >> >> if (vq->type != V4L2_BUF_TYPE_META_CAPTURE) >> - uvc_video_enable(uvc_queue_to_stream(queue), 0); >> + uvc_video_stop_streaming(uvc_queue_to_stream(queue)); >> >> spin_lock_irq(&queue->irqlock); >> uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR); >> diff --git a/drivers/media/usb/uvc/uvc_video.c >> b/drivers/media/usb/uvc/uvc_video.c index ce9e40444507..0d35e933856a 100644 >> --- a/drivers/media/usb/uvc/uvc_video.c >> +++ b/drivers/media/usb/uvc/uvc_video.c >> @@ -2082,38 +2082,10 @@ int uvc_video_init(struct uvc_streaming *stream) >> return 0; >> } >> >> -/* >> - * Enable or disable the video stream. >> - */ >> -int uvc_video_enable(struct uvc_streaming *stream, int enable) >> +int uvc_video_start_streaming(struct uvc_streaming *stream) >> { >> int ret; >> >> - if (!enable) { >> - uvc_uninit_video(stream, 1); >> - if (stream->intf->num_altsetting > 1) { >> - usb_set_interface(stream->dev->udev, >> - stream->intfnum, 0); >> - } else { >> - /* UVC doesn't specify how to inform a bulk-based device >> - * when the video stream is stopped. Windows sends a >> - * CLEAR_FEATURE(HALT) request to the video streaming >> - * bulk endpoint, mimic the same behaviour. >> - */ >> - unsigned int epnum = stream->header.bEndpointAddress >> - & USB_ENDPOINT_NUMBER_MASK; >> - unsigned int dir = stream->header.bEndpointAddress >> - & USB_ENDPOINT_DIR_MASK; >> - unsigned int pipe; >> - >> - pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir; >> - usb_clear_halt(stream->dev->udev, pipe); >> - } >> - >> - uvc_video_clock_cleanup(stream); >> - return 0; >> - } >> - >> ret = uvc_video_clock_init(stream); >> if (ret < 0) >> return ret; >> @@ -2136,3 +2108,29 @@ int uvc_video_enable(struct uvc_streaming *stream, >> int enable) >> >> return ret; >> } >> + >> +int uvc_video_stop_streaming(struct uvc_streaming *stream) >> +{ >> + uvc_uninit_video(stream, 1); >> + if (stream->intf->num_altsetting > 1) { >> + usb_set_interface(stream->dev->udev, >> + stream->intfnum, 0); > > This now holds on a single line. Ah yes. > >> + } else { >> + /* UVC doesn't specify how to inform a bulk-based device > > Let's fix the checkpatch.pl warning here. Oh ? I didn't get any checkpatch warnings. Do I need to add some parameters to my checkpatch now? > >> + * when the video stream is stopped. Windows sends a >> + * CLEAR_FEATURE(HALT) request to the video streaming >> + * bulk endpoint, mimic the same behaviour. >> + */ >> + unsigned int epnum = stream->header.bEndpointAddress >> + & USB_ENDPOINT_NUMBER_MASK; >> + unsigned int dir = stream->header.bEndpointAddress >> + & USB_ENDPOINT_DIR_MASK; >> + unsigned int pipe; >> + >> + pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir; >> + usb_clear_halt(stream->dev->udev, pipe); >> + } >> + >> + uvc_video_clock_cleanup(stream); >> + return 0; > > As this always return 0 you can make it a void function. Certainly. > > Apart from that, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > I'll take the patch in my tree with the above changes. > Great, thanks. -- KB >> +} >> diff --git a/drivers/media/usb/uvc/uvcvideo.h >> b/drivers/media/usb/uvc/uvcvideo.h index 0953e2e59a79..c0a120496a1f 100644 >> --- a/drivers/media/usb/uvc/uvcvideo.h >> +++ b/drivers/media/usb/uvc/uvcvideo.h >> @@ -784,7 +784,8 @@ void uvc_mc_cleanup_entity(struct uvc_entity *entity); >> int uvc_video_init(struct uvc_streaming *stream); >> int uvc_video_suspend(struct uvc_streaming *stream); >> int uvc_video_resume(struct uvc_streaming *stream, int reset); >> -int uvc_video_enable(struct uvc_streaming *stream, int enable); >> +int uvc_video_start_streaming(struct uvc_streaming *stream); >> +int uvc_video_stop_streaming(struct uvc_streaming *stream); >> int uvc_probe_video(struct uvc_streaming *stream, >> struct uvc_streaming_control *probe); >> int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, >
diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c index cd8c03341de0..682698ec1118 100644 --- a/drivers/media/usb/uvc/uvc_queue.c +++ b/drivers/media/usb/uvc/uvc_queue.c @@ -176,7 +176,7 @@ static int uvc_start_streaming(struct vb2_queue *vq, unsigned int count) queue->buf_used = 0; - ret = uvc_video_enable(stream, 1); + ret = uvc_video_start_streaming(stream); if (ret == 0) return 0; @@ -194,7 +194,7 @@ static void uvc_stop_streaming(struct vb2_queue *vq) lockdep_assert_irqs_enabled(); if (vq->type != V4L2_BUF_TYPE_META_CAPTURE) - uvc_video_enable(uvc_queue_to_stream(queue), 0); + uvc_video_stop_streaming(uvc_queue_to_stream(queue)); spin_lock_irq(&queue->irqlock); uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR); diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c index ce9e40444507..0d35e933856a 100644 --- a/drivers/media/usb/uvc/uvc_video.c +++ b/drivers/media/usb/uvc/uvc_video.c @@ -2082,38 +2082,10 @@ int uvc_video_init(struct uvc_streaming *stream) return 0; } -/* - * Enable or disable the video stream. - */ -int uvc_video_enable(struct uvc_streaming *stream, int enable) +int uvc_video_start_streaming(struct uvc_streaming *stream) { int ret; - if (!enable) { - uvc_uninit_video(stream, 1); - if (stream->intf->num_altsetting > 1) { - usb_set_interface(stream->dev->udev, - stream->intfnum, 0); - } else { - /* UVC doesn't specify how to inform a bulk-based device - * when the video stream is stopped. Windows sends a - * CLEAR_FEATURE(HALT) request to the video streaming - * bulk endpoint, mimic the same behaviour. - */ - unsigned int epnum = stream->header.bEndpointAddress - & USB_ENDPOINT_NUMBER_MASK; - unsigned int dir = stream->header.bEndpointAddress - & USB_ENDPOINT_DIR_MASK; - unsigned int pipe; - - pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir; - usb_clear_halt(stream->dev->udev, pipe); - } - - uvc_video_clock_cleanup(stream); - return 0; - } - ret = uvc_video_clock_init(stream); if (ret < 0) return ret; @@ -2136,3 +2108,29 @@ int uvc_video_enable(struct uvc_streaming *stream, int enable) return ret; } + +int uvc_video_stop_streaming(struct uvc_streaming *stream) +{ + uvc_uninit_video(stream, 1); + if (stream->intf->num_altsetting > 1) { + usb_set_interface(stream->dev->udev, + stream->intfnum, 0); + } else { + /* UVC doesn't specify how to inform a bulk-based device + * when the video stream is stopped. Windows sends a + * CLEAR_FEATURE(HALT) request to the video streaming + * bulk endpoint, mimic the same behaviour. + */ + unsigned int epnum = stream->header.bEndpointAddress + & USB_ENDPOINT_NUMBER_MASK; + unsigned int dir = stream->header.bEndpointAddress + & USB_ENDPOINT_DIR_MASK; + unsigned int pipe; + + pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir; + usb_clear_halt(stream->dev->udev, pipe); + } + + uvc_video_clock_cleanup(stream); + return 0; +} diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 0953e2e59a79..c0a120496a1f 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -784,7 +784,8 @@ void uvc_mc_cleanup_entity(struct uvc_entity *entity); int uvc_video_init(struct uvc_streaming *stream); int uvc_video_suspend(struct uvc_streaming *stream); int uvc_video_resume(struct uvc_streaming *stream, int reset); -int uvc_video_enable(struct uvc_streaming *stream, int enable); +int uvc_video_start_streaming(struct uvc_streaming *stream); +int uvc_video_stop_streaming(struct uvc_streaming *stream); int uvc_probe_video(struct uvc_streaming *stream, struct uvc_streaming_control *probe); int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,