From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from perceval.ideasonboard.com ([213.167.242.64]:58192 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726455AbeKHF5R (ORCPT ); Thu, 8 Nov 2018 00:57:17 -0500 From: Laurent Pinchart To: kieran.bingham@ideasonboard.com Cc: linux-media@vger.kernel.org, Guennadi Liakhovetski , Olivier BRAUN , Troy Kisky , Randy Dunlap , Philipp Zabel , Ezequiel Garcia Subject: Re: [PATCH v5 8/9] media: uvcvideo: Rename uvc_{un,}init_video() Date: Wed, 07 Nov 2018 22:25:27 +0200 Message-ID: <9290334.s72v5oSQOh@avalon> In-Reply-To: References: <1648340.AJeOYgVR3M@avalon> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-media-owner@vger.kernel.org List-ID: Hi Kieran, On Wednesday, 7 November 2018 16:30:46 EET Kieran Bingham wrote: > On 06/11/2018 23:13, Laurent Pinchart wrote: > > On Tuesday, 6 November 2018 23:27:19 EET Kieran Bingham wrote: > >> From: Kieran Bingham > >> > >> We have both uvc_init_video() and uvc_video_init() calls which can be > >> quite confusing to determine the process for each. Now that video > >> uvc_video_enable() has been renamed to uvc_video_start_streaming(), > >> adapt these calls to suit the new flow. > >> > >> Rename uvc_init_video() to uvc_video_start() and uvc_uninit_video() to > >> uvc_video_stop(). > > > > I agree that these functions are badly named and should be renamed. We are > > however entering the nitpicking territory :-) The two functions do more > > that starting and stopping, they also allocate and free URBs and the > > associated buffers. It could also be argued that they don't actually > > start and stop anything, as beyond URB management, they just queue the > > URBs initially and kill them. I thus wonder if we could come up with > > better names. > > Well the act of killing (poisoning now) the URBs will certainly stop the > stream, but I guess submitting the URBs isn't necessarily the key act to > starting the stream. > > I believe that needs the interface to be set correctly, and the buffers > to be available? > > Although - I've just double-checked uvc_{video_start,init_video}() and > that is indeed what it does? > > - start stats > - Initialise endpoints > - Perform allocations > - Submit URBs > > Am I missing something? Is there another step that is pivotal to > starting the USB packet/urb stream flow after this point ? > > > Is it not true that the USB stack will start processing data at > submitting URB completion callbacks after the end of uvc_video_start(); > and will no longer process data at the end of uvc_video_stop() (and thus > no more completion callbacks)? > > (That's a real question to verify my interpretation) > > To me - these functions feel like the real 'start' and 'stop' components > of the data stream - hence my choice in naming. The other part of the start operation is committing the streaming parameters (see uvc_video_start_streaming()). For the stop operation it's issuing a SET_INTERFACE or CLEAR_FEATURE(HALT) request (see uvc_video_stop_streaming()). > Is your concern that you would like the functions to be more descriptive > over their other actions such as? : > > uvc_video_initialise_start() > uvc_video_allocate_init_start() > > Or something else? (I don't think those two are good names though) Probably something else :-) A possibly equally bad proposal would be uvc_video_start_transfer() and uvc_video_stop_transfer(). -- Regards, Laurent Pinchart