From: Kieran Bingham <kieran@ksquared.org.uk>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-media@vger.kernel.org,
Guennadi Liakhovetski <g.liakhovetski@gmx.de>,
Olivier BRAUN <olivier.braun@stereolabs.com>,
Troy Kisky <troy.kisky@boundarydevices.com>,
Randy Dunlap <rdunlap@infradead.org>,
Philipp Zabel <philipp.zabel@gmail.com>,
Ezequiel Garcia <ezequiel@collabora.com>,
Kieran Bingham <kieran.bingham@ideasonboard.com>
Subject: Re: [PATCH v5 7/9] media: uvcvideo: Split uvc_video_enable into two
Date: Wed, 7 Nov 2018 12:20:54 +0000 [thread overview]
Message-ID: <1d97eb2c-49ad-cc86-0578-abdc525246f9@bingham.xyz> (raw)
In-Reply-To: <2116757.TDLJf9bbp6@avalon>
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,
>
next prev parent reply other threads:[~2018-11-07 21:51 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-06 21:27 [PATCH v5 0/9] Asynchronous UVC Kieran Bingham
2018-11-06 21:27 ` [PATCH v5 1/9] media: uvcvideo: Refactor URB descriptors Kieran Bingham
2018-11-06 21:27 ` [PATCH v5 2/9] media: uvcvideo: Convert decode functions to use new context structure Kieran Bingham
2018-11-06 21:27 ` [PATCH v5 3/9] media: uvcvideo: Protect queue internals with helper Kieran Bingham
2018-11-06 21:27 ` [PATCH v5 4/9] media: uvcvideo: queue: Simplify spin-lock usage Kieran Bingham
2018-11-06 21:27 ` [PATCH v5 5/9] media: uvcvideo: queue: Support asynchronous buffer handling Kieran Bingham
2018-11-06 22:38 ` Laurent Pinchart
2018-11-06 21:27 ` [PATCH v5 6/9] media: uvcvideo: Move decode processing to process context Kieran Bingham
2018-11-06 22:58 ` Laurent Pinchart
2018-11-07 12:22 ` Kieran Bingham
2018-11-06 21:27 ` [PATCH v5 7/9] media: uvcvideo: Split uvc_video_enable into two Kieran Bingham
2018-11-06 23:08 ` Laurent Pinchart
2018-11-07 12:20 ` Kieran Bingham [this message]
2018-11-06 21:27 ` [PATCH v5 8/9] media: uvcvideo: Rename uvc_{un,}init_video() Kieran Bingham
2018-11-06 23:13 ` Laurent Pinchart
2018-11-07 14:30 ` Kieran Bingham
2018-11-07 20:25 ` Laurent Pinchart
2018-11-09 15:41 ` Philipp Zabel
2018-11-09 16:08 ` Kieran Bingham
2018-11-06 21:27 ` [PATCH v5 9/9] media: uvcvideo: Utilise for_each_uvc_urb iterator Kieran Bingham
2018-11-06 23:21 ` Laurent Pinchart
2018-11-07 13:50 ` Kieran Bingham
2018-11-06 23:31 ` [PATCH v5 0/9] Asynchronous UVC Laurent Pinchart
2018-11-08 17:01 ` Laurent Pinchart
2018-11-09 13:25 ` Kieran Bingham
2018-11-27 20:17 ` Pavel Machek
2018-11-27 21:48 ` Laurent Pinchart
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1d97eb2c-49ad-cc86-0578-abdc525246f9@bingham.xyz \
--to=kieran@ksquared.org.uk \
--cc=ezequiel@collabora.com \
--cc=g.liakhovetski@gmx.de \
--cc=kieran.bingham@ideasonboard.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=olivier.braun@stereolabs.com \
--cc=philipp.zabel@gmail.com \
--cc=rdunlap@infradead.org \
--cc=troy.kisky@boundarydevices.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).