[PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
Hi Mauro, Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc for the following: - v4l2-subdev: remove unnecessary check - cx18: remove bogus init call. - cxusb: fix compile warning - atbm8830: fix compile warning - sh_mobile_ceu_camera: fix compile warning - sh_mobile_ceu_camera: requires 2.6.32 as minimum kernel version. - meye/vino: fix compile warnings - decode_tm6000: fix compile warning - zr364xx: needs at least 2.6.19. This is the first round of fixes for compile warnings. I'm also going to upgrade to the latest binutils and gcc for doing the daily builds. It would be nice if the daily build would produce an OK again... Thanks, Hans diffstat: linux/drivers/media/dvb/dvb-usb/cxusb.c |2 ++ linux/drivers/media/dvb/frontends/atbm8830.c |4 +++- linux/drivers/media/video/cx18/cx18-av-core.c| 14 +++--- linux/drivers/media/video/cx18/cx18-driver.c |1 - linux/drivers/media/video/meye.c |4 linux/drivers/media/video/sh_mobile_ceu_camera.c |1 - linux/drivers/media/video/vino.c |4 linux/include/media/v4l2-subdev.h|2 +- v4l/versions.txt |7 ++- v4l2-apps/util/decode_tm6000.c |2 +- 10 files changed, 28 insertions(+), 13 deletions(-) -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
Hi Mauro, Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc for the following: - radio-typhoon: remove obsolete RADIO_TYPHOON_PROC_FS config option Thanks, Hans diffstat: Kconfig | 10 -- 1 file changed, 10 deletions(-) -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
CX24113 change: Acked-By: Patrick Boettcher On Fri, 7 Aug 2009, Mike Isely wrote: Acked-By: Mike Isely -Mike On Fri, 7 Aug 2009, Hans Verkuil wrote: Hi Mauro, Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc for the following: - pvrusb2: fix compile warning - cx24113: fix mips compiler warning - hdpvr: add missing initialization of current_norm - v4l2-ioctl: fix G_STD and G_PARM default handlers - v4l2-ctl: fix get/set-parm bugs and add get/set-output-parm support Thanks, Hans diffstat: linux/drivers/media/dvb/frontends/cx24113.c |6 + linux/drivers/media/video/hdpvr/hdpvr-video.c |2 linux/drivers/media/video/pvrusb2/pvrusb2-audio.c |5 - linux/drivers/media/video/v4l2-ioctl.c| 15 ++- v4l2-apps/util/v4l2-ctl.cpp | 98 +- 5 files changed, 101 insertions(+), 25 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
Acked-By: Mike Isely -Mike On Fri, 7 Aug 2009, Hans Verkuil wrote: > Hi Mauro, > > Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc for the > following: > > - pvrusb2: fix compile warning > - cx24113: fix mips compiler warning > - hdpvr: add missing initialization of current_norm > - v4l2-ioctl: fix G_STD and G_PARM default handlers > - v4l2-ctl: fix get/set-parm bugs and add get/set-output-parm support > > Thanks, > > Hans > > diffstat: > linux/drivers/media/dvb/frontends/cx24113.c |6 + > linux/drivers/media/video/hdpvr/hdpvr-video.c |2 > linux/drivers/media/video/pvrusb2/pvrusb2-audio.c |5 - > linux/drivers/media/video/v4l2-ioctl.c| 15 ++- > v4l2-apps/util/v4l2-ctl.cpp | 98 > +- > 5 files changed, 101 insertions(+), 25 deletions(-) > > -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
Hi Mauro, Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc for the following: - pvrusb2: fix compile warning - cx24113: fix mips compiler warning - hdpvr: add missing initialization of current_norm - v4l2-ioctl: fix G_STD and G_PARM default handlers - v4l2-ctl: fix get/set-parm bugs and add get/set-output-parm support Thanks, Hans diffstat: linux/drivers/media/dvb/frontends/cx24113.c |6 + linux/drivers/media/video/hdpvr/hdpvr-video.c |2 linux/drivers/media/video/pvrusb2/pvrusb2-audio.c |5 - linux/drivers/media/video/v4l2-ioctl.c| 15 ++- v4l2-apps/util/v4l2-ctl.cpp | 98 +- 5 files changed, 101 insertions(+), 25 deletions(-) -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
Am Dienstag, den 21.07.2009, 11:04 +0200 schrieb hermann pitton: > Hi, > > Am Dienstag, den 21.07.2009, 08:36 +0200 schrieb Hans Verkuil: > > Hi Mauro, > > > > Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc for the > > following: > > that link fails. You have it in > > http://linuxtv.org/hg/~hverkuil/v4l-dvb-strctrl > Oops, Mauro has it already. Hermann -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
Hi, Am Dienstag, den 21.07.2009, 08:36 +0200 schrieb Hans Verkuil: > Hi Mauro, > > Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc for the > following: that link fails. You have it in http://linuxtv.org/hg/~hverkuil/v4l-dvb-strctrl > - flexcop: fix compile error > > When this fix is committed I'll start another daily build run. > > Thanks, > > Hans > > diffstat: > flexcop-fe-tuner.c |2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Cheers, Hermann -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
Hi Mauro, Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc for the following: - flexcop: fix compile error When this fix is committed I'll start another daily build run. Thanks, Hans diffstat: flexcop-fe-tuner.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
On Monday 20 July 2009 15:04:31 Hans Verkuil wrote: > Hi Mauro, > > Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc for the > following: > > - mt312: fix build for kernels < 2.6.24 > - versions.txt: freezer routines appeared in 2.6.24, not 2.6.23 > > Thanks, > > Hans > > diffstat: > linux/drivers/media/dvb/frontends/mt312.c |1 + > v4l/versions.txt |6 -- > 2 files changed, 5 insertions(+), 2 deletions(-) > > Added this fix as well: - v4l2-ctl: fix broken camera control support. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
Hi Mauro, Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc for the following: - mt312: fix build for kernels < 2.6.24 - versions.txt: freezer routines appeared in 2.6.24, not 2.6.23 Thanks, Hans diffstat: linux/drivers/media/dvb/frontends/mt312.c |1 + v4l/versions.txt |6 -- 2 files changed, 5 insertions(+), 2 deletions(-) -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
> On Tuesday 23 June 2009 08:45:51 Hans Verkuil wrote: >> Hi Mauro, >> >> Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc for >> the >> following: >> >> - v4l2-spec: add missing V4L2_PIX_FMT_OV511 documentation. > > Added this patch: > > - v4l2-common: fix uninitialized variable > > Note that this variable was only uninitialized when compiled for kernels > older than 2.6.26. And this patch: - em28xx: enable new-style i2c API for kernels >= 2.6.26, not 2.6.22. This fixes the em28xx driver when v4l-dvb is compiled for kernels 2.6.22-2.6.25. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
On Tuesday 23 June 2009 08:45:51 Hans Verkuil wrote: > Hi Mauro, > > Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc for the > following: > > - v4l2-spec: add missing V4L2_PIX_FMT_OV511 documentation. Added this patch: - v4l2-common: fix uninitialized variable Note that this variable was only uninitialized when compiled for kernels older than 2.6.26. Regards, Hans > > Hans, when you modify the videodev2.h header you should also do a 'make spec' > to check whether the v4l2-spec still builds. It's easy to forget that, but > it's important to keep the spec up to date. > > Thanks, > > Hans > > diffstat: > pixfmt.sgml |7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) > -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
Hi Mauro, Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc for the following: - v4l2-spec: add missing V4L2_PIX_FMT_OV511 documentation. Hans, when you modify the videodev2.h header you should also do a 'make spec' to check whether the v4l2-spec still builds. It's easy to forget that, but it's important to keep the spec up to date. Thanks, Hans diffstat: pixfmt.sgml |7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
Em Sat, 20 Jun 2009 09:45:41 -0700 (PDT) Trent Piepho escreveu: > On Sat, 20 Jun 2009, Hans Verkuil wrote: > > - compat: fix __fls check for the arm architecture. > > This one isn't quite right. The __fls defined for arm in 2.6.27 (between > v2.6.26-7260-g0c65f45 and v2.6.28-rc6-187-g94fc733) isn't the same as the > __fls() used everwhere else in the kernel. This alternate fix should > work. > > 01/01: compat: Fix __fls for certain ARM kernels > http://linuxtv.org/hg/~tap/fix?cmd=changeset;node=518b7754cd3d Agreed. I cherry-picked Hans patch and merged ~tap/fix tree to fix the __fls(). Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
On Sat, 20 Jun 2009, Hans Verkuil wrote: > - compat: fix __fls check for the arm architecture. This one isn't quite right. The __fls defined for arm in 2.6.27 (between v2.6.26-7260-g0c65f45 and v2.6.28-rc6-187-g94fc733) isn't the same as the __fls() used everwhere else in the kernel. This alternate fix should work. 01/01: compat: Fix __fls for certain ARM kernels http://linuxtv.org/hg/~tap/fix?cmd=changeset;node=518b7754cd3d -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
Hi Mauro, Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc for the following: - ivtv/cx18: fix regression: class controls are no longer seen - v4l2-ctl: add modulator get/set options. - v4l2-spec: update VIDIOC_G/S_MODULATOR section. - compat: fix __fls check for the arm architecture. - smscoreapi: fix compile warning - v4l2-i2c-drv.h: add comment describing when not to use this header. - radio-tea5764: fix incorrect rxsubchans value - v4l2-spec: add missing documentation for the OV518 pixel format. - tcm825x: remove incorrect __exit_p wrapper - cx231xx: fix uninitialized variable. This replaces my previous v4l-dvb-misc pull request. The video_register_device patch in that older pull request is no longer present in this tree (instead I've posted a separate review request for that yesterday). I did add a few other trivial patches. Note that the first patch is a high prio bug as it is a 2.6.30 regression. Mike, once this is merged in the git tree this one can be queued for the 2.6.30 stable series. Thanks, Hans diffstat: linux/drivers/media/dvb/siano/smscoreapi.c |4 - linux/drivers/media/radio/radio-tea5764.c |4 - linux/drivers/media/video/cx18/cx18-controls.c |2 linux/drivers/media/video/cx231xx/cx231xx-avcore.c |2 linux/drivers/media/video/cx2341x.c|2 linux/drivers/media/video/ivtv/ivtv-controls.c |2 linux/drivers/media/video/tcm825x.c|4 - linux/include/media/v4l2-i2c-drv.h |5 + v4l/compat.h |3 v4l2-apps/util/v4l2-ctl.cpp| 78 + v4l2-spec/pixfmt.sgml |5 + v4l2-spec/vidioc-g-modulator.sgml | 13 +-- 12 files changed, 110 insertions(+), 14 deletions(-) -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
On Monday 15 June 2009 21:48:32 Mauro Carvalho Chehab wrote: > Hi Hans, > > Em Mon, 15 Jun 2009 13:27:42 +0200 > > Hans Verkuil escreveu: > > On Sunday 14 June 2009 12:15:34 Hans Verkuil wrote: > > > On Sunday 14 June 2009 11:50:51 Hans Verkuil wrote: > > > > Hi Mauro, > > > > > > > > Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc > > > > for the following: > > > > > > > > - ivtv/cx18: fix regression: class controls are no longer seen > > > > - v4l2-ctl: add modulator get/set options. > > > > - v4l2-spec: update VIDIOC_G/S_MODULATOR section. > > > > - compat: fix __fls check for the arm architecture. > > > > - smscoreapi: fix compile warning > > > > > > > > The first one is a high prio bug as it is a 2.6.30 regression. > > > > Mike, once this is merged in the git tree this one can be queued > > > > for the 2.6.30 stable series. > > > > > > > > The other changes are small stuff. > > > > > > Added one more minor change: > > > > > > - v4l2-i2c-drv.h: add comment describing when not to use this header. > > The above patches seem ok. Can you pull from this tree and just skip the last controversial patch? Or do you prefer me to redo this tree without that last patch? Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
On Tuesday 16 June 2009 02:03:54 Mauro Carvalho Chehab wrote:
> Em Mon, 15 Jun 2009 23:35:59 +0200
>
> Hans Verkuil escreveu:
> > > By looking on this patch, it is obfuscating the function even more.
> > > Creating more confusion on it, due to some reasons:
> > >
> > > 1) The name "kernel number" doesn't seem much appropriate. Any number
> > > used in "kernel" can be called as a "kernel number".
> >
> > Actually, that is apparently what the number X is called in a node like
> > /dev/videoX. I didn't make up that term, it's what I found when reading
> > 'man udev'. Since udev deals extensively with these concepts I borrowed
> > the udev terminology for this. If someone knows a better word, then I'm
> > happy to use that.
>
> Ah, now I see where you got this. Yet, this is what man udev says:
>
>$number, %n
> The kernel number for this device. For example, ’sda3’ has
> kernel number of ’3’
>
>$major, %M
> The kernel major number for the device.
>
>$minor %m
> The kernel minor number for the device.
>
> See, on all all tree is calls "kernel [] number". Seems
> confusing enough to avoid those names at the V4L2 core. Also, even the
> man needed to give an example, showing that this nomenclature may not be
> widely understood.
>
> Maybe we can just call it as "dev_number" and properly explain its
> meaning.
>
> > > That's said, the logic of when minor range is not fixed seems broken,
> > > as it will change only an internal representation of the device. So
> > > the module parameters that reflect at "nr" var won't work as
> > > expected.
> >
> > No, they work exactly as expected: if you set nr to 1 then you get a
> > /dev/video1 node no matter what the FIXED_MINOR_RANGES setting is
> > (provided there isn't already a /dev/video1 node, in which case it will
> > find the next available kernel number).
> >
> > > So, the current code seems too complex and broken.
> >
> > No, it's neither too complex nor broken, although it clearly needs
> > still more comments.
>
> The code is complex. For example, take a look at this:
>
> #ifdef CONFIG_VIDEO_FIXED_MINOR_RANGES
> ...
> #else
> /* The kernel number and minor numbers are independent */
> for (i = 0; i < VIDEO_NUM_DEVICES; i++)
> if (video_device[i] == NULL)
> break;
> if (i == VIDEO_NUM_DEVICES) {
> ..
> return -ENFILE;
> }
>
> #endif
>
> vdev->minor = i + minor_offset;
> ...
> /* Should not happen since we thought this minor was free */
> WARN_ON(video_device[vdev->minor] != NULL);
>
> when !FIXED_MINOR_RANGES, you're return if all video_device[i] are used.
> Then, you do a WARN_ON(video_device[i + minor_offset]) instead of
> video_device[i].
>
> People need to look back at the code to identify that minor_offset is
> equal to 0 when !FIXED_MINOR_RANGES.
>
> Also, by letting the function to allocate a device even when video_device
> != NULL seems broken. It should instead call WARN and return with
> -EINVAL.
That's better, yes. I remember that at the time I wasn't sure what to do
here and I agree I made the wrong choice.
>
> The presence of the #if's, plus the high number of "phases" at the same
> function make it harder to read and understand. The code will be more
> readable if we break it into a few static functions (that will be inline
> anyway, due to gcc optimizer).
>
> > > Just as reference, the behavior before changeset 9133 is:
> > >
> > > switch (type) {
> > > case VFL_TYPE_GRABBER:
> > >base = MINOR_VFL_TYPE_GRABBER_MIN;
> > > ...
> > > }
> > >
> > > i = base + nr;
> > > vfd->minor = i;
> > >
> > >
> > > where nr is auto-selected for negative values, or just used above
> > > otherwise.
> > >
> > > That's said, IMO, we need to rework at the function, making it
> > > simpler, and fixing the behavior where the user wants to select a
> > > minor.
> >
> > The user doesn't select a minor number, the user selects a kernel
> > number. With udev minor numbers have become completely uninteresting
> > and unless FIXED_MINOR_RANGES is set each node will be assigned the
> > first free minor number.
>
> OK, now I see what you're meaning.
>
> With respect to the code, I still think it does too much into just one
> function. It may make sense to break the code into more functions to
> allow an easier understanding.
I'll attempt that this weekend.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
Em Mon, 15 Jun 2009 23:35:59 +0200
Hans Verkuil escreveu:
> > By looking on this patch, it is obfuscating the function even more.
> > Creating more
> > confusion on it, due to some reasons:
> >
> > 1) The name "kernel number" doesn't seem much appropriate. Any number used
> > in
> > "kernel" can be called as a "kernel number".
>
> Actually, that is apparently what the number X is called in a node like
> /dev/videoX. I didn't make up that term, it's what I found when reading
> 'man udev'. Since udev deals extensively with these concepts I borrowed the
> udev terminology for this. If someone knows a better word, then I'm happy
> to use that.
Ah, now I see where you got this. Yet, this is what man udev says:
$number, %n
The kernel number for this device. For example, ’sda3’ has
kernel number of ’3’
$major, %M
The kernel major number for the device.
$minor %m
The kernel minor number for the device.
See, on all all tree is calls "kernel [] number". Seems confusing
enough to avoid those names at the V4L2 core. Also, even the man needed to give
an example, showing that this nomenclature may not be widely understood.
Maybe we can just call it as "dev_number" and properly explain its meaning.
> > That's said, the logic of when minor range is not fixed seems broken, as it
> > will change only an internal representation of the device. So the module
> > parameters that reflect at "nr" var won't work as expected.
>
> No, they work exactly as expected: if you set nr to 1 then you get a
> /dev/video1
> node no matter what the FIXED_MINOR_RANGES setting is (provided there isn't
> already a /dev/video1 node, in which case it will find the next available
> kernel number).
>
> > So, the current code seems too complex and broken.
>
> No, it's neither too complex nor broken, although it clearly needs still more
> comments.
The code is complex. For example, take a look at this:
#ifdef CONFIG_VIDEO_FIXED_MINOR_RANGES
...
#else
/* The kernel number and minor numbers are independent */
for (i = 0; i < VIDEO_NUM_DEVICES; i++)
if (video_device[i] == NULL)
break;
if (i == VIDEO_NUM_DEVICES) {
..
return -ENFILE;
}
#endif
vdev->minor = i + minor_offset;
...
/* Should not happen since we thought this minor was free */
WARN_ON(video_device[vdev->minor] != NULL);
when !FIXED_MINOR_RANGES, you're return if all video_device[i] are used. Then,
you do a WARN_ON(video_device[i + minor_offset]) instead of video_device[i].
People need to look back at the code to identify that minor_offset is equal to
0 when !FIXED_MINOR_RANGES.
Also, by letting the function to allocate a device even when video_device !=
NULL seems broken. It should instead call WARN and return with -EINVAL.
The presence of the #if's, plus the high number of "phases" at the same
function make it harder to read and understand. The code will be more readable
if we break it into a few static functions (that will be inline anyway, due to
gcc optimizer).
>
> > Just as reference, the behavior before changeset 9133 is:
> >
> > switch (type) {
> > case VFL_TYPE_GRABBER:
> >base = MINOR_VFL_TYPE_GRABBER_MIN;
> > ...
> > }
> >
> > i = base + nr;
> > vfd->minor = i;
> >
> >
> > where nr is auto-selected for negative values, or just used above otherwise.
> >
> > That's said, IMO, we need to rework at the function, making it simpler, and
> > fixing the behavior where the user wants to select a minor.
>
> The user doesn't select a minor number, the user selects a kernel number.
> With udev minor numbers have become completely uninteresting and unless
> FIXED_MINOR_RANGES is set each node will be assigned the first free minor
> number.
OK, now I see what you're meaning.
With respect to the code, I still think it does too much into just one
function. It may make sense to break the code into more functions to allow an
easier understanding.
Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
Hi,
Am Montag, den 15.06.2009, 23:35 +0200 schrieb Hans Verkuil:
> On Monday 15 June 2009 21:48:32 Mauro Carvalho Chehab wrote:
> > Hi Hans,
> >
> > Em Mon, 15 Jun 2009 13:27:42 +0200
> > Hans Verkuil escreveu:
> >
> > > On Sunday 14 June 2009 12:15:34 Hans Verkuil wrote:
> > > > On Sunday 14 June 2009 11:50:51 Hans Verkuil wrote:
> > > > > Hi Mauro,
> > > > >
> > > > > Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc for
> > > > > the
> > > > > following:
> > > > >
> > > > > - ivtv/cx18: fix regression: class controls are no longer seen
> > > > > - v4l2-ctl: add modulator get/set options.
> > > > > - v4l2-spec: update VIDIOC_G/S_MODULATOR section.
> > > > > - compat: fix __fls check for the arm architecture.
> > > > > - smscoreapi: fix compile warning
> > > > >
> > > > > The first one is a high prio bug as it is a 2.6.30 regression. Mike,
> > > > > once
> > > > > this is merged in the git tree this one can be queued for the 2.6.30
> > > > > stable
> > > > > series.
> > > > >
> > > > > The other changes are small stuff.
> > > >
> > > > Added one more minor change:
> > > >
> > > > - v4l2-i2c-drv.h: add comment describing when not to use this header.
> >
> > The above patches seem ok.
> >
> > >
> > > And added this one as well:
> > >
> > > - v4l2-dev: fix some confusing variable names and comments
> > >
> > > # HG changeset patch
> > > # User Hans Verkuil
> > > # Date 1245063581 -7200
> > > # Node ID 083bb5ad197e4c49430de2b26f3115743fe5cc27
> > > # Parent 743225159afab6d79a0d6095bc302f9922305c33
> > > v4l2-dev: fix some confusing variable names and comments
> > >
> > > From: Hans Verkuil
> > >
> > > Some variable names and comments were rather misleading when it came
> > > to the distinction between kernel numbers and minor numbers. This should
> > > clarify things.
> > >
> > > Priority: normal
> > >
> > > Signed-off-by: Hans Verkuil
> > >
> > > --- a/linux/drivers/media/video/v4l2-dev.cSun Jun 14 12:12:11
> > > 2009 +0200
> > > +++ b/linux/drivers/media/video/v4l2-dev.cMon Jun 15 12:59:41
> > > 2009 +0200
> > > @@ -419,10 +419,10 @@ int video_register_device_index(struct v
> > > int video_register_device_index(struct video_device *vdev, int type, int
> > > nr,
> > > int index)
> > > {
> > > - int i = 0;
> > > + int minor;
> > > int ret;
> > > int minor_offset = 0;
> > > - int minor_cnt = VIDEO_NUM_DEVICES;
> > > + int kernel_nrs = VIDEO_NUM_DEVICES;
> > > const char *name_base;
> > > void *priv = video_get_drvdata(vdev);
> > >
> > > @@ -470,52 +470,52 @@ int video_register_device_index(struct v
> > > switch (type) {
> > > case VFL_TYPE_GRABBER:
> > > minor_offset = 0;
> > > - minor_cnt = 64;
> > > + kernel_nrs = 64;
> > > break;
> > > case VFL_TYPE_RADIO:
> > > minor_offset = 64;
> > > - minor_cnt = 64;
> > > + kernel_nrs = 64;
> > > break;
> > > case VFL_TYPE_VTX:
> > > minor_offset = 192;
> > > - minor_cnt = 32;
> > > + kernel_nrs = 32;
> > > break;
> > > case VFL_TYPE_VBI:
> > > minor_offset = 224;
> > > - minor_cnt = 32;
> > > + kernel_nrs = 32;
> > > break;
> > > default:
> > > minor_offset = 128;
> > > - minor_cnt = 64;
> > > - break;
> > > - }
> > > -#endif
> > > -
> > > - /* Pick a minor number */
> > > + kernel_nrs = 64;
> > > + break;
> > > + }
> > > +#endif
> > > +
> > > + /* Pick a kernel number */
> > > mutex_lock(&videodev_lock);
> > > - nr = find_next_zero_bit(video_nums[type], minor_cnt, nr == -1 ? 0 : nr);
> > > - if (nr == minor_cnt)
> > > - nr = find_first_zero_bit(video_nums[type], minor_cnt);
> > > - if (nr == minor_cnt) {
> > > + nr = find_next_zero_bit(video_nums[type], kernel_nrs, nr == -1 ? 0 :
> > > nr);
> > > + if (nr == kernel_nrs)
> > > + nr = find_first_zero_bit(video_nums[type], kernel_nrs);
> > > + if (nr == kernel_nrs) {
> > > printk(KERN_ERR "could not get a free kernel number\n");
> > > mutex_unlock(&videodev_lock);
> > > return -ENFILE;
> > > }
> > > #ifdef CONFIG_VIDEO_FIXED_MINOR_RANGES
> > > /* 1-on-1 mapping of kernel number to minor number */
> > > - i = nr;
> > > + minor = nr;
> > > #else
> > > /* The kernel number and minor numbers are independent */
> > > - for (i = 0; i < VIDEO_NUM_DEVICES; i++)
> > > - if (video_device[i] == NULL)
> > > + for (minor = 0; minor < VIDEO_NUM_DEVICES; minor++)
> > > + if (video_device[minor] == NULL)
> > > break;
> > > - if (i == VIDEO_NUM_DEVICES) {
> > > + if (minor == VIDEO_NUM_DEVICES) {
> > > mutex_unlock(&videodev_lock);
> > > printk(KERN_ERR "could not get a free minor\n");
> > > return -ENFILE;
> > > }
> > > #endif
> > > - vdev->minor = i + minor_offset;
> > > + vdev->minor = minor + minor_offset;
>
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
On Monday 15 June 2009 21:48:32 Mauro Carvalho Chehab wrote:
> Hi Hans,
>
> Em Mon, 15 Jun 2009 13:27:42 +0200
> Hans Verkuil escreveu:
>
> > On Sunday 14 June 2009 12:15:34 Hans Verkuil wrote:
> > > On Sunday 14 June 2009 11:50:51 Hans Verkuil wrote:
> > > > Hi Mauro,
> > > >
> > > > Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc for
> > > > the
> > > > following:
> > > >
> > > > - ivtv/cx18: fix regression: class controls are no longer seen
> > > > - v4l2-ctl: add modulator get/set options.
> > > > - v4l2-spec: update VIDIOC_G/S_MODULATOR section.
> > > > - compat: fix __fls check for the arm architecture.
> > > > - smscoreapi: fix compile warning
> > > >
> > > > The first one is a high prio bug as it is a 2.6.30 regression. Mike,
> > > > once
> > > > this is merged in the git tree this one can be queued for the 2.6.30
> > > > stable
> > > > series.
> > > >
> > > > The other changes are small stuff.
> > >
> > > Added one more minor change:
> > >
> > > - v4l2-i2c-drv.h: add comment describing when not to use this header.
>
> The above patches seem ok.
>
> >
> > And added this one as well:
> >
> > - v4l2-dev: fix some confusing variable names and comments
> >
> > # HG changeset patch
> > # User Hans Verkuil
> > # Date 1245063581 -7200
> > # Node ID 083bb5ad197e4c49430de2b26f3115743fe5cc27
> > # Parent 743225159afab6d79a0d6095bc302f9922305c33
> > v4l2-dev: fix some confusing variable names and comments
> >
> > From: Hans Verkuil
> >
> > Some variable names and comments were rather misleading when it came
> > to the distinction between kernel numbers and minor numbers. This should
> > clarify things.
> >
> > Priority: normal
> >
> > Signed-off-by: Hans Verkuil
> >
> > --- a/linux/drivers/media/video/v4l2-dev.c Sun Jun 14 12:12:11 2009 +0200
> > +++ b/linux/drivers/media/video/v4l2-dev.c Mon Jun 15 12:59:41 2009 +0200
> > @@ -419,10 +419,10 @@ int video_register_device_index(struct v
> > int video_register_device_index(struct video_device *vdev, int type, int
> > nr,
> > int index)
> > {
> > - int i = 0;
> > + int minor;
> > int ret;
> > int minor_offset = 0;
> > - int minor_cnt = VIDEO_NUM_DEVICES;
> > + int kernel_nrs = VIDEO_NUM_DEVICES;
> > const char *name_base;
> > void *priv = video_get_drvdata(vdev);
> >
> > @@ -470,52 +470,52 @@ int video_register_device_index(struct v
> > switch (type) {
> > case VFL_TYPE_GRABBER:
> > minor_offset = 0;
> > - minor_cnt = 64;
> > + kernel_nrs = 64;
> > break;
> > case VFL_TYPE_RADIO:
> > minor_offset = 64;
> > - minor_cnt = 64;
> > + kernel_nrs = 64;
> > break;
> > case VFL_TYPE_VTX:
> > minor_offset = 192;
> > - minor_cnt = 32;
> > + kernel_nrs = 32;
> > break;
> > case VFL_TYPE_VBI:
> > minor_offset = 224;
> > - minor_cnt = 32;
> > + kernel_nrs = 32;
> > break;
> > default:
> > minor_offset = 128;
> > - minor_cnt = 64;
> > - break;
> > - }
> > -#endif
> > -
> > - /* Pick a minor number */
> > + kernel_nrs = 64;
> > + break;
> > + }
> > +#endif
> > +
> > + /* Pick a kernel number */
> > mutex_lock(&videodev_lock);
> > - nr = find_next_zero_bit(video_nums[type], minor_cnt, nr == -1 ? 0 : nr);
> > - if (nr == minor_cnt)
> > - nr = find_first_zero_bit(video_nums[type], minor_cnt);
> > - if (nr == minor_cnt) {
> > + nr = find_next_zero_bit(video_nums[type], kernel_nrs, nr == -1 ? 0 :
> > nr);
> > + if (nr == kernel_nrs)
> > + nr = find_first_zero_bit(video_nums[type], kernel_nrs);
> > + if (nr == kernel_nrs) {
> > printk(KERN_ERR "could not get a free kernel number\n");
> > mutex_unlock(&videodev_lock);
> > return -ENFILE;
> > }
> > #ifdef CONFIG_VIDEO_FIXED_MINOR_RANGES
> > /* 1-on-1 mapping of kernel number to minor number */
> > - i = nr;
> > + minor = nr;
> > #else
> > /* The kernel number and minor numbers are independent */
> > - for (i = 0; i < VIDEO_NUM_DEVICES; i++)
> > - if (video_device[i] == NULL)
> > + for (minor = 0; minor < VIDEO_NUM_DEVICES; minor++)
> > + if (video_device[minor] == NULL)
> > break;
> > - if (i == VIDEO_NUM_DEVICES) {
> > + if (minor == VIDEO_NUM_DEVICES) {
> > mutex_unlock(&videodev_lock);
> > printk(KERN_ERR "could not get a free minor\n");
> > return -ENFILE;
> > }
> > #endif
> > - vdev->minor = i + minor_offset;
> > + vdev->minor = minor + minor_offset;
> > vdev->num = nr;
> > set_bit(nr, video_nums[type]);
> > /* Should not happen since we thought this minor was free */
> >
>
> The progressive changes at video_register_device() created a mess on this
> function, that refle
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
Hi Hans,
Em Mon, 15 Jun 2009 13:27:42 +0200
Hans Verkuil escreveu:
> On Sunday 14 June 2009 12:15:34 Hans Verkuil wrote:
> > On Sunday 14 June 2009 11:50:51 Hans Verkuil wrote:
> > > Hi Mauro,
> > >
> > > Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc for the
> > > following:
> > >
> > > - ivtv/cx18: fix regression: class controls are no longer seen
> > > - v4l2-ctl: add modulator get/set options.
> > > - v4l2-spec: update VIDIOC_G/S_MODULATOR section.
> > > - compat: fix __fls check for the arm architecture.
> > > - smscoreapi: fix compile warning
> > >
> > > The first one is a high prio bug as it is a 2.6.30 regression. Mike, once
> > > this is merged in the git tree this one can be queued for the 2.6.30
> > > stable
> > > series.
> > >
> > > The other changes are small stuff.
> >
> > Added one more minor change:
> >
> > - v4l2-i2c-drv.h: add comment describing when not to use this header.
The above patches seem ok.
>
> And added this one as well:
>
> - v4l2-dev: fix some confusing variable names and comments
>
> # HG changeset patch
> # User Hans Verkuil
> # Date 1245063581 -7200
> # Node ID 083bb5ad197e4c49430de2b26f3115743fe5cc27
> # Parent 743225159afab6d79a0d6095bc302f9922305c33
> v4l2-dev: fix some confusing variable names and comments
>
> From: Hans Verkuil
>
> Some variable names and comments were rather misleading when it came
> to the distinction between kernel numbers and minor numbers. This should
> clarify things.
>
> Priority: normal
>
> Signed-off-by: Hans Verkuil
>
> --- a/linux/drivers/media/video/v4l2-dev.cSun Jun 14 12:12:11 2009 +0200
> +++ b/linux/drivers/media/video/v4l2-dev.cMon Jun 15 12:59:41 2009 +0200
> @@ -419,10 +419,10 @@ int video_register_device_index(struct v
> int video_register_device_index(struct video_device *vdev, int type, int nr,
> int index)
> {
> - int i = 0;
> + int minor;
> int ret;
> int minor_offset = 0;
> - int minor_cnt = VIDEO_NUM_DEVICES;
> + int kernel_nrs = VIDEO_NUM_DEVICES;
> const char *name_base;
> void *priv = video_get_drvdata(vdev);
>
> @@ -470,52 +470,52 @@ int video_register_device_index(struct v
> switch (type) {
> case VFL_TYPE_GRABBER:
> minor_offset = 0;
> - minor_cnt = 64;
> + kernel_nrs = 64;
> break;
> case VFL_TYPE_RADIO:
> minor_offset = 64;
> - minor_cnt = 64;
> + kernel_nrs = 64;
> break;
> case VFL_TYPE_VTX:
> minor_offset = 192;
> - minor_cnt = 32;
> + kernel_nrs = 32;
> break;
> case VFL_TYPE_VBI:
> minor_offset = 224;
> - minor_cnt = 32;
> + kernel_nrs = 32;
> break;
> default:
> minor_offset = 128;
> - minor_cnt = 64;
> - break;
> - }
> -#endif
> -
> - /* Pick a minor number */
> + kernel_nrs = 64;
> + break;
> + }
> +#endif
> +
> + /* Pick a kernel number */
> mutex_lock(&videodev_lock);
> - nr = find_next_zero_bit(video_nums[type], minor_cnt, nr == -1 ? 0 : nr);
> - if (nr == minor_cnt)
> - nr = find_first_zero_bit(video_nums[type], minor_cnt);
> - if (nr == minor_cnt) {
> + nr = find_next_zero_bit(video_nums[type], kernel_nrs, nr == -1 ? 0 :
> nr);
> + if (nr == kernel_nrs)
> + nr = find_first_zero_bit(video_nums[type], kernel_nrs);
> + if (nr == kernel_nrs) {
> printk(KERN_ERR "could not get a free kernel number\n");
> mutex_unlock(&videodev_lock);
> return -ENFILE;
> }
> #ifdef CONFIG_VIDEO_FIXED_MINOR_RANGES
> /* 1-on-1 mapping of kernel number to minor number */
> - i = nr;
> + minor = nr;
> #else
> /* The kernel number and minor numbers are independent */
> - for (i = 0; i < VIDEO_NUM_DEVICES; i++)
> - if (video_device[i] == NULL)
> + for (minor = 0; minor < VIDEO_NUM_DEVICES; minor++)
> + if (video_device[minor] == NULL)
> break;
> - if (i == VIDEO_NUM_DEVICES) {
> + if (minor == VIDEO_NUM_DEVICES) {
> mutex_unlock(&videodev_lock);
> printk(KERN_ERR "could not get a free minor\n");
> return -ENFILE;
> }
> #endif
> - vdev->minor = i + minor_offset;
> + vdev->minor = minor + minor_offset;
> vdev->num = nr;
> set_bit(nr, video_nums[type]);
> /* Should not happen since we thought this minor was free */
>
The progressive changes at video_register_device() created a mess on this
function, that reflected on ov511 driver, but it is also present on the others.
By looking on this patch, it is obfuscating the function even more. Creating
more
confusion on it, due to some reasons:
1) The name "kernel number" doesn't seem
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
On Sunday 14 June 2009 12:15:34 Hans Verkuil wrote: > On Sunday 14 June 2009 11:50:51 Hans Verkuil wrote: > > Hi Mauro, > > > > Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc for the > > following: > > > > - ivtv/cx18: fix regression: class controls are no longer seen > > - v4l2-ctl: add modulator get/set options. > > - v4l2-spec: update VIDIOC_G/S_MODULATOR section. > > - compat: fix __fls check for the arm architecture. > > - smscoreapi: fix compile warning > > > > The first one is a high prio bug as it is a 2.6.30 regression. Mike, once > > this is merged in the git tree this one can be queued for the 2.6.30 stable > > series. > > > > The other changes are small stuff. > > Added one more minor change: > > - v4l2-i2c-drv.h: add comment describing when not to use this header. And added this one as well: - v4l2-dev: fix some confusing variable names and comments Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
On Sunday 14 June 2009 11:50:51 Hans Verkuil wrote: > Hi Mauro, > > Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc for the > following: > > - ivtv/cx18: fix regression: class controls are no longer seen > - v4l2-ctl: add modulator get/set options. > - v4l2-spec: update VIDIOC_G/S_MODULATOR section. > - compat: fix __fls check for the arm architecture. > - smscoreapi: fix compile warning > > The first one is a high prio bug as it is a 2.6.30 regression. Mike, once > this is merged in the git tree this one can be queued for the 2.6.30 stable > series. > > The other changes are small stuff. Added one more minor change: - v4l2-i2c-drv.h: add comment describing when not to use this header. Regards, Hans > > Thanks, > > Hans > > diffstat: > linux/drivers/media/dvb/siano/smscoreapi.c |4 - > linux/drivers/media/video/cx18/cx18-controls.c |2 > linux/drivers/media/video/cx2341x.c|2 > linux/drivers/media/video/ivtv/ivtv-controls.c |2 > v4l/compat.h |3 > v4l2-apps/util/v4l2-ctl.cpp| 78 > + > v4l2-spec/vidioc-g-modulator.sgml | 13 ++-- > 7 files changed, 95 insertions(+), 9 deletions(-) > -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
Hi Mauro, Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc for the following: - ivtv/cx18: fix regression: class controls are no longer seen - v4l2-ctl: add modulator get/set options. - v4l2-spec: update VIDIOC_G/S_MODULATOR section. - compat: fix __fls check for the arm architecture. - smscoreapi: fix compile warning The first one is a high prio bug as it is a 2.6.30 regression. Mike, once this is merged in the git tree this one can be queued for the 2.6.30 stable series. The other changes are small stuff. Thanks, Hans diffstat: linux/drivers/media/dvb/siano/smscoreapi.c |4 - linux/drivers/media/video/cx18/cx18-controls.c |2 linux/drivers/media/video/cx2341x.c|2 linux/drivers/media/video/ivtv/ivtv-controls.c |2 v4l/compat.h |3 v4l2-apps/util/v4l2-ctl.cpp| 78 + v4l2-spec/vidioc-g-modulator.sgml | 13 ++-- 7 files changed, 95 insertions(+), 9 deletions(-) -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
