From patchwork Mon Jun 15 13:44:21 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mauro Carvalho Chehab X-Patchwork-Id: 30326 Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by demeter.kernel.org (8.14.2/8.14.2) with ESMTP id n5FDjhjs000508 for ; Mon, 15 Jun 2009 13:45:43 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751521AbZFONph (ORCPT ); Mon, 15 Jun 2009 09:45:37 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751849AbZFONph (ORCPT ); Mon, 15 Jun 2009 09:45:37 -0400 Received: from bombadil.infradead.org ([18.85.46.34]:39949 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751521AbZFONpg (ORCPT ); Mon, 15 Jun 2009 09:45:36 -0400 Received: from 200-153-220-101.dsl.telesp.net.br ([200.153.220.101] helo=pedra.chehab.org) by bombadil.infradead.org with esmtpsa (Exim 4.69 #1 (Red Hat Linux)) id 1MGCUN-0000fE-Uz; Mon, 15 Jun 2009 13:45:15 +0000 Date: Mon, 15 Jun 2009 10:44:21 -0300 From: Mauro Carvalho Chehab To: Hans Verkuil Cc: linux-media@vger.kernel.org Subject: Re: RFC: remove video_register_device_index, add video_register_device_range Message-ID: <20090615104421.5d6db842@pedra.chehab.org> In-Reply-To: <200906151325.29079.hverkuil@xs4all.nl> References: <200906151325.29079.hverkuil@xs4all.nl> X-Mailer: Claws Mail 3.3.1 (GTK+ 2.10.4; x86_64-redhat-linux-gnu) Mime-Version: 1.0 X-SRS-Rewrite: SMTP reverse-path rewritten from by bombadil.infradead.org See http://www.infradead.org/rpr.html Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org Em Mon, 15 Jun 2009 13:25:28 +0200 Hans Verkuil escreveu: > Hi all, > > While looking at the video_register_device changes that broke ov511 I realized > that the video_register_device_index function is never called from drivers. It > will always assign a default index number. I also don't see a good use-case > for giving it an explicit index. My proposal is to remove this API. Since it > is never called, nothing will change effectively. I'm never happy with unused > functions. It sounds ok to me. > However, I think we do need a new video_register_device_range function. This > would be identical to video_register_device, but with an additional count > argument: this allows drivers to select a kernel number in the range of > nr to nr + count - 1. If cnt == -1, then the maximum is the compiled-in > maximum. > > So video_register_device would call video_register_device_range(...nr, 1), > thus restoring the original behavior, while ivtv and cx18 can call > video_register_device_range(...nr, -1), thus keeping the current behavior. I don't think that this is needed. The issue with ov511 is that it was using the error condition of video_register to identify how much ov511 devices were already registered. I already fixed it by adding an explicit device number count on it, just like all the other drivers. With this, ov511 will behave like the other drivers. With the current implementation, it should honor the explicit minor order passed via modprobe parameter, as before. There's one small change on the behavior that it is also present on all other devices that allow users to explicit the desired minor order: instead of failing if that device is already used, it will get the next available one. IMO, this is better than just failing. So, the only remaining issue is that video_register should print a warning if it had to get a different minor than specified. In other words, the enclosed patch seems to be a good approach to close this issue Cheers, Mauro --- v4l2-dev: Print a warning if need to use a different minor than specified From: Mauro Carvalho Chehab video_register_device has two behaviors: dynamic minor attribution or forced minor attribution. The latter mode is used to allow users to force probing a device using a certain minor order. Even for the last case, video_register_device won't fail if that minor is already used but, instead, it will seek for the next available minor, without warning users about that. While don't fail due to a busy minor seems the better behavior, it should be printing a warning when this happen. While here, let's remove video_register_device_index(), since no driver is using it. Signed-off-by: Mauro Carvalho Chehab --- linux/drivers/media/video/v4l2-dev.c | 31 ++++++++++++++++--------------- linux/include/media/v4l2-dev.h | 5 ++--- 2 files changed, 18 insertions(+), 18 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html --- master.orig/linux/drivers/media/video/v4l2-dev.c +++ master/linux/drivers/media/video/v4l2-dev.c @@ -384,23 +384,18 @@ static int get_index(struct video_device return i == VIDEO_NUM_DEVICES ? -ENFILE : i; } -int video_register_device(struct video_device *vdev, int type, int nr) -{ - return video_register_device_index(vdev, type, nr, -1); -} -EXPORT_SYMBOL(video_register_device); /** - * video_register_device_index - register video4linux devices + * video_register_device - register video4linux devices * @vdev: video device structure we want to register * @type: type of device to register * @nr: which device number (0 == /dev/video0, 1 == /dev/video1, ... * -1 == first free) - * @index: stream number based on parent device; - * -1 if auto assign, requested number otherwise * * The registration code assigns minor numbers based on the type - * requested. -ENFILE is returned in all the device slots for this + * requested. If the requested one is already used, get the next + * available one and prints a warning. + * -ENFILE is returned in all the device slots for this * category are full. If not then the minor field is set and the * driver initialize function is called (if non %NULL). * @@ -416,10 +411,10 @@ EXPORT_SYMBOL(video_register_device); * * %VFL_TYPE_RADIO - A radio card */ -int video_register_device_index(struct video_device *vdev, int type, int nr, - int index) +int video_register_device(struct video_device *vdev, const int type, + const int desirednr) { - int i = 0; + int i = 0, nr; int ret; int minor_offset = 0; int minor_cnt = VIDEO_NUM_DEVICES; @@ -493,7 +488,8 @@ int video_register_device_index(struct v /* Pick a minor number */ mutex_lock(&videodev_lock); - nr = find_next_zero_bit(video_nums[type], minor_cnt, nr == -1 ? 0 : nr); + nr = find_next_zero_bit(video_nums[type], minor_cnt, + desirednr == -1 ? 0 : desirednr); if (nr == minor_cnt) nr = find_first_zero_bit(video_nums[type], minor_cnt); if (nr == minor_cnt) { @@ -501,6 +497,11 @@ int video_register_device_index(struct v mutex_unlock(&videodev_lock); return -ENFILE; } + + if (desirednr >= 0 && nr != desirednr) + printk(KERN_INFO "Minor %d is already used. Using instead %d\n", + desirednr, nr); + #ifdef CONFIG_VIDEO_FIXED_MINOR_RANGES /* 1-on-1 mapping of kernel number to minor number */ i = nr; @@ -520,7 +521,7 @@ int video_register_device_index(struct v set_bit(nr, video_nums[type]); /* Should not happen since we thought this minor was free */ WARN_ON(video_device[vdev->minor] != NULL); - ret = vdev->index = get_index(vdev, index); + ret = vdev->index = get_index(vdev, -1); mutex_unlock(&videodev_lock); if (ret < 0) { @@ -612,7 +613,7 @@ cleanup: vdev->minor = -1; return ret; } -EXPORT_SYMBOL(video_register_device_index); +EXPORT_SYMBOL(video_register_device); /** * video_unregister_device - unregister a video4linux device --- master.orig/linux/include/media/v4l2-dev.h +++ master/linux/include/media/v4l2-dev.h @@ -103,9 +103,8 @@ struct video_device you call video_device_release() on failure. Also note that vdev->minor is set to -1 if the registration failed. */ -int __must_check video_register_device(struct video_device *vdev, int type, int nr); -int __must_check video_register_device_index(struct video_device *vdev, - int type, int nr, int index); +int __must_check video_register_device(struct video_device *vdev, + const int type, const int nr); /* Unregister video devices. Will do nothing if vdev == NULL or vdev->minor < 0. */