On Thu, 2009-06-11 at 01:40 -0300, Mauro Carvalho Chehab wrote:
> Em Wed, 10 Jun 2009 22:39:51 -0300
> Mauro Carvalho Chehab <mchehab@xxxxxxxxxx> escreveu:
>
> > Em Sun, 31 May 2009 14:41:52 +0800
> > "Figo.zhang" <figo1802@xxxxxxxxx> escreveu:
> >
> > > video_register_device() return zero on success, it would not return a positive integer.
> > >
> > > Signed-off-by: Figo.zhang <figo1802@xxxxxxxxx>
> > > ---
> > > drivers/media/video/ov511.c | 2 +-
> > > 1 files changed, 1 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/drivers/media/video/ov511.c b/drivers/media/video/ov511.c
> > > index 9af5532..816427e 100644
> > > --- a/drivers/media/video/ov511.c
> > > +++ b/drivers/media/video/ov511.c
> > > @@ -5851,7 +5851,7 @@ ov51x_probe(struct usb_interface *intf, const struct usb_device_id *id)
> > > break;
> > >
> > > if (video_register_device(ov->vdev, VFL_TYPE_GRABBER,
> > > - unit_video[i]) >= 0) {
> > > + unit_video[i]) == 0) {
> > > break;
> > > }
> > > }
> >
> > Nack.
> >
> > Errors are always negative. So, any zero or positive value indicates that no error occurred.
> >
> > Yet, the logic for forcing ov51x to specific minor number seems broken: it will
> > end by registering the device twice, if used.
> >
> > So, that part of the function needs a rewrite. I'll fix it.
> >
>
> Hmm... the issue seems more complex than I've imagined...
>
> When ov511 were written, it was assumed that video_register_device(dev, v, nr)
> would have the following behavior:
>
> if nr = -1, it would do dynamic minor allocation;
> if nr >= 0, it would allocate to 'nr' minor or fail.
>
> With the original behavior.
>
> The ov511_probe registering loop is doing something like this (I did a cleanup
> to allow a better understand of the logic):
>
> <snip>
> #define OV511_MAX_UNIT_VIDEO 16
> ...
> static int unit_video[OV511_MAX_UNIT_VIDEO];
> ...
> module_param_array(unit_video, int, &num_uv, 0);
> MODULE_PARM_DESC(unit_video,
> "Force use of specific minor number(s). 0 is not allowed.");
> ...
> static int
> ov51x_probe(struct usb_interface *intf, const struct usb_device_id *id)
> {
> ...
> for (i = 0; i < OV511_MAX_UNIT_VIDEO; i++) {
> /* Minor 0 cannot be specified; assume user wants autodetect */
> if (unit_video[i] == 0)
> break;
>
> if (video_register_device(ov->vdev, VFL_TYPE_GRABBER,
> unit_video[i]) >= 0)
> goto register_succeded;
> }
>
> /* Use the next available one */
> if (video_register_device(ov->vdev, VFL_TYPE_GRABBER, -1) < 0) {
> err("video_register_device failed");
> goto error;
>
> register_succeeded:
> dev_info(&intf->dev, "Device at %s registered to minor %d\n",
> ov->usb_path, ov->vdev->minor);
> </snip>
>
> So, if you probe ov511 with a list of device numbers, for example:
>
> modprobe ov511 4,1,3
>
you means specify the minior video
device: /dev/vidoe4,/dev/video1, /dev/video3 ?
it maybe : modprobe ov511 unit_video=4,1,3
> And assuming that you have 5 ov511 devices, and connect they one by one,
> they'll gain the following device numbers (at the connection order):
> /dev/video4
> /dev/video1
> /dev/video3
> /dev/video0
> /dev/video2
>
> However, changeset 9133:
>
> changeset: 9133:64aed7485a43
> parent: 9073:4db9722caf4f
> user: Hans Verkuil <hverkuil@xxxxxxxxx>
> date: Sat Oct 04 13:36:54 2008 +0200
> summary: v4l: disconnect kernel number from minor
>
> Changed the behavior video_register_device(dev, v, nr):
>
> + nr = find_next_zero_bit(video_nums[type], minor_cnt, nr == -1 ? 0 : nr);
>
> So, instead of accepting just -1 or nr, it will now do:
> if nr = -1, it will do dynamic minor allocation (as before);
> if nr >= 0, it will also do dynamic minor allocation, but starting from nr.
>
> So, the new code won't fail if nr is already allocated, but it will return the
> next unallocated nr.
>
> With the ov511 code, this means that you'll have, instead:
>
> /dev/video5
> /dev/video6
> /dev/video7
> /dev/video8
> /dev/video9
>
> So, changeset 9133 actually broke the ov511 probe original behavior.
>
> In order to restore the original behavior, the probe logic should be replaced
> by something else (like the approach taken by em28xx).
>
> Also, changeset 9133 may also cause other side effects on other drivers that
> were expecting the original behavior.
>
> To make things worse, the function comment doesn't properly explain the current
> behavior.
>
> ---
>
> Figo,
>
> Since we are in the middle of a merge window, it is unlikely that I'll have
> enough time to properly fix it right now (since my priority right now is to
> merge the existing patches).
>
> As you started proposing a patch for it, maybe you could try to fix it and
> check about similar troubles on other drivers.
>
> Cheers,
> Mauro
in v2, if insmod without specify 'unit_video', it use autodetect video device.
if specify the 'unit_video', it will try to detect start from nr.
Signed-off-by: Figo.zhang <figo1802@xxxxxxxxx>
---
drivers/media/video/ov511.c | 32 +++++++++++++++++---------------
1 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/drivers/media/video/ov511.c b/drivers/media/video/ov511.c
index 9af5532..293695f 100644
--- a/drivers/media/video/ov511.c
+++ b/drivers/media/video/ov511.c
@@ -180,7 +180,7 @@ module_param(force_palette, int, 0);
MODULE_PARM_DESC(force_palette, "Force the palette to a specific value");
module_param(backlight, int, 0);
MODULE_PARM_DESC(backlight, "For objects that are lit from behind");
-static unsigned int num_uv;
+static unsigned int num_uv = 0;
module_param_array(unit_video, int, &num_uv, 0);
MODULE_PARM_DESC(unit_video,
"Force use of specific minor number(s). 0 is not allowed.");
@@ -5845,22 +5845,24 @@ ov51x_probe(struct usb_interface *intf, const struct usb_device_id *id)
ov->vdev->parent = &intf->dev;
video_set_drvdata(ov->vdev, ov);
- for (i = 0; i < OV511_MAX_UNIT_VIDEO; i++) {
- /* Minor 0 cannot be specified; assume user wants autodetect */
- if (unit_video[i] == 0)
- break;
-
- if (video_register_device(ov->vdev, VFL_TYPE_GRABBER,
- unit_video[i]) >= 0) {
- break;
+ /*if num_uv is zero, it autodetect*/
+ if(num_uv == 0){
+ if ((ov->vdev->minor == -1) &&
+ video_register_device(ov->vdev, VFL_TYPE_GRABBER, -1) < 0) {
+ err("video_register_device failed");
+ goto error;
}
- }
+ }else {
+ for (i = 0; i < num_uv; i++) {
+ /* Minor 0 cannot be specified; assume user wants autodetect */
+ if (unit_video[i] == 0)
+ break;
- /* Use the next available one */
- if ((ov->vdev->minor == -1) &&
- video_register_device(ov->vdev, VFL_TYPE_GRABBER, -1) < 0) {
- err("video_register_device failed");
- goto error;
+ if (video_register_device(ov->vdev, VFL_TYPE_GRABBER,
+ unit_video[i]) == 0) {
+ break;
+ }
+ }
}
dev_info(&intf->dev, "Device at %s registered to minor %d\n",
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
