[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
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
Acked-By: Mike Isely is...@pobox.com -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: - 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
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
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
[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 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
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
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
Em Sat, 20 Jun 2009 09:45:41 -0700 (PDT) Trent Piepho xy...@speakeasy.org 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
[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 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
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 hverk...@xs4all.nl 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 hverk...@xs4all.nl 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 [something] 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
Hi Hans,
Em Mon, 15 Jun 2009 13:27:42 +0200
Hans Verkuil hverk...@xs4all.nl 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 hverk...@xs4all.nl
# Date 1245063581 -7200
# Node ID 083bb5ad197e4c49430de2b26f3115743fe5cc27
# Parent 743225159afab6d79a0d6095bc302f9922305c33
v4l2-dev: fix some confusing variable names and comments
From: Hans Verkuil hverk...@xs4all.nl
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 hverk...@xs4all.nl
--- 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 much appropriate. Any number used in
kernel can be called as a kernel number.
2) minor and major devices are
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 hverk...@xs4all.nl 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 hverk...@xs4all.nl
# Date 1245063581 -7200
# Node ID 083bb5ad197e4c49430de2b26f3115743fe5cc27
# Parent 743225159afab6d79a0d6095bc302f9922305c33
v4l2-dev: fix some confusing variable names and comments
From: Hans Verkuil hverk...@xs4all.nl
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 hverk...@xs4all.nl
--- 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 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 much appropriate. Any number used in
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 hverk...@xs4all.nl 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 hverk...@xs4all.nl
# Date 1245063581 -7200
# Node ID 083bb5ad197e4c49430de2b26f3115743fe5cc27
# Parent 743225159afab6d79a0d6095bc302f9922305c33
v4l2-dev: fix some confusing variable names and comments
From: Hans Verkuil hverk...@xs4all.nl
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 hverk...@xs4all.nl
--- 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
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
Em Mon, 15 Jun 2009 23:35:59 +0200
Hans Verkuil hverk...@xs4all.nl 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 [something] 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
[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
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
