Re: [RFCv1 PATCH 0/7] gspca: allow use of control framework and other fixes
Hi, On 05/01/2012 12:28 PM, Jean-Francois Moine wrote: On Mon, 30 Apr 2012 13:13:59 +0200 Hans de Goede wrote: I'll review this and add these to my tree. Jean-Francois, is it ok for these changes to go upstream through my tree? The reason I'm asking is that I plan to convert more subdrivers to the control framework for 3.5 and its easiest to have this all in one tree then. Hi Hans, As you know, I don't like them, but no matter, I am a bit tired about the webcams (4 years now) and I'd be glad to change for new applications (wayland, haiku, ARM?). I will be sad to see you stop maintaining gspca, but I can understand that after 4 years you want a change. Thanks for the great job you've done on gspca! So is there nothing we can do to change your mind? > Is anybody interested in maintaining the gspca stuff? I can take over gspca maintenance if you want me to. So if you're really going to stop maintaining gspca, may I suggest that you start playing with arm. The trimslice, which I've just bought myself :) I should get mine on Monday. It is a really nice machine for development (enough cpu power and ram to compile on the machine which is a lot easier then cross compilation). You can find the trimslice here: http://trimslice.com/web/ I'm going to work on getting Fedora to support the trimslice as good as possible. One interesting area where your hardware talents may be very useful is getting FOSS support for the NVidia Tegra 2 video core on the trimslice. There are already some people working on an opensource (kms) driver for it but I'm sure they could use some help. Regards, Hans -- 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: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
Hi,
I'm slowly working my way though this series today (both review, as well
as some tweaks and testing).
More comments inline...
On 04/28/2012 05:09 PM, Hans Verkuil wrote:
From: Hans Verkuil
Make the necessary changes to allow subdrivers to use the control framework.
This does not add control event support, that needs more work.
Signed-off-by: Hans Verkuil
---
drivers/media/video/gspca/gspca.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/media/video/gspca/gspca.c
b/drivers/media/video/gspca/gspca.c
index ca5a2b1..56dff10 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -38,6 +38,7 @@
#include
#include
#include
+#include
#include "gspca.h"
@@ -1006,6 +1007,8 @@ static void gspca_set_default_mode(struct gspca_dev
*gspca_dev)
/* set the current control values to their default values
* which may have changed in sd_init() */
+ /* does nothing if ctrl_handler == NULL */
+ v4l2_ctrl_handler_setup(gspca_dev->vdev.ctrl_handler);
ctrl = gspca_dev->cam.ctrls;
if (ctrl != NULL) {
for (i = 0;
@@ -1323,6 +1326,7 @@ static void gspca_release(struct video_device *vfd)
PDEBUG(D_PROBE, "%s released",
video_device_node_name(&gspca_dev->vdev));
+ v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
kfree(gspca_dev->usb_buf);
kfree(gspca_dev);
}
@@ -2347,6 +2351,10 @@ int gspca_dev_probe2(struct usb_interface *intf,
gspca_dev->sd_desc = sd_desc;
gspca_dev->nbufread = 2;
gspca_dev->empty_packet = -1;/* don't check the empty packets */
+ gspca_dev->vdev = gspca_template;
+ gspca_dev->vdev.parent =&intf->dev;
+ gspca_dev->module = module;
+ gspca_dev->present = 1;
/* configure the subdriver and initialize the USB device */
ret = sd_desc->config(gspca_dev, id);
You also need to move the initialization of the mutexes here, as the
v4l2_ctrl_handler_setup will call s_ctrl on all the controls, and s_ctrl
should take the usb_lock (see my review of the next patch in this series),
I'll make this change myself and merge it into your patch.
@@ -2368,10 +2376,6 @@ int gspca_dev_probe2(struct usb_interface *intf,
init_waitqueue_head(&gspca_dev->wq);
/* init video stuff */
- memcpy(&gspca_dev->vdev,&gspca_template, sizeof gspca_template);
- gspca_dev->vdev.parent =&intf->dev;
- gspca_dev->module = module;
- gspca_dev->present = 1;
ret = video_register_device(&gspca_dev->vdev,
VFL_TYPE_GRABBER,
-1);
@@ -2391,6 +2395,7 @@ out:
if (gspca_dev->input_dev)
input_unregister_device(gspca_dev->input_dev);
#endif
+ v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
kfree(gspca_dev->usb_buf);
kfree(gspca_dev);
return ret;
Otherwise looks good, I've added it to my local tree (with the
described change), and will include it in my next pullreq.
Regards,
Hans
--
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: [PATCH 0/3] gspca - ov534: saturation and hue (using fixp-arith.h)
On Mon, 30 Apr 2012 15:51:01 +0200 Antonio Ospite wrote: > On Mon, 23 Apr 2012 14:16:25 -0600 > Jonathan Corbet wrote: > > > On Mon, 23 Apr 2012 15:21:04 +0200 > > Antonio Ospite wrote: > > > > > Jonathan, maybe fixp_sin() and fixp_cos() can be used in > > > drivers/media/video/ov7670.c too where currently ov7670_sine() and > > > ov7670_cosine() are defined, but I didn't want to send a patch I could > > > not test. > > > > Seems like a good idea. No reason to have multiple such hacks in the > > kernel; I'll look at dumping the ov7670 version when I get a chance. That > > may not be all that soon, though; life is a bit challenging at the moment. > > > > One concern is that if we're going to add users to fixp-arith.h, some of > > it should maybe go to a C file. Otherwise we'll create duplicated copies > > of the cos_table array for each user. I'm not sure the functions need to > > be inline either; nobody expects cos() to be blindingly fast. > > > > Jonathan, does lib/fixp-arith.c sound OK to you? I'll put that on my > TODO, unless Johann wans to do some more rework; in any case I think we > can still merge this patchset like it is right now. > > Jean-Francois will you take care of that? > Or wait :) I'll send a v2, I think I should merge the current COLORS control used for SENSOR_OV767x and the SATURATION one I added for SENSOR_OV772x as they both are V4L2_CID_SATURATION. Thanks, Antonio -- Antonio Ospite http://ao2.it A: Because it messes up the order in which people normally read text. See http://en.wikipedia.org/wiki/Posting_style Q: Why is top-posting such a bad thing? pgp7OH4SEvDn8.pgp Description: PGP signature
Re: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
On Sat May 5 2012 09:43:01 Hans de Goede wrote:
> Hi,
>
> I'm slowly working my way though this series today (both review, as well
> as some tweaks and testing).
Thanks for that!
One note: I initialized the controls in sd_init. That's wrong, it should be
sd_config. sd_init is also called on resume, so that would initialize the
controls twice.
I'm working on this as well today, together with finishing the stv06xx and
mars conversion.
I will also do some suspend/resume testing today to check whether the controls
are put back correctly after a resume.
Regards,
Hans
>
> More comments inline...
>
> On 04/28/2012 05:09 PM, Hans Verkuil wrote:
> > From: Hans Verkuil
> >
> > Make the necessary changes to allow subdrivers to use the control framework.
> > This does not add control event support, that needs more work.
> >
> > Signed-off-by: Hans Verkuil
> > ---
> > drivers/media/video/gspca/gspca.c | 13 +
> > 1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/video/gspca/gspca.c
> > b/drivers/media/video/gspca/gspca.c
> > index ca5a2b1..56dff10 100644
> > --- a/drivers/media/video/gspca/gspca.c
> > +++ b/drivers/media/video/gspca/gspca.c
> > @@ -38,6 +38,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include "gspca.h"
> >
> > @@ -1006,6 +1007,8 @@ static void gspca_set_default_mode(struct gspca_dev
> > *gspca_dev)
> >
> > /* set the current control values to their default values
> > * which may have changed in sd_init() */
> > + /* does nothing if ctrl_handler == NULL */
> > + v4l2_ctrl_handler_setup(gspca_dev->vdev.ctrl_handler);
> > ctrl = gspca_dev->cam.ctrls;
> > if (ctrl != NULL) {
> > for (i = 0;
> > @@ -1323,6 +1326,7 @@ static void gspca_release(struct video_device *vfd)
> > PDEBUG(D_PROBE, "%s released",
> > video_device_node_name(&gspca_dev->vdev));
> >
> > + v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
> > kfree(gspca_dev->usb_buf);
> > kfree(gspca_dev);
> > }
> > @@ -2347,6 +2351,10 @@ int gspca_dev_probe2(struct usb_interface *intf,
> > gspca_dev->sd_desc = sd_desc;
> > gspca_dev->nbufread = 2;
> > gspca_dev->empty_packet = -1; /* don't check the empty packets */
> > + gspca_dev->vdev = gspca_template;
> > + gspca_dev->vdev.parent =&intf->dev;
> > + gspca_dev->module = module;
> > + gspca_dev->present = 1;
> >
> > /* configure the subdriver and initialize the USB device */
> > ret = sd_desc->config(gspca_dev, id);
>
> You also need to move the initialization of the mutexes here, as the
> v4l2_ctrl_handler_setup will call s_ctrl on all the controls, and s_ctrl
> should take the usb_lock (see my review of the next patch in this series),
> I'll make this change myself and merge it into your patch.
>
> > @@ -2368,10 +2376,6 @@ int gspca_dev_probe2(struct usb_interface *intf,
> > init_waitqueue_head(&gspca_dev->wq);
> >
> > /* init video stuff */
> > - memcpy(&gspca_dev->vdev,&gspca_template, sizeof gspca_template);
> > - gspca_dev->vdev.parent =&intf->dev;
> > - gspca_dev->module = module;
> > - gspca_dev->present = 1;
> > ret = video_register_device(&gspca_dev->vdev,
> > VFL_TYPE_GRABBER,
> > -1);
> > @@ -2391,6 +2395,7 @@ out:
> > if (gspca_dev->input_dev)
> > input_unregister_device(gspca_dev->input_dev);
> > #endif
> > + v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
> > kfree(gspca_dev->usb_buf);
> > kfree(gspca_dev);
> > return ret;
>
> Otherwise looks good, I've added it to my local tree (with the
> described change), and will include it in my next pullreq.
>
> Regards,
>
> Hans
>
--
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: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
On Sat May 5 2012 09:43:01 Hans de Goede wrote:
> Hi,
>
> I'm slowly working my way though this series today (both review, as well
> as some tweaks and testing).
>
> More comments inline...
>
> On 04/28/2012 05:09 PM, Hans Verkuil wrote:
> > From: Hans Verkuil
> >
> > Make the necessary changes to allow subdrivers to use the control framework.
> > This does not add control event support, that needs more work.
> >
> > Signed-off-by: Hans Verkuil
> > ---
> > drivers/media/video/gspca/gspca.c | 13 +
> > 1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/video/gspca/gspca.c
> > b/drivers/media/video/gspca/gspca.c
> > index ca5a2b1..56dff10 100644
> > --- a/drivers/media/video/gspca/gspca.c
> > +++ b/drivers/media/video/gspca/gspca.c
> > @@ -38,6 +38,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include "gspca.h"
> >
> > @@ -1006,6 +1007,8 @@ static void gspca_set_default_mode(struct gspca_dev
> > *gspca_dev)
> >
> > /* set the current control values to their default values
> > * which may have changed in sd_init() */
> > + /* does nothing if ctrl_handler == NULL */
> > + v4l2_ctrl_handler_setup(gspca_dev->vdev.ctrl_handler);
> > ctrl = gspca_dev->cam.ctrls;
> > if (ctrl != NULL) {
> > for (i = 0;
> > @@ -1323,6 +1326,7 @@ static void gspca_release(struct video_device *vfd)
> > PDEBUG(D_PROBE, "%s released",
> > video_device_node_name(&gspca_dev->vdev));
> >
> > + v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
> > kfree(gspca_dev->usb_buf);
> > kfree(gspca_dev);
> > }
> > @@ -2347,6 +2351,10 @@ int gspca_dev_probe2(struct usb_interface *intf,
> > gspca_dev->sd_desc = sd_desc;
> > gspca_dev->nbufread = 2;
> > gspca_dev->empty_packet = -1; /* don't check the empty packets */
> > + gspca_dev->vdev = gspca_template;
> > + gspca_dev->vdev.parent =&intf->dev;
> > + gspca_dev->module = module;
> > + gspca_dev->present = 1;
> >
> > /* configure the subdriver and initialize the USB device */
> > ret = sd_desc->config(gspca_dev, id);
>
> You also need to move the initialization of the mutexes here, as the
> v4l2_ctrl_handler_setup will call s_ctrl on all the controls, and s_ctrl
> should take the usb_lock (see my review of the next patch in this series),
> I'll make this change myself and merge it into your patch.
Looking at how usb_lock is used I am inclined to just set video_device->lock
to it and let the v4l2 core do all the locking for me, which will automatically
fix the missing s_ctrl lock too.
I've realized that there is a problem if you do your own locking *and* use the
control framework: if you need to set a control from within the driver, then
you do that using v4l2_ctrl_s_ctrl. But if s_ctrl has to take the driver's lock,
then you can't call v4l2_ctrl_s_ctrl with that lock already taken!
So you get:
vidioc_foo()
lock(mylock)
v4l2_ctrl_s_ctrl(ctrl, val)
s_ctrl(ctrl, val)
lock(mylock)
If the core takes care of locking then everything is fine.
All the current drivers that use v4l2_ctrl_g/s_ctrl use core locking. But this
can be a problem in the future. The only way to resolve this is to tell
v4l2-ioctl.c
about your own lock so it can take it for you when calling into the control
framework.
Regards,
Hans
--
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
[PATCH 1/1] v4l2: use __u32 rather than enums in ioctl() structs
From: Rémi Denis-Courmont
V4L2 uses the enum type in IOCTL arguments in IOCTLs that were defined until
the use of enum was considered less than ideal. Recently Rémi Denis-Courmont
brought up the issue by proposing a patch to convert the enums to unsigned:
http://www.spinics.net/lists/linux-media/msg46167.html>
This sparked a long discussion where another solution to the issue was
proposed: two sets of IOCTL structures, one with __u32 and the other with
enums, and conversion code between the two:
http://www.spinics.net/lists/linux-media/msg47168.html>
Both approaches implement a complete solution that resolves the problem. The
first one is simple but requires assuming enums and __u32 are the same in
size (so we won't break the ABI) while the second one is more complex and
less clean but does not require making that assumption.
The issue boils down to whether enums are fundamentally different from __u32
or not, and can the former be substituted by the latter. During the
discussion it was concluded that the __u32 has the same size as enums on all
archs Linux is supported: it has not been shown that replacing those enums
in IOCTL arguments would break neither source or binary compatibility. If no
such reason is found, just replacing the enums with __u32s is the way to go.
This is what this patch does. This patch is slightly different from Remi's
first RFC (link above): it uses __u32 instead of unsigned and also changes
the arguments of VIDIOC_G_PRIORITY and VIDIOC_S_PRIORITY.
Signed-off-by: Rémi Denis-Courmont
Signed-off-by: Sakari Ailus
---
include/linux/videodev2.h | 46 ++--
1 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index 5a09ac3..585e4b4 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -292,10 +292,10 @@ struct v4l2_pix_format {
__u32 width;
__u32 height;
__u32 pixelformat;
- enum v4l2_field field;
+ __u32 field;
__u32 bytesperline; /* for padding, zero if unused
*/
__u32 sizeimage;
- enum v4l2_colorspacecolorspace;
+ __u32 colorspace;
__u32 priv; /* private data, depends on
pixelformat */
};
@@ -432,7 +432,7 @@ struct v4l2_pix_format {
*/
struct v4l2_fmtdesc {
__u32 index; /* Format number */
- enum v4l2_buf_type type; /* buffer type*/
+ __u32 type; /* buffer type*/
__u32 flags;
__u8description[32]; /* Description string */
__u32 pixelformat; /* Format fourcc */
@@ -573,8 +573,8 @@ struct v4l2_jpegcompression {
*/
struct v4l2_requestbuffers {
__u32 count;
- enum v4l2_buf_type type;
- enum v4l2_memorymemory;
+ __u32 type;
+ __u32 memory;
__u32 reserved[2];
};
@@ -636,16 +636,16 @@ struct v4l2_plane {
*/
struct v4l2_buffer {
__u32 index;
- enum v4l2_buf_type type;
+ __u32 type;
__u32 bytesused;
__u32 flags;
- enum v4l2_field field;
+ __u32 field;
struct timeval timestamp;
struct v4l2_timecodetimecode;
__u32 sequence;
/* memory location */
- enum v4l2_memorymemory;
+ __u32 memory;
union {
__u32 offset;
unsigned long userptr;
@@ -708,7 +708,7 @@ struct v4l2_clip {
struct v4l2_window {
struct v4l2_rectw;
- enum v4l2_field field;
+ __u32 field;
__u32 chromakey;
struct v4l2_clip__user *clips;
__u32 clipcount;
@@ -745,14 +745,14 @@ struct v4l2_outputparm {
* I N P U T I M A G E C R O P P I N G
*/
struct v4l2_cropcap {
- enum v4l2_buf_type type;
+ __u32 type;
struct v4l2_rectbounds;
struct v4l2_rectdefrect;
struct v4l2_fract pixelaspect;
};
struct v4l2_crop {
- enum v4l2_buf_type type;
+ __u32 type;
struct v4l2_rectc;
};
@@ -1157,7 +1157,7 @@ enum v4l2_ctrl_type {
/* Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
struct v4l2_queryctrl {
__u32id;
- enum v4l2_ctrl_type type;
+ __u32type;
__u8 name[32]; /* Whatever */
__s32
Re: [PATCH 1/1] v4l2: use __u32 rather than enums in ioctl() structs
On Sat May 5 2012 12:25:45 Sakari Ailus wrote:
> From: Rémi Denis-Courmont
>
> V4L2 uses the enum type in IOCTL arguments in IOCTLs that were defined until
> the use of enum was considered less than ideal. Recently Rémi Denis-Courmont
> brought up the issue by proposing a patch to convert the enums to unsigned:
>
> http://www.spinics.net/lists/linux-media/msg46167.html>
>
> This sparked a long discussion where another solution to the issue was
> proposed: two sets of IOCTL structures, one with __u32 and the other with
> enums, and conversion code between the two:
>
> http://www.spinics.net/lists/linux-media/msg47168.html>
>
> Both approaches implement a complete solution that resolves the problem. The
> first one is simple but requires assuming enums and __u32 are the same in
> size (so we won't break the ABI) while the second one is more complex and
> less clean but does not require making that assumption.
>
> The issue boils down to whether enums are fundamentally different from __u32
> or not, and can the former be substituted by the latter. During the
> discussion it was concluded that the __u32 has the same size as enums on all
> archs Linux is supported: it has not been shown that replacing those enums
> in IOCTL arguments would break neither source or binary compatibility. If no
> such reason is found, just replacing the enums with __u32s is the way to go.
>
> This is what this patch does. This patch is slightly different from Remi's
> first RFC (link above): it uses __u32 instead of unsigned and also changes
> the arguments of VIDIOC_G_PRIORITY and VIDIOC_S_PRIORITY.
>
> Signed-off-by: Rémi Denis-Courmont
> Signed-off-by: Sakari Ailus
> ---
> include/linux/videodev2.h | 46 ++--
> 1 files changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index 5a09ac3..585e4b4 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -292,10 +292,10 @@ struct v4l2_pix_format {
> __u32 width;
> __u32 height;
> __u32 pixelformat;
> - enum v4l2_field field;
> + __u32 field;
One suggestion: add a comment like this:
__u32 field; /* refer to enum v4l2_field */
This keeps the link between the u32 and the enum values.
Note that the DocBook documentation also has to be updated.
Looks good to me otherwise.
Regards,
Hans
> __u32 bytesperline; /* for padding, zero if unused
> */
> __u32 sizeimage;
> - enum v4l2_colorspacecolorspace;
> + __u32 colorspace;
> __u32 priv; /* private data, depends on
> pixelformat */
> };
>
> @@ -432,7 +432,7 @@ struct v4l2_pix_format {
> */
> struct v4l2_fmtdesc {
> __u32 index; /* Format number */
> - enum v4l2_buf_type type; /* buffer type*/
> + __u32 type; /* buffer type*/
> __u32 flags;
> __u8description[32]; /* Description string */
> __u32 pixelformat; /* Format fourcc */
> @@ -573,8 +573,8 @@ struct v4l2_jpegcompression {
> */
> struct v4l2_requestbuffers {
> __u32 count;
> - enum v4l2_buf_type type;
> - enum v4l2_memorymemory;
> + __u32 type;
> + __u32 memory;
> __u32 reserved[2];
> };
>
> @@ -636,16 +636,16 @@ struct v4l2_plane {
> */
> struct v4l2_buffer {
> __u32 index;
> - enum v4l2_buf_type type;
> + __u32 type;
> __u32 bytesused;
> __u32 flags;
> - enum v4l2_field field;
> + __u32 field;
> struct timeval timestamp;
> struct v4l2_timecodetimecode;
> __u32 sequence;
>
> /* memory location */
> - enum v4l2_memorymemory;
> + __u32 memory;
> union {
> __u32 offset;
> unsigned long userptr;
> @@ -708,7 +708,7 @@ struct v4l2_clip {
>
> struct v4l2_window {
> struct v4l2_rectw;
> - enum v4l2_field field;
> + __u32 field;
> __u32 chromakey;
> struct v4l2_clip__user *clips;
> __u32 clipcount;
> @@ -745,14 +745,14 @@ struct v4l2_outputparm {
> * I N P U T I M A G E C R O P P I N G
> */
> struct v4l2_cropcap {
> - enum v4l2_buf_type type;
> + __u32 type;
> struct v4l2_rectbounds;
> struct v4l2_rectdefrect;
> struct v4l2_fract pixelaspect
Re: [PATCH 1/1] v4l2: use __u32 rather than enums in ioctl() structs
Hi Hans,
On Sat, May 05, 2012 at 12:55:01PM +0200, Hans Verkuil wrote:
> On Sat May 5 2012 12:25:45 Sakari Ailus wrote:
> > From: Rémi Denis-Courmont
> >
> > V4L2 uses the enum type in IOCTL arguments in IOCTLs that were defined until
> > the use of enum was considered less than ideal. Recently Rémi Denis-Courmont
> > brought up the issue by proposing a patch to convert the enums to unsigned:
> >
> > http://www.spinics.net/lists/linux-media/msg46167.html>
> >
> > This sparked a long discussion where another solution to the issue was
> > proposed: two sets of IOCTL structures, one with __u32 and the other with
> > enums, and conversion code between the two:
> >
> > http://www.spinics.net/lists/linux-media/msg47168.html>
> >
> > Both approaches implement a complete solution that resolves the problem. The
> > first one is simple but requires assuming enums and __u32 are the same in
> > size (so we won't break the ABI) while the second one is more complex and
> > less clean but does not require making that assumption.
> >
> > The issue boils down to whether enums are fundamentally different from __u32
> > or not, and can the former be substituted by the latter. During the
> > discussion it was concluded that the __u32 has the same size as enums on all
> > archs Linux is supported: it has not been shown that replacing those enums
> > in IOCTL arguments would break neither source or binary compatibility. If no
> > such reason is found, just replacing the enums with __u32s is the way to go.
> >
> > This is what this patch does. This patch is slightly different from Remi's
> > first RFC (link above): it uses __u32 instead of unsigned and also changes
> > the arguments of VIDIOC_G_PRIORITY and VIDIOC_S_PRIORITY.
> >
> > Signed-off-by: Rémi Denis-Courmont
> > Signed-off-by: Sakari Ailus
> > ---
> > include/linux/videodev2.h | 46
> > ++--
> > 1 files changed, 23 insertions(+), 23 deletions(-)
> >
> > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> > index 5a09ac3..585e4b4 100644
> > --- a/include/linux/videodev2.h
> > +++ b/include/linux/videodev2.h
> > @@ -292,10 +292,10 @@ struct v4l2_pix_format {
> > __u32 width;
> > __u32 height;
> > __u32 pixelformat;
> > - enum v4l2_field field;
> > + __u32 field;
>
> One suggestion: add a comment like this:
>
> __u32 field; /* refer to enum v4l2_field */
>
> This keeps the link between the u32 and the enum values.
>
> Note that the DocBook documentation also has to be updated.
>
> Looks good to me otherwise.
Thanks for the comments. I'll make the above changes.
Cheers,
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi jabber/XMPP/Gmail: sai...@retiisi.org.uk
--
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: [PATCH 1/1] v4l2: use __u32 rather than enums in ioctl() structs
On Sat May 5 2012 13:14:10 Sakari Ailus wrote:
> Hi Hans,
>
> On Sat, May 05, 2012 at 12:55:01PM +0200, Hans Verkuil wrote:
> > On Sat May 5 2012 12:25:45 Sakari Ailus wrote:
> > > From: Rémi Denis-Courmont
> > >
> > > V4L2 uses the enum type in IOCTL arguments in IOCTLs that were defined
> > > until
> > > the use of enum was considered less than ideal. Recently Rémi
> > > Denis-Courmont
> > > brought up the issue by proposing a patch to convert the enums to
> > > unsigned:
> > >
> > > http://www.spinics.net/lists/linux-media/msg46167.html>
> > >
> > > This sparked a long discussion where another solution to the issue was
> > > proposed: two sets of IOCTL structures, one with __u32 and the other with
> > > enums, and conversion code between the two:
> > >
> > > http://www.spinics.net/lists/linux-media/msg47168.html>
> > >
> > > Both approaches implement a complete solution that resolves the problem.
> > > The
> > > first one is simple but requires assuming enums and __u32 are the same in
> > > size (so we won't break the ABI) while the second one is more complex and
> > > less clean but does not require making that assumption.
> > >
> > > The issue boils down to whether enums are fundamentally different from
> > > __u32
> > > or not, and can the former be substituted by the latter. During the
> > > discussion it was concluded that the __u32 has the same size as enums on
> > > all
> > > archs Linux is supported: it has not been shown that replacing those enums
> > > in IOCTL arguments would break neither source or binary compatibility. If
> > > no
> > > such reason is found, just replacing the enums with __u32s is the way to
> > > go.
> > >
> > > This is what this patch does. This patch is slightly different from Remi's
> > > first RFC (link above): it uses __u32 instead of unsigned and also changes
> > > the arguments of VIDIOC_G_PRIORITY and VIDIOC_S_PRIORITY.
> > >
> > > Signed-off-by: Rémi Denis-Courmont
> > > Signed-off-by: Sakari Ailus
> > > ---
> > > include/linux/videodev2.h | 46
> > > ++--
> > > 1 files changed, 23 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> > > index 5a09ac3..585e4b4 100644
> > > --- a/include/linux/videodev2.h
> > > +++ b/include/linux/videodev2.h
> > > @@ -292,10 +292,10 @@ struct v4l2_pix_format {
> > > __u32 width;
> > > __u32 height;
> > > __u32 pixelformat;
> > > - enum v4l2_field field;
> > > + __u32 field;
> >
> > One suggestion: add a comment like this:
> >
> > __u32 field; /* refer to enum v4l2_field */
> >
> > This keeps the link between the u32 and the enum values.
> >
> > Note that the DocBook documentation also has to be updated.
And v4l2-compat-ioctl32 as well! :-) That too has enums in the compat structs.
Regards,
Hans
> >
> > Looks good to me otherwise.
>
> Thanks for the comments. I'll make the above changes.
>
> Cheers,
>
>
--
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: [media-ctl PATCH 1/3] Support selections API for crop
Hi Sakari,
Thanks for the patch.
On Friday 04 May 2012 11:24:41 Sakari Ailus wrote:
> Support the new selections API for crop. Fall back to use the old crop API
> in case the selection API isn't available.
Thanks for the patch. A few minor comments below. There's no need to resubmit,
I've fixed the problems and applied the patch.
> Signed-off-by: Sakari Ailus
> ---
> src/main.c |4 ++-
> src/v4l2subdev.c | 100 +++
> src/v4l2subdev.h | 37 +++-
> 3 files changed, 93 insertions(+), 48 deletions(-)
[snip]
> diff --git a/src/v4l2subdev.c b/src/v4l2subdev.c
> index b886b72..92360bb 100644
> --- a/src/v4l2subdev.c
> +++ b/src/v4l2subdev.c
> @@ -104,48 +104,85 @@ int v4l2_subdev_set_format(struct media_entity
> *entity, return 0;
> }
>
> -int v4l2_subdev_get_crop(struct media_entity *entity, struct v4l2_rect
> *rect,
> - unsigned int pad, enum v4l2_subdev_format_whence which)
> +int v4l2_subdev_get_selection(
> + struct media_entity *entity, struct v4l2_rect *r,
> + unsigned int pad, int target, enum v4l2_subdev_format_whence which)
Let's make target an unsigned int.
> {
> - struct v4l2_subdev_crop crop;
> + union {
> + struct v4l2_subdev_selection sel;
> + struct v4l2_subdev_crop crop;
> + } u;
> int ret;
>
> ret = v4l2_subdev_open(entity);
> if (ret < 0)
> return ret;
>
> - memset(&crop, 0, sizeof(crop));
> - crop.pad = pad;
> - crop.which = which;
> + memset(&u.sel, 0, sizeof(u.sel));
> + u.sel.pad = pad;
> + u.sel.target = target;
> + u.sel.which = which;
>
> - ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_CROP, &crop);
> + ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_SELECTION, &u.sel);
> + if (ret >= 0) {
> + *r = u.sel.r;
> + return 0;
> + }
> + if (errno != ENOTTY
> + || target != V4L2_SUBDEV_SEL_TGT_CROP_ACTUAL)
No need to split the line :-)
> + return -errno;
> +
> + memset(&u.crop, 0, sizeof(u.crop));
> + u.crop.pad = pad;
> + u.crop.which = which;
> +
> + ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_CROP, &u.crop);
> if (ret < 0)
> return -errno;
>
> - *rect = crop.rect;
> + *r = u.crop.rect;
> return 0;
> }
[snip]
> @@ -355,30 +392,31 @@ static int set_format(struct media_pad *pad,
> return 0;
> }
>
> -static int set_crop(struct media_pad *pad, struct v4l2_rect *crop)
> +static int set_selection(struct media_pad *pad, int tgt,
unsigned int here as well.
> + struct v4l2_rect *rect)
[snip]
> @@ -429,18 +467,18 @@ static int v4l2_subdev_parse_setup_format(struct
> media_device *media, return -EINVAL;
> }
>
> - if (pad->flags & MEDIA_PAD_FL_SOURCE) {
> - ret = set_crop(pad, &crop);
> + if (pad->flags & MEDIA_PAD_FL_SINK) {
> + ret = set_format(pad, &format);
> if (ret < 0)
> return ret;
> }
>
> - ret = set_format(pad, &format);
> + ret = set_selection(pad, V4L2_SUBDEV_SEL_TGT_CROP_ACTUAL, &crop);
> if (ret < 0)
> return ret;
>
> - if (pad->flags & MEDIA_PAD_FL_SINK) {
> - ret = set_crop(pad, &crop);
> + if (pad->flags & MEDIA_PAD_FL_SOURCE) {
> + ret = set_format(pad, &format);
> if (ret < 0)
> return ret;
> }
I would just replace set_crop with set_selection here, and apply the rest of
the change in patch 3/3.
> diff --git a/src/v4l2subdev.h b/src/v4l2subdev.h
> index 1e75f94..1020747 100644
> --- a/src/v4l2subdev.h
> +++ b/src/v4l2subdev.h
> @@ -88,34 +88,38 @@ int v4l2_subdev_set_format(struct media_entity *entity,
> enum v4l2_subdev_format_whence which);
>
> /**
> - * @brief Retrieve the crop rectangle on a pad.
> + * @brief Retrieve a selection rectangle on a pad.
> * @param entity - subdev-device media entity.
> - * @param rect - crop rectangle to be filled.
> + * @param r - rectangle to be filled.
> * @param pad - pad number.
> + * @param target - selection target
> * @param which - identifier of the format to get.
> *
> - * Retrieve the current crop rectangleon the @a entity @a pad and store it
> in
> - * the @a rect structure.
> + * Retrieve the @a target selection rectangle on the @a entity @a pad
> + * and store it in the @a rect structure.
'@a rect' doesn't match '@param r' (same for set_selection).
> *
> - * @a which is set to V4L2_SUBDEV_FORMAT_TRY to retrieve the try crop
> rectangle
> - * stored in the file handle, of V4L2_SUBDEV_FORMAT_ACTIVE to retrieve the
> - * current active crop rectangle.
> + * @a which is set to V4L2_SUBDEV_FORMAT_TRY to retrieve the try
> + * selection rectangle stored in the file handle, of
s/of/or/ (the typo was already there, but let's fix it).
> + * V4L2_SUBDEV_FORMAT_ACTIVE to retrieve the current activ
Re: [RFC PATCH 1/4] si470x: Clean up, introduce the control framework.
Hello Hans,
thanks for the improvements. Looks good to me.
Acked-by: Tobias Lorenz
Bye,
Toby
Am Freitag, 4. Mai 2012, 15:30:29 schrieb Hans Verkuil:
> From: Hans Verkuil
>
> This cleans up the code and si470x now uses the proper v4l2 frameworks
> and passes most of the v4l2-compliance tests.
>
> Signed-off-by: Hans Verkuil
> ---
> drivers/media/radio/si470x/radio-si470x-common.c | 193
> +++--- drivers/media/radio/si470x/radio-si470x-i2c.c|
> 65 ++--
> drivers/media/radio/si470x/radio-si470x-usb.c| 146 +++-
> drivers/media/radio/si470x/radio-si470x.h| 14 +-
> 4 files changed, 105 insertions(+), 313 deletions(-)
>
> diff --git a/drivers/media/radio/si470x/radio-si470x-common.c
> b/drivers/media/radio/si470x/radio-si470x-common.c index 0e740c9..de9475f
> 100644
> --- a/drivers/media/radio/si470x/radio-si470x-common.c
> +++ b/drivers/media/radio/si470x/radio-si470x-common.c
> @@ -196,9 +196,9 @@ static int si470x_set_chan(struct si470x_device *radio,
> unsigned short chan) }
>
> if ((radio->registers[STATUSRSSI] & STATUSRSSI_STC) == 0)
> - dev_warn(&radio->videodev->dev, "tune does not complete\n");
> + dev_warn(&radio->videodev.dev, "tune does not complete\n");
> if (timed_out)
> - dev_warn(&radio->videodev->dev,
> + dev_warn(&radio->videodev.dev,
> "tune timed out after %u ms\n", tune_timeout);
>
> stop:
> @@ -344,12 +344,12 @@ static int si470x_set_seek(struct si470x_device
> *radio, }
>
> if ((radio->registers[STATUSRSSI] & STATUSRSSI_STC) == 0)
> - dev_warn(&radio->videodev->dev, "seek does not complete\n");
> + dev_warn(&radio->videodev.dev, "seek does not complete\n");
> if (radio->registers[STATUSRSSI] & STATUSRSSI_SF)
> - dev_warn(&radio->videodev->dev,
> + dev_warn(&radio->videodev.dev,
> "seek failed / band limit reached\n");
> if (timed_out)
> - dev_warn(&radio->videodev->dev,
> + dev_warn(&radio->videodev.dev,
> "seek timed out after %u ms\n", seek_timeout);
>
> stop:
> @@ -463,7 +463,6 @@ static ssize_t si470x_fops_read(struct file *file, char
> __user *buf, unsigned int block_count = 0;
>
> /* switch on rds reception */
> - mutex_lock(&radio->lock);
> if ((radio->registers[SYSCONFIG1] & SYSCONFIG1_RDS) == 0)
> si470x_rds_on(radio);
>
> @@ -505,7 +504,6 @@ static ssize_t si470x_fops_read(struct file *file, char
> __user *buf, }
>
> done:
> - mutex_unlock(&radio->lock);
> return retval;
> }
>
> @@ -521,10 +519,8 @@ static unsigned int si470x_fops_poll(struct file
> *file,
>
> /* switch on rds reception */
>
> - mutex_lock(&radio->lock);
> if ((radio->registers[SYSCONFIG1] & SYSCONFIG1_RDS) == 0)
> si470x_rds_on(radio);
> - mutex_unlock(&radio->lock);
>
> poll_wait(file, &radio->read_queue, pts);
>
> @@ -553,134 +549,27 @@ static const struct v4l2_file_operations si470x_fops
> = { * Video4Linux Interface
>
> **
> /
>
> -/*
> - * si470x_vidioc_queryctrl - enumerate control items
> - */
> -static int si470x_vidioc_queryctrl(struct file *file, void *priv,
> - struct v4l2_queryctrl *qc)
> -{
> - struct si470x_device *radio = video_drvdata(file);
> - int retval = -EINVAL;
> -
> - /* abort if qc->id is below V4L2_CID_BASE */
> - if (qc->id < V4L2_CID_BASE)
> - goto done;
> -
> - /* search video control */
> - switch (qc->id) {
> - case V4L2_CID_AUDIO_VOLUME:
> - return v4l2_ctrl_query_fill(qc, 0, 15, 1, 15);
> - case V4L2_CID_AUDIO_MUTE:
> - return v4l2_ctrl_query_fill(qc, 0, 1, 1, 1);
> - }
>
> - /* disable unsupported base controls */
> - /* to satisfy kradio and such apps */
> - if ((retval == -EINVAL) && (qc->id < V4L2_CID_LASTP1)) {
> - qc->flags = V4L2_CTRL_FLAG_DISABLED;
> - retval = 0;
> - }
> -
> -done:
> - if (retval < 0)
> - dev_warn(&radio->videodev->dev,
> - "query controls failed with %d\n", retval);
> - return retval;
> -}
> -
> -
> -/*
> - * si470x_vidioc_g_ctrl - get the value of a control
> - */
> -static int si470x_vidioc_g_ctrl(struct file *file, void *priv,
> - struct v4l2_control *ctrl)
> +static int si470x_s_ctrl(struct v4l2_ctrl *ctrl)
> {
> - struct si470x_device *radio = video_drvdata(file);
> - int retval = 0;
> -
> - mutex_lock(&radio->lock);
> - /* safety checks */
> - retval = si470x_disconnect_check(radio);
> - if (retval)
> - goto done;
> -
> - switch (ctrl->id) {
> - case V4L2_CID_AUDIO_VOLUME:
> - ctrl->value = radio->registers[SYSCONFIG2] &
> - SYSCONFIG2_VOLUME;
> -
Re: [RFC PATCH 2/4] si470x: add control event support and more v4l2 compliancy fixes.
Hello Hans,
thanks for the improvements. Looks good to me.
Acked-by: Tobias Lorenz
Bye,
Toby
Am Freitag, 4. Mai 2012, 15:30:30 schrieb Hans Verkuil:
> From: Hans Verkuil
>
> Signed-off-by: Hans Verkuil
> ---
> drivers/media/radio/si470x/radio-si470x-common.c | 45
> ++ 1 file changed, 28 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/media/radio/si470x/radio-si470x-common.c
> b/drivers/media/radio/si470x/radio-si470x-common.c index de9475f..e70badf
> 100644
> --- a/drivers/media/radio/si470x/radio-si470x-common.c
> +++ b/drivers/media/radio/si470x/radio-si470x-common.c
> @@ -262,7 +262,7 @@ static int si470x_get_freq(struct si470x_device *radio,
> unsigned int *freq) */
> int si470x_set_freq(struct si470x_device *radio, unsigned int freq)
> {
> - unsigned int spacing, band_bottom;
> + unsigned int spacing, band_bottom, band_top;
> unsigned short chan;
>
> /* Spacing (kHz) */
> @@ -278,19 +278,26 @@ int si470x_set_freq(struct si470x_device *radio,
> unsigned int freq) spacing = 0.050 * FREQ_MUL; break;
> };
>
> - /* Bottom of Band (MHz) */
> + /* Bottom/Top of Band (MHz) */
> switch ((radio->registers[SYSCONFIG2] & SYSCONFIG2_BAND) >> 6) {
> /* 0: 87.5 - 108 MHz (USA, Europe) */
> case 0:
> - band_bottom = 87.5 * FREQ_MUL; break;
> + band_bottom = 87.5 * FREQ_MUL;
> + band_top = 108 * FREQ_MUL;
> + break;
> /* 1: 76 - 108 MHz (Japan wide band) */
> default:
> - band_bottom = 76 * FREQ_MUL; break;
> + band_bottom = 76 * FREQ_MUL;
> + band_top = 108 * FREQ_MUL;
> + break;
> /* 2: 76 - 90 MHz (Japan) */
> case 2:
> - band_bottom = 76 * FREQ_MUL; break;
> + band_bottom = 76 * FREQ_MUL;
> + band_top = 90 * FREQ_MUL;
> + break;
> };
>
> + freq = clamp(freq, band_bottom, band_top);
> /* Chan = [ Freq (Mhz) - Bottom of Band (MHz) ] / Spacing (kHz) */
> chan = (freq - band_bottom) / spacing;
>
> @@ -515,17 +522,19 @@ static unsigned int si470x_fops_poll(struct file
> *file, struct poll_table_struct *pts)
> {
> struct si470x_device *radio = video_drvdata(file);
> - int retval = 0;
> -
> - /* switch on rds reception */
> + unsigned long req_events = poll_requested_events(pts);
> + int retval = v4l2_ctrl_poll(file, pts);
>
> - if ((radio->registers[SYSCONFIG1] & SYSCONFIG1_RDS) == 0)
> - si470x_rds_on(radio);
> + if (req_events & (POLLIN | POLLRDNORM)) {
> + /* switch on rds reception */
> + if ((radio->registers[SYSCONFIG1] & SYSCONFIG1_RDS) == 0)
> + si470x_rds_on(radio);
>
> - poll_wait(file, &radio->read_queue, pts);
> + poll_wait(file, &radio->read_queue, pts);
>
> - if (radio->rd_index != radio->wr_index)
> - retval = POLLIN | POLLRDNORM;
> + if (radio->rd_index != radio->wr_index)
> + retval |= POLLIN | POLLRDNORM;
> + }
>
> return retval;
> }
> @@ -637,6 +646,8 @@ static int si470x_vidioc_g_tuner(struct file *file,
> void *priv, tuner->signal = (radio->registers[STATUSRSSI] &
> STATUSRSSI_RSSI); /* the ideal factor is 0x/75 = 873,8 */
> tuner->signal = (tuner->signal * 873) + (8 * tuner->signal / 10);
> + if (tuner->signal > 0x)
> + tuner->signal = 0x;
>
> /* automatic frequency control: -1: freq to low, 1 freq to high */
> /* AFCRL does only indicate that freq. differs, not if too low/high */
> @@ -660,7 +671,7 @@ static int si470x_vidioc_s_tuner(struct file *file,
> void *priv, int retval = 0;
>
> if (tuner->index != 0)
> - goto done;
> + return -EINVAL;
>
> /* mono/stereo selector */
> switch (tuner->audmode) {
> @@ -668,15 +679,13 @@ static int si470x_vidioc_s_tuner(struct file *file,
> void *priv, radio->registers[POWERCFG] |= POWERCFG_MONO; /* force mono */
> break;
> case V4L2_TUNER_MODE_STEREO:
> + default:
> radio->registers[POWERCFG] &= ~POWERCFG_MONO; /* try stereo */
> break;
> - default:
> - goto done;
> }
>
> retval = si470x_set_register(radio, POWERCFG);
>
> -done:
> if (retval < 0)
> dev_warn(&radio->videodev.dev,
> "set tuner failed with %d\n", retval);
> @@ -770,6 +779,8 @@ static const struct v4l2_ioctl_ops si470x_ioctl_ops = {
> .vidioc_g_frequency = si470x_vidioc_g_frequency,
> .vidioc_s_frequency = si470x_vidioc_s_frequency,
> .vidioc_s_hw_freq_seek = si470x_vidioc_s_hw_freq_seek,
> + .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
> + .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> };
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a m
Re: [RFC PATCH 3/4] radio-si470x-common.c: remove unnecessary kernel log spam.
Hello Hans,
thanks for the improvements. Looks good to me.
Acked-by: Tobias Lorenz
Bye,
Toby
Am Freitag, 4. Mai 2012, 15:30:31 schrieb Hans Verkuil:
> From: Hans Verkuil
>
> There is no need to report an error in the log, you are already returning
> that error to userspace after all.
>
> Signed-off-by: Hans Verkuil
> ---
> drivers/media/radio/si470x/radio-si470x-common.c | 78
> +- 1 file changed, 17 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/media/radio/si470x/radio-si470x-common.c
> b/drivers/media/radio/si470x/radio-si470x-common.c index e70badf..b9a44d4
> 100644
> --- a/drivers/media/radio/si470x/radio-si470x-common.c
> +++ b/drivers/media/radio/si470x/radio-si470x-common.c
> @@ -327,7 +327,7 @@ static int si470x_set_seek(struct si470x_device *radio,
> radio->registers[POWERCFG] &= ~POWERCFG_SEEKUP;
> retval = si470x_set_register(radio, POWERCFG);
> if (retval < 0)
> - goto done;
> + return retval;
>
> /* currently I2C driver only uses interrupt way to seek */
> if (radio->stci_enabled) {
> @@ -355,20 +355,15 @@ static int si470x_set_seek(struct si470x_device
> *radio, if (radio->registers[STATUSRSSI] & STATUSRSSI_SF)
> dev_warn(&radio->videodev.dev,
> "seek failed / band limit reached\n");
> - if (timed_out)
> - dev_warn(&radio->videodev.dev,
> - "seek timed out after %u ms\n", seek_timeout);
>
> stop:
> /* stop seeking */
> radio->registers[POWERCFG] &= ~POWERCFG_SEEK;
> retval = si470x_set_register(radio, POWERCFG);
>
> -done:
> /* try again, if timed out */
> - if ((retval == 0) && timed_out)
> - retval = -EAGAIN;
> -
> + if (retval == 0 && timed_out)
> + return -EAGAIN;
> return retval;
> }
>
> @@ -589,16 +584,14 @@ static int si470x_vidioc_g_tuner(struct file *file,
> void *priv, struct v4l2_tuner *tuner)
> {
> struct si470x_device *radio = video_drvdata(file);
> - int retval = 0;
> + int retval;
>
> - if (tuner->index != 0) {
> - retval = -EINVAL;
> - goto done;
> - }
> + if (tuner->index != 0)
> + return -EINVAL;
>
> retval = si470x_get_register(radio, STATUSRSSI);
> if (retval < 0)
> - goto done;
> + return retval;
>
> /* driver constants */
> strcpy(tuner->name, "FM");
> @@ -653,10 +646,6 @@ static int si470x_vidioc_g_tuner(struct file *file,
> void *priv, /* AFCRL does only indicate that freq. differs, not if too
> low/high */ tuner->afc = (radio->registers[STATUSRSSI] & STATUSRSSI_AFCRL)
> ? 1 : 0;
>
> -done:
> - if (retval < 0)
> - dev_warn(&radio->videodev.dev,
> - "get tuner failed with %d\n", retval);
> return retval;
> }
>
> @@ -668,7 +657,6 @@ static int si470x_vidioc_s_tuner(struct file *file,
> void *priv, struct v4l2_tuner *tuner)
> {
> struct si470x_device *radio = video_drvdata(file);
> - int retval = 0;
>
> if (tuner->index != 0)
> return -EINVAL;
> @@ -684,12 +672,7 @@ static int si470x_vidioc_s_tuner(struct file *file,
> void *priv, break;
> }
>
> - retval = si470x_set_register(radio, POWERCFG);
> -
> - if (retval < 0)
> - dev_warn(&radio->videodev.dev,
> - "set tuner failed with %d\n", retval);
> - return retval;
> + return si470x_set_register(radio, POWERCFG);
> }
>
>
> @@ -700,21 +683,12 @@ static int si470x_vidioc_g_frequency(struct file
> *file, void *priv, struct v4l2_frequency *freq)
> {
> struct si470x_device *radio = video_drvdata(file);
> - int retval = 0;
>
> - if (freq->tuner != 0) {
> - retval = -EINVAL;
> - goto done;
> - }
> + if (freq->tuner != 0)
> + return -EINVAL;
>
> freq->type = V4L2_TUNER_RADIO;
> - retval = si470x_get_freq(radio, &freq->frequency);
> -
> -done:
> - if (retval < 0)
> - dev_warn(&radio->videodev.dev,
> - "get frequency failed with %d\n", retval);
> - return retval;
> + return si470x_get_freq(radio, &freq->frequency);
> }
>
>
> @@ -725,20 +699,11 @@ static int si470x_vidioc_s_frequency(struct file
> *file, void *priv, struct v4l2_frequency *freq)
> {
> struct si470x_device *radio = video_drvdata(file);
> - int retval = 0;
> -
> - if (freq->tuner != 0) {
> - retval = -EINVAL;
> - goto done;
> - }
>
> - retval = si470x_set_freq(radio, freq->frequency);
> + if (freq->tuner != 0)
> + return -EINVAL;
>
> -done:
> - if (retval < 0)
> - dev_warn(&radio->videodev.dev,
> - "set frequency failed with %d\n", retval);
> - return retval;
> + return si470x_set_freq(radio, freq->frequency);
> }
>
>
> @@ -749,20 +714,11 @@ static int si470x_vid
Re: [RFC PATCH 4/4] radio-si470x-usb: remove autosuspend, implement suspend/resume.
Hello Hans,
thanks for the improvements. Looks good to me.
Acked-by: Tobias Lorenz
Bye,
Toby
Am Freitag, 4. Mai 2012, 15:30:32 schrieb Hans Verkuil:
> From: Hans Verkuil
>
> The radio-si470x-usb driver supported both autosuspend and it stopped the
> radio the moment the last user of the radio device closed it. However, that
> was very confusing since if you play the audio from the device (e.g.
> through arecord -D ... | aplay) then no sound would play unless you had
> the radio device open at the same time, even though there is no need to do
> anything with that node.
>
> On the other hand, the actual suspend/resume functions didn't do anything,
> which would fail if you *did* have the radio node open at that time.
>
> So:
>
> - remove autosuspend (bad idea in general for USB radio devices)
> - move the start/stop out of the open/release functions into the
> resume/suspend functions.
>
> Signed-off-by: Hans Verkuil
> ---
> drivers/media/radio/si470x/radio-si470x-common.c |1 -
> drivers/media/radio/si470x/radio-si470x-usb.c| 149
> ++ 2 files changed, 70 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/media/radio/si470x/radio-si470x-common.c
> b/drivers/media/radio/si470x/radio-si470x-common.c index b9a44d4..969cf49
> 100644
> --- a/drivers/media/radio/si470x/radio-si470x-common.c
> +++ b/drivers/media/radio/si470x/radio-si470x-common.c
> @@ -570,7 +570,6 @@ static int si470x_s_ctrl(struct v4l2_ctrl *ctrl)
> else
> radio->registers[POWERCFG] |= POWERCFG_DMUTE;
> return si470x_set_register(radio, POWERCFG);
> - break;
> default:
> return -EINVAL;
> }
> diff --git a/drivers/media/radio/si470x/radio-si470x-usb.c
> b/drivers/media/radio/si470x/radio-si470x-usb.c index f133c3d..e9f6387
> 100644
> --- a/drivers/media/radio/si470x/radio-si470x-usb.c
> +++ b/drivers/media/radio/si470x/radio-si470x-usb.c
> @@ -481,91 +481,20 @@ resubmit:
> }
>
>
> -
> -/*
> * - * File Operations Interface
> -
> **
> / -
> -/*
> - * si470x_fops_open - file open
> - */
> int si470x_fops_open(struct file *file)
> {
> - struct si470x_device *radio = video_drvdata(file);
> - int retval = v4l2_fh_open(file);
> -
> - if (retval)
> - return retval;
> -
> - retval = usb_autopm_get_interface(radio->intf);
> - if (retval < 0)
> - goto done;
> -
> - if (v4l2_fh_is_singular_file(file)) {
> - /* start radio */
> - retval = si470x_start(radio);
> - if (retval < 0) {
> - usb_autopm_put_interface(radio->intf);
> - goto done;
> - }
> -
> - /* initialize interrupt urb */
> - usb_fill_int_urb(radio->int_in_urb, radio->usbdev,
> - usb_rcvintpipe(radio->usbdev,
> - radio->int_in_endpoint->bEndpointAddress),
> - radio->int_in_buffer,
> - le16_to_cpu(radio->int_in_endpoint->wMaxPacketSize),
> - si470x_int_in_callback,
> - radio,
> - radio->int_in_endpoint->bInterval);
> -
> - radio->int_in_running = 1;
> - mb();
> -
> - retval = usb_submit_urb(radio->int_in_urb, GFP_KERNEL);
> - if (retval) {
> - dev_info(&radio->intf->dev,
> - "submitting int urb failed (%d)\n", retval);
> - radio->int_in_running = 0;
> - usb_autopm_put_interface(radio->intf);
> - }
> - }
> -
> -done:
> - if (retval)
> - v4l2_fh_release(file);
> - return retval;
> + return v4l2_fh_open(file);
> }
>
> -
> -/*
> - * si470x_fops_release - file release
> - */
> int si470x_fops_release(struct file *file)
> {
> - struct si470x_device *radio = video_drvdata(file);
> -
> - if (v4l2_fh_is_singular_file(file)) {
> - /* shutdown interrupt handler */
> - if (radio->int_in_running) {
> - radio->int_in_running = 0;
> - if (radio->int_in_urb)
> - usb_kill_urb(radio->int_in_urb);
> - }
> -
> - /* cancel read processes */
> - wake_up_interruptible(&radio->read_queue);
> -
> - /* stop radio */
> - si470x_stop(radio);
> - usb_autopm_put_interface(radio->intf);
> - }
> return v4l2_fh_release(file);
> }
>
> -static void si470x_usb_release(struct video_device *vdev)
> +static void si470x_usb_release(struct v4l2_device *v4l2_dev)
> {
> - struct si470x_device *radio = video_get_drvdata(vdev);
> + struct si470x_device *radio =
> + container_of(v4l2_dev, stru
Re: [media-ctl PATCH 2/3] New, more flexible syntax for media-ctl
Hi Sakari,
Thanks for the patch.
On Friday 04 May 2012 11:24:42 Sakari Ailus wrote:
> More flexible and extensible syntax for media-ctl which allows better usage
> of the selection API.
[snip]
> diff --git a/src/options.c b/src/options.c
> index 60cf6d5..4d9d48f 100644
> --- a/src/options.c
> +++ b/src/options.c
> @@ -53,12 +53,15 @@ static void usage(const char *argv0, int verbose)
> printf("\n");
> printf("Links and formats are defined as\n");
> printf("\tlink= pad, '->', pad, '[', flags, ']' ;\n");
> - printf("\tformat = pad, '[', fcc, ' ', size, [ ' ', crop ], [ '
> ', '@', frame interval ], ']' ;\n");
> + printf("\tformat = pad, '[', formats ']' ;\n");
> + printf("\tformats = formats ',' formats ;\n");
> + printf("\tformats = fmt | crop | frame interval ;\n");
That's not a valid EBNF. I'm not an expert on the subject, but I think the
following is better.
formats = format { ' ', formats }
format = fmt | crop | frame interval
> + printf("\fmt = 'fmt:', fcc, '/', size ;\n");
format, formats and fmt are becoming confusing. A different name for 'formats'
would be good.
I find the '/' a bit confusing compared to the ' ' (but I think you find the
space confusing compared to '/' :-)). I also wonder whether we shouldn't just
drop 'fmt:', as there can be a single format only.
> printf("\tpad = entity, ':', pad number ;\n");
> printf("\tentity = entity number | ( '\"', entity name, '\"' )
> ;\n");
> printf("\tsize= width, 'x', height ;\n");
> - printf("\tcrop= '(', left, ',', top, ')', '/', size ;\n");
> - printf("\tframe interval = numerator, '/', denominator ;\n");
> + printf("\tcrop= 'crop.actual:', left, ',', top, '/', size
> ;\n");
Would it make sense to make .actual implicit ? Both 'crop' and 'crop.actual'
would be supported by the parser. The code would be more generic if the target
was parsed in a generic way, not associated with the rectangle name.
I would keep the parenthesis around the (top,left) coordinates. You could then
define
rectangle = '(', left, ',', top, ')', '/', size
crop = 'crop' [ '.', target ] ':', rectangle
target = 'actual'
or something similar.
What about also keeping compatibility with the existing syntax (both for
formats and crop rectangles) ? That shouldn't be too difficult in the parser,
crop rectangles start with a '(', and formats come first. We would then have
rectangle = '(', left, ',', top, ')', '/', size
crop = [ 'crop' [ '.', target ] ':' ], rectangle
target = 'actual'
> + printf("\tframe interval = '@', numerator, '/', denominator ;\n");
> printf("where the fields are\n");
> printf("\tentity number Entity numeric identifier\n");
> printf("\tentity name Entity name (string) \n");
--
Regards,
Laurent Pinchart
--
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
PCTV 520e - DVB-C not working
Hi, I have problem getting the PCTV 520e to work with DVB-C on Linux. The video output stream is full of defects. "drxk: SCU_RESULT_INVPAR while sending cmd 0x0203 with params" is printed to the dmesg log when scanning and watching DVB-C channels. I have no problems scanning and watching DVB-T. On Windows 7, both DVB-T and DVB-C works fine. So I guess its not an hardware problem or the quality of the tv signal. I have tried the drivers that came with kernel 3.3.4 and the latest media drivers built from source. The dvb-demod-drxk-pctv.fw firmware file is downloaded with the get_dvb_firmware script and copied to /lib/firmware. I can't figure out what the problem is. Has anyone got PCTV 520e with DVB-C to work on Linux? I have attached some log outputs below. Cheers, Per [per@tux ~]$ uname -a Linux tux 3.3.4-1.fc16.x86_64 #1 SMP Fri Apr 27 20:12:28 UTC 2012 x86_64 x86_64 x86_64 GNU/Linux Log from initialization on kernel 3.3.4 with latest linux media drivers built from source. *** [ 285.856249] usb 2-1.5: new high-speed USB device number 4 using ehci_hcd [ 285.943002] usb 2-1.5: New USB device found, idVendor=2013, idProduct=0251 [ 285.943008] usb 2-1.5: New USB device strings: Mfr=1, Product=2, SerialNumber=3 [ 285.943013] usb 2-1.5: Product: PCTV 520e [ 285.943016] usb 2-1.5: Manufacturer: PCTV Systems [ 285.943019] usb 2-1.5: SerialNumber: 00XX [ 285.962866] Linux media interface: v0.10 [ 285.965105] Linux video capture interface: v2.00 [ 285.965113] WARNING: You are using an experimental version of the media stack. [ 285.965116] As the driver is backported to an older kernel, it doesn't offer [ 285.965119] enough quality for its usage in production. [ 285.965122] Use it with care. [ 285.965124] Latest git patches (needed if you report a bug to linux-media@vger.kernel.org): [ 285.965127] a1ac5dc28d2b4ca78e183229f7c595ffd725241c [media] gspca - sn9c20x: Change the exposure setting of Omnivision sensors [ 285.965130] 4fb8137c43ebc0f5bc0dde6b64faa9dd1b1d7970 [media] gspca - sn9c20x: Don't do sensor update before the capture is started [ 285.965132] c4407fe86d3856f60ec711e025bbe9a0159354a3 [media] gspca - sn9c20x: Set the i2c interface speed [ 285.967402] em28xx: New device PCTV Systems PCTV 520e @ 480 Mbps (2013:0251, interface 0, class 0) [ 285.967407] em28xx: Audio Vendor Class interface 0 found [ 285.967410] em28xx: Video interface 0 found [ 285.967413] em28xx: DVB interface 0 found [ 285.967503] em28xx #0: chip ID is em2884 [ 286.261235] em28xx #0: Identified as PCTV QuatroStick nano (520e) (card=86) [ 286.261291] em28xx #0: Config register raw data: 0x78 [ 286.261294] em28xx #0: I2S Audio (5 sample rates) [ 286.261296] em28xx #0: No AC97 audio processor [ 286.277834] em28xx #0: v4l2 driver version 0.1.3 [ 286.300381] em28xx #0: V4L2 video device registered as video0 [ 286.300851] usbcore: registered new interface driver em28xx [ 286.303734] em28xx-audio.c: probing for em28xx Audio Vendor Class [ 286.303739] em28xx-audio.c: Copyright (C) 2006 Markus Rechberger [ 286.303743] em28xx-audio.c: Copyright (C) 2007-2011 Mauro Carvalho Chehab [ 286.304014] Em28xx: Initialized (Em28xx Audio Extension) extension [ 286.308285] WARNING: You are using an experimental version of the media stack. [ 286.308288] As the driver is backported to an older kernel, it doesn't offer [ 286.308290] enough quality for its usage in production. [ 286.308292] Use it with care. [ 286.308293] Latest git patches (needed if you report a bug to linux-media@vger.kernel.org): [ 286.308295] a1ac5dc28d2b4ca78e183229f7c595ffd725241c [media] gspca - sn9c20x: Change the exposure setting of Omnivision sensors [ 286.308298] 4fb8137c43ebc0f5bc0dde6b64faa9dd1b1d7970 [media] gspca - sn9c20x: Don't do sensor update before the capture is started [ 286.308300] c4407fe86d3856f60ec711e025bbe9a0159354a3 [media] gspca - sn9c20x: Set the i2c interface speed [ 286.324233] drxk: status = 0x639260d9 [ 286.324239] drxk: detected a drx-3926k, spin A3, xtal 20.250 MHz [ 287.719396] DRXK driver version 0.9.4300 [ 287.734984] drxk: frontend initialized. [ 287.738016] tda18271 3-0060: creating new instance [ 287.747206] TDA18271HD/C2 detected @ 3-0060 [ 288.087250] DVB: registering new adapter (em28xx #0) [ 288.087256] DVB: registering adapter 0 frontend 0 (DRXK DVB-C DVB-T)... [ 288.088101] em28xx #0: Successfully loaded em28xx-dvb [ 288.088112] Em28xx: Initialized (Em28xx dvb Extension) extension Using czap: * [per@tux ~]$ czap -c comhem_zapc.conf -r "TV6" using '/dev/dvb/adapter0/frontend0' and '/dev/dvb/adapter0/demux0' reading channels from file 'comhem_zapc.conf' 1 TV6:36200:INVERSION_AUTO:6875100:FEC_3_4:QAM_64:4102:4358:1135 1 TV6: f 36200, s 6875100, i 2, fec 3, qam 3, v 0x1006, a 0x1106, s 0x46f status 00 | signal |
[RFC] V4L: Rename V4L2_SEL_TGT_*_ACTIVE to V4L2_SEL_TGT_*_ACTUAL
After introduction of the selection API on subdevs we have following sets of selection targets: /dev/v4l-subdev? | /dev/video? - V4L2_SUBDEV_SEL_TGT_CROP_ACTUAL| V4L2_SEL_TGT_CROP_ACTIVE V4L2_SUBDEV_SEL_TGT_CROP_BOUNDS| V4L2_SEL_TGT_CROP_BOUNDS | V4L2_SEL_TGT_CROP_DEFAULT | V4L2_SEL_TGT_CROP_PADDED V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTUAL | V4L2_SEL_TGT_COMPOSE_ACTIVE V4L2_SUBDEV_SEL_TGT_COMPOSE_BOUNDS | V4L2_SEL_TGT_COMPOSE_BOUNDS | V4L2_SEL_TGT_COMPOSE_DEFAULT | V4L2_SEL_TGT_COMPOSE_PADDED Although not exactly the same, the meaning of V4L2_SEL_TGT_*_ACTIVE and V4L2_SUBDEV_SEL_TGT_*_ACTUAL selection targets is logically the same. Different names add to confusion where both APIs are used in a single driver or an application. Then, rename the V4l2_SEL_TGT_[CROP/COMPOSE]_ACTIVE to V4l2_SEL_TGT_[CROP/COMPOSE]_ACTUAL to avoid the API inconsistencies. The selections API is experimental, so no any compatibility layer is added. The ABI remains unchanged. Signed-off-by: Sylwester Nawrocki --- Documentation/DocBook/media/v4l/selection-api.xml | 24 ++-- .../DocBook/media/v4l/vidioc-g-selection.xml | 12 +- drivers/media/video/s5p-fimc/fimc-capture.c|8 +++--- drivers/media/video/s5p-jpeg/jpeg-core.c |4 +- drivers/media/video/s5p-tv/mixer_video.c |8 +++--- drivers/media/video/v4l2-ioctl.c |8 +++--- include/linux/videodev2.h |4 +- 7 files changed, 34 insertions(+), 34 deletions(-) diff --git a/Documentation/DocBook/media/v4l/selection-api.xml b/Documentation/DocBook/media/v4l/selection-api.xml index b299e47..5a02067 100644 --- a/Documentation/DocBook/media/v4l/selection-api.xml +++ b/Documentation/DocBook/media/v4l/selection-api.xml @@ -91,7 +91,7 @@ top/left corner at position (0,0) . The rectangle's coordinates are expressed in pixels. The top left corner, width and height of the source rectangle, that is -the area actually sampled, is given by the V4L2_SEL_TGT_CROP_ACTIVE +the area actually sampled, is given by the V4L2_SEL_TGT_CROP_ACTUAL target. It uses the same coordinate system as V4L2_SEL_TGT_CROP_BOUNDS . The active cropping area must lie completely inside the capture boundaries. The driver may further adjust the @@ -111,7 +111,7 @@ height are equal to the image size set by VIDIOC_S_FMT . The part of a buffer into which the image is inserted by the hardware is -controlled by the V4L2_SEL_TGT_COMPOSE_ACTIVE target. +controlled by the V4L2_SEL_TGT_COMPOSE_ACTUAL target. The rectangle's coordinates are also expressed in the same coordinate system as the bounds rectangle. The composing rectangle must lie completely inside bounds rectangle. The driver must adjust the composing rectangle to fit to the @@ -125,7 +125,7 @@ bounding rectangle. The part of a buffer that is modified by the hardware is given by V4L2_SEL_TGT_COMPOSE_PADDED . It contains all pixels -defined using V4L2_SEL_TGT_COMPOSE_ACTIVE plus all +defined using V4L2_SEL_TGT_COMPOSE_ACTUAL plus all padding data modified by hardware during insertion process. All pixels outside this rectangle must not be changed by the hardware. The content of pixels that lie inside the padded area but outside active area is @@ -153,7 +153,7 @@ specified using VIDIOC_S_FMT ioctl. The top left corner, width and height of the source rectangle, that is the area from which image date are processed by the hardware, is given by the - V4L2_SEL_TGT_CROP_ACTIVE . Its coordinates are expressed + V4L2_SEL_TGT_CROP_ACTUAL . Its coordinates are expressed in in the same coordinate system as the bounds rectangle. The active cropping area must lie completely inside the crop boundaries and the driver may further adjust the requested size and/or position according to hardware @@ -165,7 +165,7 @@ bounding rectangle. The part of a video signal or graphics display where the image is inserted by the hardware is controlled by -V4L2_SEL_TGT_COMPOSE_ACTIVE target. The rectangle's coordinates +V4L2_SEL_TGT_COMPOSE_ACTUAL target. The rectangle's coordinates are expressed in pixels. The composing rectangle must lie completely inside the bounds rectangle. The driver must adjust the area to fit to the bounding limits. Moreover, the driver can perform other adjustments according to @@ -184,7 +184,7 @@ such a padded area is driver-dependent feature not covered by this document. Driver developers are encouraged to keep padded rectangle equal to active one. The padded target is accessed by the V4L2_SEL_TGT_COMPOSE_PADDED identifier. It must contain all pixels from the -V4L2_SEL_TGT_COMPOSE_ACTIVE target. +V4L2_SEL_TGT_COMPOSE_ACTUAL target. @@ -193,8 +193,8 @@ V4L2_S
Re: [media-ctl PATCH 2/3] New, more flexible syntax for media-ctl
Hi Laurent,
On Sat, May 05, 2012 at 02:22:26PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thanks for the patch.
Thanks for the review!
> On Friday 04 May 2012 11:24:42 Sakari Ailus wrote:
> > More flexible and extensible syntax for media-ctl which allows better usage
> > of the selection API.
>
> [snip]
>
> > diff --git a/src/options.c b/src/options.c
> > index 60cf6d5..4d9d48f 100644
> > --- a/src/options.c
> > +++ b/src/options.c
> > @@ -53,12 +53,15 @@ static void usage(const char *argv0, int verbose)
> > printf("\n");
> > printf("Links and formats are defined as\n");
> > printf("\tlink= pad, '->', pad, '[', flags, ']' ;\n");
> > - printf("\tformat = pad, '[', fcc, ' ', size, [ ' ', crop ], [ '
> > ', '@', frame interval ], ']' ;\n");
> > + printf("\tformat = pad, '[', formats ']' ;\n");
> > + printf("\tformats = formats ',' formats ;\n");
> > + printf("\tformats = fmt | crop | frame interval ;\n");
>
> That's not a valid EBNF. I'm not an expert on the subject, but I think the
> following is better.
>
> formats = format { ' ', formats }
> format = fmt | crop | frame interval
I'm fine with that change.
> > + printf("\fmt = 'fmt:', fcc, '/', size ;\n");
>
> format, formats and fmt are becoming confusing. A different name for
> 'formats'
> would be good.
I agree but I didn't immediately come up with something better.
The pixel format and the image size at the pad are clearly format
(VIDIOC_SUBDEV_S_FMT) but the other things are related to pads but not
format.
I see them different kinds of properties of pads. That suggests we might be
better renaming the option (-f) to something else as well.
> I find the '/' a bit confusing compared to the ' ' (but I think you find the
> space confusing compared to '/' :-)). I also wonder whether we shouldn't just
> drop 'fmt:', as there can be a single format only.
You can set it multiple times, or you may not set it at all. That's why I
think we should explicitly say it's the format.
> > printf("\tpad = entity, ':', pad number ;\n");
> > printf("\tentity = entity number | ( '\"', entity name, '\"' )
> > ;\n");
> > printf("\tsize= width, 'x', height ;\n");
> > - printf("\tcrop= '(', left, ',', top, ')', '/', size ;\n");
> > - printf("\tframe interval = numerator, '/', denominator ;\n");
> > + printf("\tcrop= 'crop.actual:', left, ',', top, '/', size
> > ;\n");
>
> Would it make sense to make .actual implicit ? Both 'crop' and 'crop.actual'
> would be supported by the parser. The code would be more generic if the
> target
> was parsed in a generic way, not associated with the rectangle name.
I've been also thinking does the actual / active really signify something,
or should that be even dropped form the V4L2 / V4L2 subdev interface
definitions.
> I would keep the parenthesis around the (top,left) coordinates. You could
> then
> define
>
> rectangle = '(', left, ',', top, ')', '/', size
> crop = 'crop' [ '.', target ] ':', rectangle
> target = 'actual'
>
> or something similar.
Sounds good to me.
> What about also keeping compatibility with the existing syntax (both for
> formats and crop rectangles) ? That shouldn't be too difficult in the parser,
> crop rectangles start with a '(', and formats come first. We would then have
>
> rectangle = '(', left, ',', top, ')', '/', size
> crop = [ 'crop' [ '.', target ] ':' ], rectangle
> target = 'actual'
I'll see what I can do. We still should drop the documentation for this so
that people will stop writing rules that look like that.
Cheers,
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi jabber/XMPP/Gmail: sai...@retiisi.org.uk
--
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: [RFC] V4L: Rename V4L2_SEL_TGT_*_ACTIVE to V4L2_SEL_TGT_*_ACTUAL
Hi Sylwester, Sylwester Nawrocki wrote: After introduction of the selection API on subdevs we have following sets of selection targets: /dev/v4l-subdev? | /dev/video? - V4L2_SUBDEV_SEL_TGT_CROP_ACTUAL| V4L2_SEL_TGT_CROP_ACTIVE V4L2_SUBDEV_SEL_TGT_CROP_BOUNDS| V4L2_SEL_TGT_CROP_BOUNDS | V4L2_SEL_TGT_CROP_DEFAULT | V4L2_SEL_TGT_CROP_PADDED V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTUAL | V4L2_SEL_TGT_COMPOSE_ACTIVE V4L2_SUBDEV_SEL_TGT_COMPOSE_BOUNDS | V4L2_SEL_TGT_COMPOSE_BOUNDS | V4L2_SEL_TGT_COMPOSE_DEFAULT | V4L2_SEL_TGT_COMPOSE_PADDED Although not exactly the same, the meaning of V4L2_SEL_TGT_*_ACTIVE and V4L2_SUBDEV_SEL_TGT_*_ACTUAL selection targets is logically the same. Different names add to confusion where both APIs are used in a single driver or an application. Then, rename the V4l2_SEL_TGT_[CROP/COMPOSE]_ACTIVE to V4l2_SEL_TGT_[CROP/COMPOSE]_ACTUAL to avoid the API inconsistencies. The selections API is experimental, so no any compatibility layer is added. The ABI remains unchanged. I'm definitely for keeping the two sets of target as uniform as possible. I have one question, though: how about dropping the ACTIVE / ACTUAL altogether? Then we'd have V4L2_SUBDEV_SEL_TGT_CROP and V4L2_SUBDEV_SEL_TGT_CROP_BOUNDS etc. I feel that the ACTIVE or ACTUAL doesn't really say anything there, and don't think it's an issue that a target name is a part of another one. What's your opinion? Kind regards, -- Sakari Ailus sakari.ai...@iki.fi -- 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: [RFC] V4L: Rename V4L2_SEL_TGT_*_ACTIVE to V4L2_SEL_TGT_*_ACTUAL
Hi Sakari, On 05/05/2012 03:15 PM, Sakari Ailus wrote: > Sylwester Nawrocki wrote: >> After introduction of the selection API on subdevs we have following sets >> of selection targets: >> >> /dev/v4l-subdev? | /dev/video? >> - >> V4L2_SUBDEV_SEL_TGT_CROP_ACTUAL | V4L2_SEL_TGT_CROP_ACTIVE >> V4L2_SUBDEV_SEL_TGT_CROP_BOUNDS | V4L2_SEL_TGT_CROP_BOUNDS >> | V4L2_SEL_TGT_CROP_DEFAULT >> | V4L2_SEL_TGT_CROP_PADDED >> V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTUAL | V4L2_SEL_TGT_COMPOSE_ACTIVE >> V4L2_SUBDEV_SEL_TGT_COMPOSE_BOUNDS | V4L2_SEL_TGT_COMPOSE_BOUNDS >> | V4L2_SEL_TGT_COMPOSE_DEFAULT >> | V4L2_SEL_TGT_COMPOSE_PADDED >> >> Although not exactly the same, the meaning of V4L2_SEL_TGT_*_ACTIVE >> and V4L2_SUBDEV_SEL_TGT_*_ACTUAL selection targets is logically the >> same. Different names add to confusion where both APIs are used in >> a single driver or an application. >> Then, rename the V4l2_SEL_TGT_[CROP/COMPOSE]_ACTIVE to >> V4l2_SEL_TGT_[CROP/COMPOSE]_ACTUAL to avoid the API inconsistencies. >> The selections API is experimental, so no any compatibility layer >> is added. The ABI remains unchanged. > > I'm definitely for keeping the two sets of target as uniform as possible. > > I have one question, though: how about dropping the ACTIVE / ACTUAL > altogether? Then we'd have V4L2_SUBDEV_SEL_TGT_CROP and > V4L2_SUBDEV_SEL_TGT_CROP_BOUNDS etc. I feel that the ACTIVE or ACTUAL > doesn't really say anything there, and don't think it's an issue that a > target name is a part of another one. Agreed. I really like the idea of dropping the rather meaningless postfixes. I don't see an issue here with the subdev API, 'crop/compose' and 'crop/compose bounds' would mean exactly what it says. Only V4L2_SEL_TGT_COMPOSE_PADDED stands a bit out. I don't see any driver really using it at the moment though and I recall Tomasz saying it wasn't best idea to add it in first place. So I believe V4L2_SEL_TGT_COMPOSE_PADDED might get removed from the API in future. I'm going to rework the patch, then let's see what's Tomasz's and others' opinion. -- Regards, Sylwester -- 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: [RFCv1 PATCH 2/7] zc3xx: convert to the control framework.
Hi,
Reviewing and testing this one was, erm, interesting. See comments
inline.
On 04/28/2012 05:09 PM, Hans Verkuil wrote:
From: Hans Verkuil
Signed-off-by: Hans Verkuil
---
drivers/media/video/gspca/zc3xx.c | 451 +++--
1 file changed, 182 insertions(+), 269 deletions(-)
diff --git a/drivers/media/video/gspca/zc3xx.c
b/drivers/media/video/gspca/zc3xx.c
index 7d9a4f1..e7b7599 100644
--- a/drivers/media/video/gspca/zc3xx.c
+++ b/drivers/media/video/gspca/zc3xx.c
+static int sd_setautogain(struct gspca_dev *gspca_dev, s32 val)
+{
+ if (!gspca_dev->streaming)
+ return 0;
+ setautogain(gspca_dev, val);
+
+ return gspca_dev->usb_err;
+}
+
zcxx_s_ctrl can just as well call setautogain directly as it
does for the others. It should gspca_dev->streaming for all
controls anyways (more on that below).
+static int sd_setquality(struct gspca_dev *gspca_dev, s32 val)
+{
+ struct sd *sd = (struct sd *) gspca_dev;
+ int i;
+
+ for (i = 0; i< ARRAY_SIZE(jpeg_qual) - 1; i++) {
+ if (val<= jpeg_qual[i])
+ break;
+ }
+ sd->reg08 = i;
+ if (!gspca_dev->streaming)
+ return 0;
+ jpeg_set_qual(sd->jpeg_hdr, val);
+ return gspca_dev->usb_err;
+}
+
This for 99% duplicates the special handling you already
have for this in zcxx_s_ctrl, which is also the only caller.
More over this gets its wrong as under certain conditions
it will end up with a different value of i then the code in
zcxx_s_ctrl, and zcxx_s_ctrl bases the value it returns
to the caller as the "achieved" value on its own calculation
of i.
So I thought lets refactor this a bit to get rid of this
duplication and that turned out to be a long journey rather
then a quick patch :) While testing my changes I found that
there are various (pre-existing) problems with how zc3xx
handles JPEG quality so I've fixed those first.
The result is a 6 patch patchset (attached), which besides
many fixes to the JPEG quality handling, also includes
a new version of this patch removing the duplication.
+static int sd_set_jcomp(struct gspca_dev *gspca_dev,
+ struct v4l2_jpegcompression *jcomp)
+{
+ struct sd *sd = (struct sd *) gspca_dev;
+
+ if (sd->jpegqual) {
+ v4l2_ctrl_s_ctrl(sd->jpegqual, jcomp->quality);
+ jcomp->quality = v4l2_ctrl_g_ctrl(sd->jpegqual);
+ return gspca_dev->usb_err;
+ }
+ jcomp->quality = jpeg_qual[sd->reg08];
+ return 0;
+}
+
+static int sd_get_jcomp(struct gspca_dev *gspca_dev,
+ struct v4l2_jpegcompression *jcomp)
+{
+ struct sd *sd = (struct sd *) gspca_dev;
+
+ memset(jcomp, 0, sizeof *jcomp);
+ jcomp->quality = jpeg_qual[sd->reg08];
+ jcomp->jpeg_markers = V4L2_JPEG_MARKER_DHT
+ | V4L2_JPEG_MARKER_DQT;
+ return 0;
+}
+
No need to move these upwards in the source file, leaving them
where they are makes it easier to see what is actually changed.
+static int zcxx_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct sd *sd = container_of(ctrl->handler, struct sd, ctrl_handler);
+
+ switch (ctrl->id) {
+ case V4L2_CID_AUTOGAIN:
+ if (ctrl->val&& sd->exposure&& sd->gspca_dev.streaming)
+ sd->exposure->val = getexposure(&sd->gspca_dev);
+ break;
+ }
+ return 0;
+}
The call to getexposure needs to be surrounded by locking / unlocking
usb_lock, and gspca_de->usb_err should be checked after the call.
In general all calls to the actual hardware need to be made with
the usb_lock hold, so that they cannot race with for example
hardware accesses done from sd_start. This is important since
although the usb-core will ensure that we never can do more then
one control request at a time sometimes several control requests
need to be done in order without interference (for example for
accessing the i2c bus between the bridge and the sensor).
+
+static int zcxx_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct sd *sd = container_of(ctrl->handler, struct sd, ctrl_handler);
+ int ret;
+ int i;
+
+ switch (ctrl->id) {
+ /* gamma/brightness/contrast cluster */
+ case V4L2_CID_GAMMA:
+ setcontrast(&sd->gspca_dev, sd->gamma->val,
+ sd->brightness->val, sd->contrast->val);
+ return 0;
+ /* autogain/exposure cluster */
+ case V4L2_CID_AUTOGAIN:
+ ret = sd_setautogain(&sd->gspca_dev, ctrl->val);
+ if (!ret&& !ctrl->val&& sd->exposure)
+ setexposure(&sd->gspca_dev, sd->exposure->val);
+ return ret;
+ case V4L2_CID_POWER_LINE_FREQUENCY:
+ setlightfreq(&sd->gspca_dev, ctrl->val);
+ return 0;
+ case V4L2_CID_SHARPNESS:
+ setsharpness(&sd->gspca
Re: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
Hi,
On 05/05/2012 11:14 AM, Hans Verkuil wrote:
On Sat May 5 2012 09:43:01 Hans de Goede wrote:
Hi,
I'm slowly working my way though this series today (both review, as well
as some tweaks and testing).
More comments inline...
On 04/28/2012 05:09 PM, Hans Verkuil wrote:
From: Hans Verkuil
Make the necessary changes to allow subdrivers to use the control framework.
This does not add control event support, that needs more work.
Signed-off-by: Hans Verkuil
---
drivers/media/video/gspca/gspca.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/media/video/gspca/gspca.c
b/drivers/media/video/gspca/gspca.c
index ca5a2b1..56dff10 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -38,6 +38,7 @@
#include
#include
#include
+#include
#include "gspca.h"
@@ -1006,6 +1007,8 @@ static void gspca_set_default_mode(struct gspca_dev
*gspca_dev)
/* set the current control values to their default values
* which may have changed in sd_init() */
+ /* does nothing if ctrl_handler == NULL */
+ v4l2_ctrl_handler_setup(gspca_dev->vdev.ctrl_handler);
ctrl = gspca_dev->cam.ctrls;
if (ctrl != NULL) {
for (i = 0;
@@ -1323,6 +1326,7 @@ static void gspca_release(struct video_device *vfd)
PDEBUG(D_PROBE, "%s released",
video_device_node_name(&gspca_dev->vdev));
+ v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
kfree(gspca_dev->usb_buf);
kfree(gspca_dev);
}
@@ -2347,6 +2351,10 @@ int gspca_dev_probe2(struct usb_interface *intf,
gspca_dev->sd_desc = sd_desc;
gspca_dev->nbufread = 2;
gspca_dev->empty_packet = -1;/* don't check the empty packets */
+ gspca_dev->vdev = gspca_template;
+ gspca_dev->vdev.parent =&intf->dev;
+ gspca_dev->module = module;
+ gspca_dev->present = 1;
/* configure the subdriver and initialize the USB device */
ret = sd_desc->config(gspca_dev, id);
You also need to move the initialization of the mutexes here, as the
v4l2_ctrl_handler_setup will call s_ctrl on all the controls, and s_ctrl
should take the usb_lock (see my review of the next patch in this series),
I'll make this change myself and merge it into your patch.
Looking at how usb_lock is used I am inclined to just set video_device->lock
to it and let the v4l2 core do all the locking for me, which will automatically
fix the missing s_ctrl lock too.
Please don't I've really bad experience with this with pwc (large latencies
on dqbuf), also gspca uses worker-threads which issue usb control requests
in various places, currently these take usb_lock to avoid them fighting with
sd_start or controls, these won't know about the global device lock and if
these start taking that lock too things will become pretty messy pretty quickly.
I've realized that there is a problem if you do your own locking *and* use the
control framework: if you need to set a control from within the driver, then
you do that using v4l2_ctrl_s_ctrl. But if s_ctrl has to take the driver's lock,
then you can't call v4l2_ctrl_s_ctrl with that lock already taken!
So you get:
vidioc_foo()
lock(mylock)
v4l2_ctrl_s_ctrl(ctrl, val)
s_ctrl(ctrl, val)
lock(mylock)
Easy solution here, remove the first lock(mylock), since we are not using
v4l2-dev's
locking, we are the one doing the first lock, and if we are going to call
v4l2_ctrl_s_ctrl
we should simply not do that!
Now I see that we are doing exactly that in for example vidioc_g_jpegcomp in
gspca.c, so
we should stop doing that. We can make vidioc_g/s_jpegcomp only do the usb
locking if
gspca_dev->vdev.ctrl_handler == NULL, and once all sub drivers are converted
simply remove
it. Actually I'm thinking about making the jpegqual control part of the
gspca_dev struct
itself and move all handling of vidioc_g/s_jpegcomp out of the sub drivers and
into
the core.
Regards,
Hans
--
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: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
Hi, On 05/05/2012 10:34 AM, Hans Verkuil wrote: On Sat May 5 2012 09:43:01 Hans de Goede wrote: Hi, I'm slowly working my way though this series today (both review, as well as some tweaks and testing). Thanks for that! One note: I initialized the controls in sd_init. That's wrong, it should be sd_config. sd_init is also called on resume, so that would initialize the controls twice. You cannot move the initializing of the controls to sd_config, since in many cases the sensor probing is done in sd_init, and we need to know the sensor type to init the controls. I suggest that instead you give the sd_init function a resume parameter and only init the controls if the resume parameter is false. I'm working on this as well today, together with finishing the stv06xx and mars conversion. Cool! Regards, Hans -- 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: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
On Sat May 5 2012 16:46:32 Hans de Goede wrote: > Hi, > > On 05/05/2012 10:34 AM, Hans Verkuil wrote: > > On Sat May 5 2012 09:43:01 Hans de Goede wrote: > >> Hi, > >> > >> I'm slowly working my way though this series today (both review, as well > >> as some tweaks and testing). > > > > Thanks for that! > > > > One note: I initialized the controls in sd_init. That's wrong, it should be > > sd_config. sd_init is also called on resume, so that would initialize the > > controls twice. > > You cannot move the initializing of the controls to sd_config, since in many > cases the sensor probing is done in sd_init, and we need to know the sensor > type to init the controls. Or you move the sensor probing to sd_config as I did. It makes no sense anyway to do sensor probing every time you resume. Unless there is another good reason for doing the probing in sd_init I prefer to move it to sd_config. Regards, Hans > I suggest that instead you give the sd_init > function a resume parameter and only init the controls if the resume parameter > is false. > > > I'm working on this as well today, together with finishing the stv06xx and > > mars conversion. > > Cool! > > Regards, > > Hans > -- 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: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
Hi, On 05/05/2012 04:50 PM, Hans Verkuil wrote: On Sat May 5 2012 16:46:32 Hans de Goede wrote: Hi, On 05/05/2012 10:34 AM, Hans Verkuil wrote: On Sat May 5 2012 09:43:01 Hans de Goede wrote: Hi, I'm slowly working my way though this series today (both review, as well as some tweaks and testing). Thanks for that! One note: I initialized the controls in sd_init. That's wrong, it should be sd_config. sd_init is also called on resume, so that would initialize the controls twice. You cannot move the initializing of the controls to sd_config, since in many cases the sensor probing is done in sd_init, and we need to know the sensor type to init the controls. Or you move the sensor probing to sd_config as I did. It makes no sense anyway to do sensor probing every time you resume. Unless there is another good reason for doing the probing in sd_init I prefer to move it to sd_config. Sensor probing does more then just sensor probing, it also configures things like the i2c clockrate, and if the bus between bridge and sensor is spi / i2c or 3-wire, or whatever ... After a suspend resume all bets are of wrt bridge state, so we prefer to always do a full re-init as we do on initial probe, so that we (hopefully) will put the bridge back in a sane state. I think moving the probing from init to config is a bad idea, the chance that we will get regressions (after a suspend/resume) from this are too big IMHO. Regards, Hans -- 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: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
On Sat May 5 2012 16:44:34 Hans de Goede wrote:
> Hi,
>
> On 05/05/2012 11:14 AM, Hans Verkuil wrote:
> > On Sat May 5 2012 09:43:01 Hans de Goede wrote:
> >> Hi,
> >>
> >> I'm slowly working my way though this series today (both review, as well
> >> as some tweaks and testing).
> >>
> >> More comments inline...
> >>
> >> On 04/28/2012 05:09 PM, Hans Verkuil wrote:
> >>> From: Hans Verkuil
> >>>
> >>> Make the necessary changes to allow subdrivers to use the control
> >>> framework.
> >>> This does not add control event support, that needs more work.
> >>>
> >>> Signed-off-by: Hans Verkuil
> >>> ---
> >>>drivers/media/video/gspca/gspca.c | 13 +
> >>>1 file changed, 9 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/media/video/gspca/gspca.c
> >>> b/drivers/media/video/gspca/gspca.c
> >>> index ca5a2b1..56dff10 100644
> >>> --- a/drivers/media/video/gspca/gspca.c
> >>> +++ b/drivers/media/video/gspca/gspca.c
> >>> @@ -38,6 +38,7 @@
> >>>#include
> >>>#include
> >>>#include
> >>> +#include
> >>>
> >>>#include "gspca.h"
> >>>
> >>> @@ -1006,6 +1007,8 @@ static void gspca_set_default_mode(struct gspca_dev
> >>> *gspca_dev)
> >>>
> >>> /* set the current control values to their default values
> >>>* which may have changed in sd_init() */
> >>> + /* does nothing if ctrl_handler == NULL */
> >>> + v4l2_ctrl_handler_setup(gspca_dev->vdev.ctrl_handler);
> >>> ctrl = gspca_dev->cam.ctrls;
> >>> if (ctrl != NULL) {
> >>> for (i = 0;
> >>> @@ -1323,6 +1326,7 @@ static void gspca_release(struct video_device *vfd)
> >>> PDEBUG(D_PROBE, "%s released",
> >>> video_device_node_name(&gspca_dev->vdev));
> >>>
> >>> + v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
> >>> kfree(gspca_dev->usb_buf);
> >>> kfree(gspca_dev);
> >>>}
> >>> @@ -2347,6 +2351,10 @@ int gspca_dev_probe2(struct usb_interface *intf,
> >>> gspca_dev->sd_desc = sd_desc;
> >>> gspca_dev->nbufread = 2;
> >>> gspca_dev->empty_packet = -1; /* don't check the empty
> >>> packets */
> >>> + gspca_dev->vdev = gspca_template;
> >>> + gspca_dev->vdev.parent =&intf->dev;
> >>> + gspca_dev->module = module;
> >>> + gspca_dev->present = 1;
> >>>
> >>> /* configure the subdriver and initialize the USB device */
> >>> ret = sd_desc->config(gspca_dev, id);
> >>
> >> You also need to move the initialization of the mutexes here, as the
> >> v4l2_ctrl_handler_setup will call s_ctrl on all the controls, and s_ctrl
> >> should take the usb_lock (see my review of the next patch in this series),
> >> I'll make this change myself and merge it into your patch.
> >
> > Looking at how usb_lock is used I am inclined to just set video_device->lock
> > to it and let the v4l2 core do all the locking for me, which will
> > automatically
> > fix the missing s_ctrl lock too.
>
> Please don't I've really bad experience with this with pwc (large latencies
> on dqbuf),
Can you refresh my memory on that? I remember that you had problems with it
but I don't think I ever followed up on it (i.e. trying to find a solution
that works with core locking).
> also gspca uses worker-threads which issue usb control requests
> in various places, currently these take usb_lock to avoid them fighting with
> sd_start or controls, these won't know about the global device lock and if
> these start taking that lock too things will become pretty messy pretty
> quickly.
Well, gspca takes usb_lock at the same places the core lock is taken. And it
actually fails to take the lock in the suspend and resume functions which can
lead to an oops (reproduced). Those worker threads do not handle suspend/resume
well at the moment (I have patches for that).
>
> >
> > I've realized that there is a problem if you do your own locking *and* use
> > the
> > control framework: if you need to set a control from within the driver, then
> > you do that using v4l2_ctrl_s_ctrl. But if s_ctrl has to take the driver's
> > lock,
> > then you can't call v4l2_ctrl_s_ctrl with that lock already taken!
> >
> > So you get:
> >
> > vidioc_foo()
> > lock(mylock)
> > v4l2_ctrl_s_ctrl(ctrl, val)
> > s_ctrl(ctrl, val)
> > lock(mylock)
>
> Easy solution here, remove the first lock(mylock), since we are not using
> v4l2-dev's
> locking, we are the one doing the first lock, and if we are going to call
> v4l2_ctrl_s_ctrl
> we should simply not do that!
It's really ugly that way: you basically take a low-level lock (that of the
control handler which is held whenever s_ctrl et al is called) first, then take
a high-level lock (usb_lock). That's asking for problems.
> Now I see that we are doing exactly that in for example vidioc_g_jpegcomp in
> gspca.c, so
> we should stop doing that. We can make vidioc_g/s_jpegcomp only do the usb
> locking if
> gspca_dev->vdev.ctrl_handler =
Re: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
Hi, On 05/05/2012 04:44 PM, Hans de Goede wrote: Hi, On 05/05/2012 11:14 AM, Hans Verkuil wrote: So you get: vidioc_foo() lock(mylock) v4l2_ctrl_s_ctrl(ctrl, val) s_ctrl(ctrl, val) lock(mylock) Easy solution here, remove the first lock(mylock), since we are not using v4l2-dev's locking, we are the one doing the first lock, and if we are going to call v4l2_ctrl_s_ctrl we should simply not do that! Now I see that we are doing exactly that in for example vidioc_g_jpegcomp in gspca.c, so we should stop doing that. We can make vidioc_g/s_jpegcomp only do the usb locking if gspca_dev->vdev.ctrl_handler == NULL, and once all sub drivers are converted simply remove it. Actually I'm thinking about making the jpegqual control part of the gspca_dev struct itself and move all handling of vidioc_g/s_jpegcomp out of the sub drivers and into the core. Here is an updated version of this patch implementing this approach for vidioc_g/s_jpegcomp. We may need to do something similar in other places, although I cannot think of any such places atm, Regards, Hans -- 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: [PATCH] v4l: soc-camera: Add support for enum_frameintervals ioctl
On Fri, 4 May 2012, Aguirre, Sergio wrote:
> Hi Guennadi,
>
> No problem.
>
> On Fri, May 4, 2012 at 10:05 AM, Guennadi Liakhovetski
> wrote:
> > Hi Sergio
> >
> > Sorry about the delay.
> >
> > On Wed, 18 Apr 2012, Aguirre, Sergio wrote:
> >
> >> From: Sergio Aguirre
> >>
> >> Signed-off-by: Sergio Aguirre
> >> ---
> >> drivers/media/video/soc_camera.c | 37
> >> +
> >> include/media/soc_camera.h | 1 +
> >> 2 files changed, 38 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/media/video/soc_camera.c
> >> b/drivers/media/video/soc_camera.c
> >> index eb25756..62c8956 100644
> >> --- a/drivers/media/video/soc_camera.c
> >> +++ b/drivers/media/video/soc_camera.c
> >> @@ -266,6 +266,15 @@ static int soc_camera_enum_fsizes(struct file
> >> *file, void *fh,
> >> return ici->ops->enum_fsizes(icd, fsize);
> >> }
> >>
> >> +static int soc_camera_enum_fivals(struct file *file, void *fh,
> >
> > "fivals" is a bit short for my taste. Yes, I know about the
> > *_enum_fsizes() precedent in soc_camera.c, we should have used a more
> > descriptive name for that too. So, maybe I'll push a patch to change that
> > to enum_frmsizes() or enum_framesizes().
>
> Agreed.
>
> >
> > But that brings in a larger question, which is also the reason, why I
> > added a couple more people to the CC: the following 3 operations in struct
> > v4l2_subdev_video_ops don't make me particularly happy:
> >
> > int (*enum_framesizes)(struct v4l2_subdev *sd, struct
> > v4l2_frmsizeenum *fsize);
> > int (*enum_frameintervals)(struct v4l2_subdev *sd, struct
> > v4l2_frmivalenum *fival);
> > int (*enum_mbus_fsizes)(struct v4l2_subdev *sd,
> > struct v4l2_frmsizeenum *fsize);
> >
> > The problems are:
> >
> > 1. enum_framesizes and enum_mbus_fsizes seem to be identical (yes, I see
> > my Sob under the latter:-()
>
> Yeah, IMHO, the mbus one should go, since there's no mbus specific structure
> being handed as a parameter.
Right, we can do that.
> > 2. both struct v4l2_frmsizeenum and struct v4l2_frmivalenum are the
> > structs, used in the respective V4L2 ioctl()s, and they both contain a
> > field for a fourcc value, which doesn't make sense to subdevice drivers.
> > So far the only driver combination in the mainline, that I see, that uses
> > these operations is marvell-ccic & ov7670. These drivers just ignore the
> > pixel format. Relatively recently enum_mbus_fsizes() has been added to
> > soc-camera, and this patch is adding enum_frameintervals(). Both these
> > implementations abuse the .pixel_format field to pass a media-bus code
> > value in it to subdevice drivers. This sends meaningful information to
> > subdevice drivers, but is really a hack, rather than a proper
> > implementation.
>
> True.
>
> >
> > Any idea how to improve this? Shall we create mediabus clones of those
> > structs with an mbus code instead of fourcc, and drop one of the above
> > enum_framesizes() operations?
>
> Well, to add more confusion to this.. :)
>
> We have this v4l2-subdev IOCTLs exported to userspace:
>
> #define VIDIOC_SUBDEV_ENUM_FRAME_SIZE \
> _IOWR('V', 74, struct v4l2_subdev_frame_size_enum)
> #define VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL \
> _IOWR('V', 75, struct v4l2_subdev_frame_interval_enum)
>
> Which in "drivers/media/video/v4l2-subdev.c", are translated to pad ops:
> - v4l2_subdev_call(... enum_frame_size ...);
> - v4l2_subdev_call(... enum_frame_interval ...);
>
> respectively.
>
> So, this is also another thing that's causing some confusion.
Wow, didn't know about those. I was about to propose to use those in video
subdev ops, but then I noticed: pad operations don't support stepwise
enumerations. struct v4l2_subdev_frame_size_enum seems to implement
continuous enumeration implicitly, with discrete being a particular
degenerate case of it. struct v4l2_subdev_frame_interval_enum only
implements discrete enumeration only. I personally don't have a problem
with that, I'm not a big fan of these enumeration operations anyway, and
AFAICS, the only subdevice driver, implementing those video operations is
ov7670, and it implements only DISCRETE.
So, would it be good enough for everyone to drop enum_mbus_fsizes() and to
convert enum_frameintervals() and enum_framesizes() video operations to
use structs from pad operations and just ignore the pad field? Any
objections?
> Does soc_camera use pad ops?
No, not yet.
> Regards,
> Sergio
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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
Problems with Gadmei UTV380New [id=1f71:3301]
Model = Gadmei UTV380New
Vendor/ProductID = 1f71:3301
Test =
- I follow instruction on link
http://www.linuxtv.org/wiki/index.php?title=Gadmei_USB_TVBox_UTV380
- I change "SB_DEVICE(0xeb1a, 0x50a3)" with "SB_DEVICE(0x1f71, 0x3301)"
- after i compile ("make distsclean", "make menuconfig", "make", and "make
install") then connect tvTunner to my PC
- I get error like this
#dmesg
...
...
[ 9643.782870] usb 2-1: USB disconnect, address 5
[ 9819.912068] usb 2-1: new high speed USB device using ehci_hcd and
address 6
[ 9820.047446] usb 2-1: config 1 interface 0 altsetting 1 bulk endpoint
0x83 has invalid maxpacket 256
[ 9820.063627] v4l2_common: disagrees about version of symbol
v4l2_device_register_subdev
[ 9820.063635] v4l2_common: Unknown symbol v4l2_device_register_subdev
(err -22)
[ 9820.063813] v4l2_common: disagrees about version of symbol
v4l2_device_unregister_subdev
[ 9820.063818] v4l2_common: Unknown symbol v4l2_device_unregister_subdev
(err -22)
- Here output from "lsusb"
#lsusb
Bus 007 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
Bus 006 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
Bus 005 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
Bus 004 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
Bus 003 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub
Bus 002 Device 005: ID 1f71:3301
Bus 002 Device 002: ID 04f2:b044 Chicony Electronics Co., Ltd Acer
CrystalEye Webcam
Bus 002 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
(1f71:3301 <- my tvTunner ID)
#lsusb -v -d 1f71:3301
Bus 002 Device 006: ID 1f71:3301
Device Descriptor:
bLength18
bDescriptorType 1
bcdUSB 2.00
bDeviceClass0 (Defined at Interface level)
bDeviceSubClass 0
bDeviceProtocol 0
bMaxPacketSize064
idVendor 0x1f71
idProduct 0x3301
bcdDevice1.00
iManufacturer 3 Gadmei
iProduct4 USB TV Box
iSerial 2 3309
bNumConfigurations 1
Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength 83
bNumInterfaces 1
bConfigurationValue 1
iConfiguration 0
bmAttributes 0x80
(Bus Powered)
MaxPower 500mA
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber0
bAlternateSetting 0
bNumEndpoints 4
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol255
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes1
Transfer TypeIsochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x 1x 0 bytes
bInterval 1
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes2
Transfer TypeBulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes2
Transfer TypeBulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x84 EP 4 IN
bmAttributes3
Transfer TypeInterrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0040 1x 64 bytes
bInterval 4
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber0
bAlternateSetting 1
bNumEndpoints 4
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 0
bInterfaceProtocol255
iInterface 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes1
Transfer TypeIsochronous
Synch Type None
Usage Type Data
wMaxPacketSize 0x1400 3x 1024 bytes
bInterval 1
Endpoint Descriptor:
Re: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
Hi,
Oops, forgot the attachment it is here now...
Regards,
Hans
On 05/05/2012 05:05 PM, Hans de Goede wrote:
Hi,
On 05/05/2012 04:44 PM, Hans de Goede wrote:
Hi,
On 05/05/2012 11:14 AM, Hans Verkuil wrote:
So you get:
vidioc_foo()
lock(mylock)
v4l2_ctrl_s_ctrl(ctrl, val)
s_ctrl(ctrl, val)
lock(mylock)
Easy solution here, remove the first lock(mylock), since we are not using
v4l2-dev's
locking, we are the one doing the first lock, and if we are going to call
v4l2_ctrl_s_ctrl
we should simply not do that!
Now I see that we are doing exactly that in for example vidioc_g_jpegcomp in
gspca.c, so
we should stop doing that. We can make vidioc_g/s_jpegcomp only do the usb
locking if
gspca_dev->vdev.ctrl_handler == NULL, and once all sub drivers are converted
simply remove
it. Actually I'm thinking about making the jpegqual control part of the
gspca_dev struct
itself and move all handling of vidioc_g/s_jpegcomp out of the sub drivers and
into
the core.
Here is an updated version of this patch implementing this approach for
vidioc_g/s_jpegcomp. We may need to do something similar in other places,
although I cannot
think of any such places atm,
Regards,
Hans
--
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
>From eb7eb7c63156c1c040a7fddaeddcf1b1891f0fb7 Mon Sep 17 00:00:00 2001
From: Hans Verkuil
Date: Sat, 28 Apr 2012 17:09:50 +0200
Subject: [PATCH 1/8] gspca: allow subdrivers to use the control framework.
Make the necessary changes to allow subdrivers to use the control framework.
This does not add control event support, that needs more work.
Signed-off-by: Hans Verkuil
Signed-off-by: Hans de Goede
---
drivers/media/video/gspca/gspca.c | 47 ++---
1 file changed, 33 insertions(+), 14 deletions(-)
diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
index ca5a2b1..dfe2e8a 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -38,6 +38,7 @@
#include
#include
#include
+#include
#include "gspca.h"
@@ -1006,6 +1007,8 @@ static void gspca_set_default_mode(struct gspca_dev *gspca_dev)
/* set the current control values to their default values
* which may have changed in sd_init() */
+ /* does nothing if ctrl_handler == NULL */
+ v4l2_ctrl_handler_setup(gspca_dev->vdev.ctrl_handler);
ctrl = gspca_dev->cam.ctrls;
if (ctrl != NULL) {
for (i = 0;
@@ -1323,6 +1326,7 @@ static void gspca_release(struct video_device *vfd)
PDEBUG(D_PROBE, "%s released",
video_device_node_name(&gspca_dev->vdev));
+ v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
kfree(gspca_dev->usb_buf);
kfree(gspca_dev);
}
@@ -1771,14 +1775,21 @@ static int vidioc_g_jpegcomp(struct file *file, void *priv,
if (!gspca_dev->sd_desc->get_jcomp)
return -EINVAL;
- if (mutex_lock_interruptible(&gspca_dev->usb_lock))
- return -ERESTARTSYS;
+
+ /* Don't take the usb_lock when using the v4l2-ctrls framework */
+ if (gspca_dev->vdev.ctrl_handler == NULL)
+ if (mutex_lock_interruptible(&gspca_dev->usb_lock))
+ return -ERESTARTSYS;
+
gspca_dev->usb_err = 0;
if (gspca_dev->present)
ret = gspca_dev->sd_desc->get_jcomp(gspca_dev, jpegcomp);
else
ret = -ENODEV;
- mutex_unlock(&gspca_dev->usb_lock);
+
+ if (gspca_dev->vdev.ctrl_handler == NULL)
+ mutex_unlock(&gspca_dev->usb_lock);
+
return ret;
}
@@ -1790,14 +1801,21 @@ static int vidioc_s_jpegcomp(struct file *file, void *priv,
if (!gspca_dev->sd_desc->set_jcomp)
return -EINVAL;
- if (mutex_lock_interruptible(&gspca_dev->usb_lock))
- return -ERESTARTSYS;
+
+ /* Don't take the usb_lock when using the v4l2-ctrls framework */
+ if (gspca_dev->vdev.ctrl_handler == NULL)
+ if (mutex_lock_interruptible(&gspca_dev->usb_lock))
+ return -ERESTARTSYS;
+
gspca_dev->usb_err = 0;
if (gspca_dev->present)
ret = gspca_dev->sd_desc->set_jcomp(gspca_dev, jpegcomp);
else
ret = -ENODEV;
- mutex_unlock(&gspca_dev->usb_lock);
+
+ if (gspca_dev->vdev.ctrl_handler == NULL)
+ mutex_unlock(&gspca_dev->usb_lock);
+
return ret;
}
@@ -2347,6 +2365,14 @@ int gspca_dev_probe2(struct usb_interface *intf,
gspca_dev->sd_desc = sd_desc;
gspca_dev->nbufread = 2;
gspca_dev->empty_packet = -1; /* don't check the empty packets */
+ gspca_dev->vdev = gspca_template;
+ gspca_dev->vdev.parent = &intf->dev;
+ gspca_dev->module = module;
+ gspca_dev->present = 1;
+
+ mutex_init(&gspca_dev->usb_lock);
+ mutex_init(&gspca_dev->queue_lock);
+ init_waitqueue_head(&gspca_dev->wq);
/* configure the subdriver and initialize the USB device */
ret = sd_desc->config(gspca_dev, id);
@@ -2363,15 +2389,7 @@ int gspca_dev_probe2(struct usb_interface *intf,
if (ret)
goto out;
- mutex_init(&gspca_dev->usb_lock);
- mutex_init(&gspca_dev->queue_lock);
- init_waitqueue_head(&gspca_dev->wq);
-
/* i
Re: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
Hi, On 05/05/2012 05:02 PM, Hans Verkuil wrote: BTW, this is getting messy: both of us producing patches for the same driver :-) For those reading along on the mailinglist, we've got together on irc and coordinated how we are going to handle this there. Regards, Hans -- 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: [PATCH v2] add support for DeLOCK-USB-2.0-DVB-T-Receiver-61744
I am currently finishing up the work at the demod driver and will probably send a new version to the list tomorrow. As I don't own a device with a different tuner than the fc0012 I will include an error message about the unsupported tuner and print its type. So It is easier to get the information about the tuners. Right now I am writing the signal_strength callback and stumbled upon the following problem: The signal strength is read from the fc0012 tuner (only for fc0012). How should the driver implement this situation. Is there a callback I could implement within the tuner or should I just read the tuner registers from the demodulator? Regards Thomas 2012/5/5 poma : > On 05/04/2012 03:49 PM, Gianluca Gennari wrote: >> Hi poma, >> thanks for the very interesting links. >> > ;) > >> Il 04/05/2012 03:27, poma ha scritto: >>> On 05/03/2012 11:03 AM, Gianluca Gennari wrote: Hi poma, I have a 0BDA:2838 (Easycap EZTV646) and a 0BDA:2832 (no name 20x20mm mini DVB-T stick) and both are based on the E4000 tuner, which is not supported in the kernel at the moment. I have no idea if there are sticks with the same USB PID and the fc0012 tuner. >>> >>> OK, second one - no name device is "Realtek RTL2832U reference design"**. >>> >>> First one: >>> Once upon a time there was a "EasyCAP"� >>> "After while crocodile!" >>> �and "EzCAP" was born. >>> http://szforwardvideo.en.alibaba.com/aboutus.html >>> Obviously Easycap EZTV646 != EzCAP EzTV646 >>> http://www.reddit.com/r/RTLSDR/comments/s6ddo/rtlsdr_compatibility_list_v2_work_in_progress/ >>> ezcap EzTV646 0BDA:2838 RTL2832U/FC0012 Some revisions >>> may have the E4000* >>> http://i.imgur.com/mFD1X.jpg >>> (Generic) 0BDa:2838 RTL2832U/E4000* >>> � >>> And, in addition: >>> http://sdr.osmocom.org/trac/wiki/rtl-sdr >>> 0x0bda 0x2832 all of them Generic RTL2832U (e.g. hama nano)** >>> 0x0bda 0x2838 E4000 ezcap USB 2.0 DVB-T/DAB/FM dongle >>> � >>> Maybe? >>> https://sites.google.com/site/myrtlsdr/ >> >> That's it. Opening the device enclosure, I can read this on the PCB: >> "EzTV668 1.0" >> and it looks identical to the picture posted there. >> > Groovy! > >>> "EzCap EZTV646 has got RTL2832U/FC0012. However rtl-sdr must be tweaked >>> to force FC0012 tuner because it has the same PID as EZTV668 (PID: >>> 0x2838) so running it whithout a tweak will select Elonics E4000 tuner. >>> Works, not so good at filtering." >>> � >>> Conclusion: >>> At least two devices share same vid/pid with different tuners - fc0012 >>> vs e4000. >>> How to resolve this from a drivers perspective in a proper way? >> >> This is not a big problem: the rtl2832 driver should read the tuner type >> from an internal register and load the proper module (or exit with an >> error message if the tuner is unsupported). >> > Ack, thanks! > >>> Beside, >>> there is GPL'ed 'e4k' tuner source code aka 'e4000 improved'*** (Elonics >>> E4000) >>> by Harald Welte >>> http://cgit.osmocom.org/cgit/osmo-sdr/tree/firmware/src/tuner_e4k.c >>> http://sdr.osmocom.org/trac/ >>> http://sdr.osmocom.org/trac/wiki/rtl-sdr >>> http://wiki.spench.net/wiki/RTL2832U*** >> >> Very nice. So we should ask Harald Welte if he is willing to have his >> driver merged in the kernel. >> > Undoubtedly! > Please ping Thomas and Antti, accordingly. > >>> regards, >>> poma >>> >> >> Regards, >> Gianluca > > regards, > poma > -- > 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 -- 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
[PATCH] em28xx: Remove unused wait_queue's
Nobody ever waits on any of these wait_queue's,
so this patch removes them completely.
Tested by compilation only.
Signed-off-by: Ezequiel Garcia
---
drivers/media/video/em28xx/em28xx-cards.c |7 ---
drivers/media/video/em28xx/em28xx-video.c |1 -
drivers/media/video/em28xx/em28xx.h |1 -
3 files changed, 0 insertions(+), 9 deletions(-)
diff --git a/drivers/media/video/em28xx/em28xx-cards.c
b/drivers/media/video/em28xx/em28xx-cards.c
index eea90b0..c59e198 100644
--- a/drivers/media/video/em28xx/em28xx-cards.c
+++ b/drivers/media/video/em28xx/em28xx-cards.c
@@ -3007,9 +3007,6 @@ static int em28xx_init_dev(struct em28xx *dev, struct
usb_device *udev,
dev->udev = udev;
mutex_init(&dev->ctrl_urb_lock);
spin_lock_init(&dev->slock);
- init_waitqueue_head(&dev->open);
- init_waitqueue_head(&dev->wait_frame);
- init_waitqueue_head(&dev->wait_stream);
dev->em28xx_write_regs = em28xx_write_regs;
dev->em28xx_read_reg = em28xx_read_reg;
@@ -3447,8 +3444,6 @@ static void em28xx_usb_disconnect(struct usb_interface
*interface)
resources */
mutex_lock(&dev->lock);
- wake_up_interruptible_all(&dev->open);
-
v4l2_device_disconnect(&dev->v4l2_dev);
if (dev->users) {
@@ -3460,8 +3455,6 @@ static void em28xx_usb_disconnect(struct usb_interface
*interface)
dev->state |= DEV_MISCONFIGURED;
em28xx_uninit_isoc(dev, dev->mode);
dev->state |= DEV_DISCONNECTED;
- wake_up_interruptible(&dev->wait_frame);
- wake_up_interruptible(&dev->wait_stream);
} else {
dev->state |= DEV_DISCONNECTED;
em28xx_release_resources(dev);
diff --git a/drivers/media/video/em28xx/em28xx-video.c
b/drivers/media/video/em28xx/em28xx-video.c
index 324b695..8404ec4 100644
--- a/drivers/media/video/em28xx/em28xx-video.c
+++ b/drivers/media/video/em28xx/em28xx-video.c
@@ -2286,7 +2286,6 @@ static int em28xx_v4l2_close(struct file *filp)
videobuf_mmap_free(&fh->vb_vbiq);
kfree(fh);
dev->users--;
- wake_up_interruptible_nr(&dev->open, 1);
return 0;
}
diff --git a/drivers/media/video/em28xx/em28xx.h
b/drivers/media/video/em28xx/em28xx.h
index b5bac9c..b19422e 100644
--- a/drivers/media/video/em28xx/em28xx.h
+++ b/drivers/media/video/em28xx/em28xx.h
@@ -583,7 +583,6 @@ struct em28xx {
struct mutex ctrl_urb_lock; /* protects urb_buf */
/* spinlock_t queue_lock; */
struct list_head inqueue, outqueue;
- wait_queue_head_t open, wait_frame, wait_stream;
struct video_device *vbi_dev;
struct video_device *radio_dev;
--
1.7.3.4
--
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: GENIUS TV-GO A12 tv analog card
Hola Sebastián, :) On Fri, May 4, 2012 at 12:13 PM, Sebastián Misuraca wrote: > Hi, > > I add a tv card support for saa7134 driver, the card name is "Genius > TV Go A12" and i test the RF capture with pal-nc and I test the > composite input too. I want to known if I would make a patch or what i > have to do to give us the patch. Here is the code: > This is not the right way to submit patches and probably maintainers won't apply it. For instance, your patch is not "unified style" (take a look at other patches). You should read Documentation/SubmittingPatches. Also, I think you will find much easier to use git-format-patch and git-send-email to create and send patches. Here is a talk that explains how to use it: "Write and Submit your first Linux kernel Patch" http://www.youtube.com/watch?v=LLBrBBImJt4 If you have any doubts about this process feel free to ask, I'll be glad to help. Regards, Ezequiel. -- 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: [PATCH v2] add support for DeLOCK-USB-2.0-DVB-T-Receiver-61744
Am Samstag, 5. Mai 2012 schrieben Sie:
> I am currently finishing up the work at the demod driver and will
> probably send a new version to the list tomorrow.
>
> As I don't own a device with a different tuner than the fc0012 I will
> include an error message about the unsupported tuner and print its
> type. So It is easier to get the information about the tuners.
>
> Right now I am writing the signal_strength callback and stumbled upon
> the following problem:
> The signal strength is read from the fc0012 tuner (only for fc0012).
> How should the driver implement this situation. Is there a callback I
> could implement within the tuner or should I just read the tuner
> registers from the demodulator?
I would recommend implementing the function into the demod driver. Please see
an example implementation
attached. To be able to decide which tuner is in use, I had to move the
definition of enum rtl28xxu_tuner out of
rtl28xxu.h into an own file. By the way, what is the sense behind naming the
tuners after the demodulator? I
think a more appropriate naming would be TUNER_RTL28XX_...
p.s.: I have written a tuner driver for the fc0013, which I hope to be able to
send to the mailing list later this
weekend.
Cheers,
Hans-Frieder
> [...]
and here is the patch:
--- a/drivers/media/dvb/frontends/rtl2832.c 2012-05-01 12:28:26.407776103
+0200
+++ b/drivers/media/dvb/frontends/rtl2832.c 2012-05-05 18:35:28.778377211
+0200
@@ -19,10 +19,11 @@
*/
#include "rtl2832_priv.h"
+#include "rtl28xxu_tuners.h"
-int rtl2832_debug = 1;
+int rtl2832_debug;
module_param_named(debug, rtl2832_debug, int, 0644);
MODULE_PARM_DESC(debug, "Turn on/off frontend debugging (default:off).");
@@ -391,6 +392,58 @@ err:
}
+static int rtl2832_wr_i2c(struct rtl2832_priv *priv, u8 addr, u8 reg, u8 *val,
int len)
+{
+ int ret;
+ u8 buf[1+len];
+ struct i2c_msg msg[1] = {
+ {
+ .addr = addr,
+ .flags = 0,
+ .len = 1+len,
+ .buf = buf,
+ }
+ };
+
+ buf[0] = reg;
+ memcpy(&buf[1], val, len);
+
+ ret = i2c_transfer(priv->i2c, msg, 1);
+ if (ret == 1) {
+ ret = 0;
+ } else {
+ warn("i2c wr failed=%d reg=%02x len=%d", ret, reg, len);
+ ret = -EREMOTEIO;
+ }
+ return ret;
+}
+
+static int rtl2832_rd_i2c(struct rtl2832_priv *priv, u8 addr, u8 reg, u8 *val,
int len)
+{
+ int ret;
+ struct i2c_msg msg[2] = {
+ {
+ .addr = addr,
+ .flags = 0,
+ .len = 1,
+ .buf = ®,
+ }, {
+ .addr = addr,
+ .flags = I2C_M_RD,
+ .len = len,
+ .buf = val,
+ }
+ };
+
+ ret = i2c_transfer(priv->i2c, msg, 2);
+ if (ret == 2) {
+ ret = 0;
+ } else {
+ warn("i2c rd failed=%d reg=%02x len=%d", ret, reg, len);
+ ret = -EREMOTEIO;
+ }
+ return ret;
+}
static int rtl2832_i2c_gate_ctrl(struct dvb_frontend *fe, int enable)
{
@@ -683,7 +736,9 @@ static int rtl2832_read_status(struct dv
*status = 0;
+#if 0
info("%s", __func__);
+#endif
if (priv->sleeping)
return 0;
@@ -707,32 +762,198 @@ err:
return ret;
}
+#define RTL2832_CE_EST_EVM_MAX_VALUE 65535
+#define RTL2832_SNR_FRAC_BIT_NUM 10
+#define RTL2832_SNR_DB_DEN 3402
+
static int rtl2832_read_snr(struct dvb_frontend *fe, u16 *snr)
{
- info("%s", __func__);
- *snr = 0;
+ struct rtl2832_priv *priv = fe->demodulator_priv;
+ int ret;
+ u32 fsm_stage, ce_est_evm, constellation, hierarchy;
+ int num;
+ static const int snr_db_num_const[3][4] =
+ {
+ {122880,122880, 122880, 122880, },
+ {146657,146657, 156897, 171013, },
+ {167857,167857, 173127, 181810, },
+ };
+
+ ret = rtl2832_rd_demod_reg(priv, DVBT_FSM_STAGE, &fsm_stage);
+ if (ret)
+ goto err;
+
+ if (fsm_stage < 10)
+ ce_est_evm = RTL2832_CE_EST_EVM_MAX_VALUE;
+ else {
+ ret = rtl2832_rd_demod_reg(priv, DVBT_CE_EST_EVM, &ce_est_evm);
+ if (ret)
+ goto err;
+ }
+
+ ret = rtl2832_rd_demod_reg(priv, DVBT_RX_CONSTEL, &constellation);
+ if (ret)
+ goto err;
+ if (constellation > 2)
+ goto err;
+
+ ret = rtl2832_rd_demod_reg(priv, DVBT_RX_HIER, &hierarchy);
+ if (ret)
+ goto err;
+ if (hierarchy > 3)
+ goto err;
+
+ num = snr_db_num_const[constellation][hierarchy] -
+ 10*ilog2(ce_est
Re: [PATCH v2] add support for DeLOCK-USB-2.0-DVB-T-Receiver-61744
la 5.5.2012 19:01 Thomas Mair kirjoitti: > I am currently finishing up the work at the demod driver and will > probably send a new version to the list tomorrow. Nice! I will try to review it on Monday... I looked quickly your old patch last week and tuner driver was done by Hans-Frieder Vogt. I think he should send tuner patch first and then your rtl2832 applied top of that. > As I don't own a device with a different tuner than the fc0012 I will > include an error message about the unsupported tuner and print its > type. So It is easier to get the information about the tuners. Sounds good. > Right now I am writing the signal_strength callback and stumbled upon > the following problem: > The signal strength is read from the fc0012 tuner (only for fc0012). > How should the driver implement this situation. Is there a callback I > could implement within the tuner or should I just read the tuner > registers from the demodulator? Demod should report signal strength, normally based IF AGC. But that estimation is very poor, tuner could report it more accurate. On optimal situation you should implement demod callback for signal strength and if there is tuner callback then override demod callback in order to get better reports. IMHO that override should be done in DVB-USB driver, in that case dvb-usb-rtl2832u. So when you attach rtl2832u just after that override demod callback with tuner. regards Antti -- 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: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
On Sat, 05 May 2012 16:59:30 +0200 Hans de Goede wrote: > > Unless there is another good reason for doing the probing in sd_init I > > prefer > > to move it to sd_config. > > Sensor probing does more then just sensor probing, it also configures > things like the i2c clockrate, and if the bus between bridge and sensor > is spi / i2c or 3-wire, or whatever ... > > After a suspend resume all bets are of wrt bridge state, so we prefer to > always do a full re-init as we do on initial probe, so that we (hopefully) > will put the bridge back in a sane state. > > I think moving the probing from init to config is a bad idea, the chance > that we will get regressions (after a suspend/resume) from this are too > big IMHO. Moving the sensor probing to sd_config is normally safe because the init sequences which are sent actually after probing do all the re-initialization job. An easy way to know it in zc3xx is to force the sensor via the module parameter. -- Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ -- 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: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
On Sat, 05 May 2012 17:05:28 +0200 Hans de Goede wrote: > > Now I see that we are doing exactly that in for example vidioc_g_jpegcomp > > in gspca.c, so > > we should stop doing that. We can make vidioc_g/s_jpegcomp only do the usb > > locking if > > gspca_dev->vdev.ctrl_handler == NULL, and once all sub drivers are > > converted simply remove > > it. Actually I'm thinking about making the jpegqual control part of the > > gspca_dev struct > > itself and move all handling of vidioc_g/s_jpegcomp out of the sub drivers > > and into > > the core. > > Here is an updated version of this patch implementing this approach for > vidioc_g/s_jpegcomp. We may need to do something similar in other places, > although I cannot > think of any such places atm, As the JPEG parameters have been redefined as standard controls, and as there should be only a very few applications which use it, I think the vidioc_g/s_jpegcomp code could be fully removed. -- Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ -- 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
cron job: media_tree daily build: WARNINGS
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date:Sat May 5 19:00:16 CEST 2012 git hash:a1ac5dc28d2b4ca78e183229f7c595ffd725241c gcc version: i686-linux-gcc (GCC) 4.6.3 host hardware:x86_64 host os: 3.1-2.slh.1-amd64 linux-git-arm-eabi-exynos: WARNINGS linux-git-arm-eabi-omap: WARNINGS linux-git-armv5-ixp: WARNINGS linux-git-i686: OK linux-git-m32r: OK linux-git-mips: WARNINGS linux-git-powerpc64: OK linux-git-x86_64: WARNINGS linux-2.6.31.12-i686: WARNINGS linux-2.6.32.6-i686: WARNINGS linux-2.6.33-i686: WARNINGS linux-2.6.34-i686: WARNINGS linux-2.6.35.3-i686: WARNINGS linux-2.6.36-i686: WARNINGS linux-2.6.37-i686: WARNINGS linux-2.6.38.2-i686: WARNINGS linux-2.6.39.1-i686: WARNINGS linux-3.0-i686: OK linux-3.1-i686: OK linux-3.2.1-i686: OK linux-3.3-i686: WARNINGS linux-2.6.31.12-x86_64: WARNINGS linux-2.6.32.6-x86_64: WARNINGS linux-2.6.33-x86_64: WARNINGS linux-2.6.34-x86_64: WARNINGS linux-2.6.35.3-x86_64: WARNINGS linux-2.6.36-x86_64: WARNINGS linux-2.6.37-x86_64: WARNINGS linux-2.6.38.2-x86_64: WARNINGS linux-2.6.39.1-x86_64: WARNINGS linux-3.0-x86_64: WARNINGS linux-3.1-x86_64: WARNINGS linux-3.2.1-x86_64: WARNINGS linux-3.3-x86_64: WARNINGS apps: WARNINGS spec-git: WARNINGS sparse: ERRORS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Saturday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Saturday.tar.bz2 The V4L-DVB specification from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/media.html -- 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
gspca zc3xx - JPEG quality / frame overflow
Hi Hans, I quickly looked at your patches about the changes for the JPEG quality, and I have some remarks. Indeed, as I don't have any zc3xx webcam nor a lot of documentation about the zc3xx bridge, my information come only from USB trace analysis, and I am not sure there are fully valid. - the register 08 always have values 0..3 (bits 0 and 1). I never saw the bits 2 or 3 set when the frame transfer regulation is active. - when frame overflows occur or disappear, the register 07 is always updated before the register 08. There are bug fixes about the setting of the registers 07 and 08 in my gspca test tarball 2.15.17. - as it is (read the register 11 every 100 ms), the work queue is usefull when there is no polling of the snapshot button, because the frame overflow is reported as the bit 0 in the forth byte (data[3]) of the interrupt messages. In fact, in the traces I have, only the webcams which don't do button polling by interrupt messages have to read the register 11. Why only when the sensor is hv7131r or pas202b? I don't know. But with gspca, the polling is always active when CONFIG_INPUT is defined. Regards. -- Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ -- 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
staging/media/as102: Else statements in as10x_cmd.c
Hi all, There are lots of places where if the xfer_cmd ptr is null we are assiging error to AS10X_CMD_ERROR. actually we can do it at the intialisation of error. if you people think its correct, i can submit a change to you people. Thanks, -- 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
[PATCH] em28xx: Fix memory leak on driver defered resource release
When the device is physically unplugged but there are still open file handles, resource release is defered until last opened handle is closed. This patch fixes a missing em28xx_fh struct release. Tested by compilation only. Signed-off-by: Ezequiel Garcia --- drivers/media/video/em28xx/em28xx-video.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/drivers/media/video/em28xx/em28xx-video.c b/drivers/media/video/em28xx/em28xx-video.c index 8404ec4..0a19071 100644 --- a/drivers/media/video/em28xx/em28xx-video.c +++ b/drivers/media/video/em28xx/em28xx-video.c @@ -2262,6 +2262,7 @@ static int em28xx_v4l2_close(struct file *filp) em28xx_release_resources(dev); kfree(dev->alt_max_pkt_size); kfree(dev); + kfree(fh); return 0; } -- 1.7.3.4 -- 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
Initial scan file update for fi-sonera
Update for fi-sonera. Modulation for all channels were changed to QAM256 in April 2012. Source: http://www5.sonera.fi/ohjeet/Kaapeli-TV-taajuudet Mikko -- diff -r 4030c51d6e7b util/scan/dvb-c/fi-sonera --- a/util/scan/dvb-c/fi-sonera Tue Apr 10 16:44:06 2012 +0200 +++ b/util/scan/dvb-c/fi-sonera Wed Apr 25 00:33:44 2012 +0300 @@ -2,22 +2,25 @@ # Maksuttomat kanavat ovat 162 ja 170 MHz:n muxeissa # # freq sr fec mod -C 15400 690 NONE QAM128 -C 16200 690 NONE QAM128 -C 17000 690 NONE QAM128 +C 15400 690 NONE QAM256 +C 16200 690 NONE QAM256 +C 17000 690 NONE QAM256 +C 17800 690 NONE QAM256 C 23400 690 NONE QAM256 C 24200 690 NONE QAM256 C 25000 690 NONE QAM256 C 25800 690 NONE QAM256 C 26600 690 NONE QAM256 -C 31400 690 NONE QAM128 -C 32200 690 NONE QAM128 -C 33800 690 NONE QAM128 -C 34600 690 NONE QAM128 -C 35400 690 NONE QAM128 -C 36200 690 NONE QAM128 -C 37000 690 NONE QAM128 -C 37800 690 NONE QAM128 -C 38600 690 NONE QAM128 -C 39400 690 NONE QAM128 -C 40200 690 NONE QAM128 +C 27400 690 NONE QAM256 +C 31400 690 NONE QAM256 +C 32200 690 NONE QAM256 +C 33000 690 NONE QAM256 +C 33800 690 NONE QAM256 +C 34600 690 NONE QAM256 +C 35400 690 NONE QAM256 +C 36200 690 NONE QAM256 +C 37000 690 NONE QAM256 +C 37800 690 NONE QAM256 +C 38600 690 NONE QAM256 +C 39400 690 NONE QAM256 +C 40200 690 NONE QAM256 -- 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: [media-ctl PATCH 2/3] New, more flexible syntax for media-ctl
Hi Sakari,
On Saturday 05 May 2012 16:09:33 Sakari Ailus wrote:
> On Sat, May 05, 2012 at 02:22:26PM +0200, Laurent Pinchart wrote:
> > On Friday 04 May 2012 11:24:42 Sakari Ailus wrote:
> > > More flexible and extensible syntax for media-ctl which allows better
> > > usage
> > > of the selection API.
> >
> > [snip]
> >
> > > diff --git a/src/options.c b/src/options.c
> > > index 60cf6d5..4d9d48f 100644
> > > --- a/src/options.c
> > > +++ b/src/options.c
> > > @@ -53,12 +53,15 @@ static void usage(const char *argv0, int verbose)
> > >
> > > printf("\n");
> > > printf("Links and formats are defined as\n");
> > > printf("\tlink= pad, '->', pad, '[', flags, ']' ;\n");
> > >
> > > - printf("\tformat = pad, '[', fcc, ' ', size, [ ' ', crop ],
[
> > > '
> > > ', '@', frame interval ], ']' ;\n");
> > > + printf("\tformat = pad, '[', formats ']' ;\n");
> > > + printf("\tformats = formats ',' formats ;\n");
> > > + printf("\tformats = fmt | crop | frame interval ;\n");
> >
> > That's not a valid EBNF. I'm not an expert on the subject, but I think the
> > following is better.
> >
> > formats = format { ' ', formats }
> > format = fmt | crop | frame interval
>
> I'm fine with that change.
>
> > > + printf("\fmt = 'fmt:', fcc, '/', size ;\n");
> >
> > format, formats and fmt are becoming confusing. A different name for
> > 'formats' would be good.
>
> I agree but I didn't immediately come up with something better.
I haven't found a better option at first sight either, let's try to sleep on
it.
> The pixel format and the image size at the pad are clearly format
> (VIDIOC_SUBDEV_S_FMT) but the other things are related to pads but not
> format.
>
> I see them different kinds of properties of pads. That suggests we might be
> better renaming the option (-f) to something else as well.
You like breaking interfaces, don't you ? :-D
> > I find the '/' a bit confusing compared to the ' ' (but I think you find
> > the space confusing compared to '/' :-)). I also wonder whether we
> > shouldn't just drop 'fmt:', as there can be a single format only.
>
> You can set it multiple times, or you may not set it at all. That's why I
> think we should explicitly say it's the format.
Not at all makes sense, but why would you set it multiple times ?
> > > printf("\tpad = entity, ':', pad number ;\n");
> > > printf("\tentity = entity number | ( '\"', entity name, '\"'
> > > )
> > >
> > > ;\n");
> > >
> > > printf("\tsize= width, 'x', height ;\n");
> > >
> > > - printf("\tcrop= '(', left, ',', top, ')', '/', size ;
\n");
> > > - printf("\tframe interval = numerator, '/', denominator ;\n");
> > > + printf("\tcrop= 'crop.actual:', left, ',', top, '/', size
> > > ;\n");
> >
> > Would it make sense to make .actual implicit ? Both 'crop' and
> > 'crop.actual' would be supported by the parser. The code would be more
> > generic if the target was parsed in a generic way, not associated with
> > the rectangle name.
> I've been also thinking does the actual / active really signify something,
> or should that be even dropped form the V4L2 / V4L2 subdev interface
> definitions.
>
> > I would keep the parenthesis around the (top,left) coordinates. You could
> > then define
> >
> > rectangle = '(', left, ',', top, ')', '/', size
> > crop = 'crop' [ '.', target ] ':', rectangle
> > target = 'actual'
> >
> > or something similar.
>
> Sounds good to me.
>
> > What about also keeping compatibility with the existing syntax (both for
> > formats and crop rectangles) ? That shouldn't be too difficult in the
> > parser, crop rectangles start with a '(', and formats come first. We
> > would then have
> >
> > rectangle = '(', left, ',', top, ')', '/', size
> > crop = [ 'crop' [ '.', target ] ':' ], rectangle
> > target = 'actual'
>
> I'll see what I can do. We still should drop the documentation for this so
> that people will stop writing rules that look like that.
Good point, I agree.
--
Regards,
Laurent Pinchart
--
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
error - cx18 driver
Note: I’m a relatively n00b trying to set up mythtv and having issues installing the hauppage hvr-1600 tuner/capture card. When I run dmesg, I get the following: [ 117.013178] a1ac5dc28d2b4ca78e183229f7c595ffd725241c [media] gspca - sn9c20x: Change the exposure setting of Omnivision sensors [ 117.013183] 4fb8137c43ebc0f5bc0dde6b64faa9dd1b1d7970 [media] gspca - sn9c20x: Don't do sensor update before the capture is started [ 117.013188] c4407fe86d3856f60ec711e025bbe9a0159354a3 [media] gspca - sn9c20x: Set the i2c interface speed [ 117.028665] cx18: Unknown symbol i2c_bit_add_bus (err 0) Help. Thanks, H -- 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: error - cx18 driver
Hector Catre wrote: >Note: I’m a relatively n00b trying to set up mythtv and having issues >installing the hauppage hvr-1600 tuner/capture card. > >When I run dmesg, I get the following: > >[ 117.013178] a1ac5dc28d2b4ca78e183229f7c595ffd725241c [media] gspca >- sn9c20x: Change the exposure setting of Omnivision sensors >[ 117.013183] 4fb8137c43ebc0f5bc0dde6b64faa9dd1b1d7970 [media] gspca >- sn9c20x: Don't do sensor update before the capture is started >[ 117.013188] c4407fe86d3856f60ec711e025bbe9a0159354a3 [media] gspca >- sn9c20x: Set the i2c interface speed >[ 117.028665] cx18: Unknown symbol i2c_bit_add_bus (err 0) > >Help. > >Thanks, >H >-- >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 You must ensure i2c_algo_bit.ko exists as a kernel module or that i2c_algo_bit is built into your kernel. Regards, Andy -- 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: error - cx18 driver
Thanks Andy. Again I'm a n00b, thus, can you point me to a page or resource where I can find and install i2c_algo_bit? Thanks again, H -Original Message- From: Andy Walls [mailto:awa...@md.metrocast.net] Sent: May-05-12 8:43 PM To: Hector Catre; linux-media@vger.kernel.org Subject: Re: error - cx18 driver Hector Catre wrote: >Note: I’m a relatively n00b trying to set up mythtv and having issues >installing the hauppage hvr-1600 tuner/capture card. > >When I run dmesg, I get the following: > >[ 117.013178] a1ac5dc28d2b4ca78e183229f7c595ffd725241c [media] gspca >- sn9c20x: Change the exposure setting of Omnivision sensors [ >117.013183] 4fb8137c43ebc0f5bc0dde6b64faa9dd1b1d7970 [media] gspca >- sn9c20x: Don't do sensor update before the capture is started [ >117.013188] c4407fe86d3856f60ec711e025bbe9a0159354a3 [media] gspca >- sn9c20x: Set the i2c interface speed >[ 117.028665] cx18: Unknown symbol i2c_bit_add_bus (err 0) > >Help. > >Thanks, >H >-- >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 You must ensure i2c_algo_bit.ko exists as a kernel module or that i2c_algo_bit is built into your kernel. Regards, Andy -- 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: error - cx18 driver
On 05/06/2012 03:41 AM, Hector Catre wrote: > Thanks Andy. > Again I'm a n00b, thus, can you point me to a page or resource where I can > find and install i2c_algo_bit? > Thanks again, > H > Please paste output of: grep -w 'CONFIG_I2C\|CONFIG_I2C_HELPER_AUTO\|CONFIG_I2C_ALGOBIT' /boot/config-`uname -r` grep -i i2c /etc/modprobe.d/* /etc/modules.conf modinfo i2c-core modinfo i2c_algo_bit lsmod | grep -w 'i2c-core\|i2c_algo_bit' > -Original Message- > From: Andy Walls [mailto:awa...@md.metrocast.net] > Sent: May-05-12 8:43 PM > To: Hector Catre; linux-media@vger.kernel.org > Subject: Re: error - cx18 driver > > Hector Catre wrote: > >> Note: I’m a relatively n00b trying to set up mythtv and having issues >> installing the hauppage hvr-1600 tuner/capture card. >> >> When I run dmesg, I get the following: >> >> [ 117.013178] a1ac5dc28d2b4ca78e183229f7c595ffd725241c [media] gspca >> - sn9c20x: Change the exposure setting of Omnivision sensors [ >> 117.013183] 4fb8137c43ebc0f5bc0dde6b64faa9dd1b1d7970 [media] gspca >> - sn9c20x: Don't do sensor update before the capture is started [ >> 117.013188] c4407fe86d3856f60ec711e025bbe9a0159354a3 [media] gspca >> - sn9c20x: Set the i2c interface speed >> [ 117.028665] cx18: Unknown symbol i2c_bit_add_bus (err 0) >> >> Help. >> >> Thanks, >> H >> -- >> 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 > > You must ensure i2c_algo_bit.ko exists as a kernel module or that > i2c_algo_bit is built into your kernel. > > Regards, > Andy > > -- > 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 -- 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
[PATCH] staging/media/as102: removed else statements
The else statement is actually not required, as we can assign AS10X_CMD_ERROR
to the error variable directly.
Signed-off-by: joseph daniel
---
drivers/staging/media/as102/as10x_cmd.c | 28 +++-
1 file changed, 7 insertions(+), 21 deletions(-)
diff --git a/drivers/staging/media/as102/as10x_cmd.c
b/drivers/staging/media/as102/as10x_cmd.c
index 262bb94..a73df10 100644
--- a/drivers/staging/media/as102/as10x_cmd.c
+++ b/drivers/staging/media/as102/as10x_cmd.c
@@ -31,7 +31,7 @@
*/
int as10x_cmd_turn_on(struct as10x_bus_adapter_t *adap)
{
- int error;
+ int error = AS10X_CMD_ERROR;
struct as10x_cmd_t *pcmd, *prsp;
ENTER();
@@ -54,8 +54,6 @@ int as10x_cmd_turn_on(struct as10x_bus_adapter_t *adap)
(uint8_t *) prsp,
sizeof(prsp->body.turn_on.rsp) +
HEADER_SIZE);
- } else {
- error = AS10X_CMD_ERROR;
}
if (error < 0)
@@ -77,7 +75,7 @@ out:
*/
int as10x_cmd_turn_off(struct as10x_bus_adapter_t *adap)
{
- int error;
+ int error = AS10X_CMD_ERROR;
struct as10x_cmd_t *pcmd, *prsp;
ENTER();
@@ -99,8 +97,6 @@ int as10x_cmd_turn_off(struct as10x_bus_adapter_t *adap)
sizeof(pcmd->body.turn_off.req) + HEADER_SIZE,
(uint8_t *) prsp,
sizeof(prsp->body.turn_off.rsp) + HEADER_SIZE);
- } else {
- error = AS10X_CMD_ERROR;
}
if (error < 0)
@@ -124,7 +120,7 @@ out:
int as10x_cmd_set_tune(struct as10x_bus_adapter_t *adap,
struct as10x_tune_args *ptune)
{
- int error;
+ int error = AS10X_CMD_ERROR;
struct as10x_cmd_t *preq, *prsp;
ENTER();
@@ -159,8 +155,6 @@ int as10x_cmd_set_tune(struct as10x_bus_adapter_t *adap,
(uint8_t *) prsp,
sizeof(prsp->body.set_tune.rsp)
+ HEADER_SIZE);
- } else {
- error = AS10X_CMD_ERROR;
}
if (error < 0)
@@ -184,7 +178,7 @@ out:
int as10x_cmd_get_tune_status(struct as10x_bus_adapter_t *adap,
struct as10x_tune_status *pstatus)
{
- int error;
+ int error = AS10X_CMD_ERROR;
struct as10x_cmd_t *preq, *prsp;
ENTER();
@@ -208,8 +202,6 @@ int as10x_cmd_get_tune_status(struct as10x_bus_adapter_t
*adap,
sizeof(preq->body.get_tune_status.req) + HEADER_SIZE,
(uint8_t *) prsp,
sizeof(prsp->body.get_tune_status.rsp) + HEADER_SIZE);
- } else {
- error = AS10X_CMD_ERROR;
}
if (error < 0)
@@ -241,7 +233,7 @@ out:
*/
int as10x_cmd_get_tps(struct as10x_bus_adapter_t *adap, struct as10x_tps *ptps)
{
- int error;
+ int error = AS10X_CMD_ERROR;
struct as10x_cmd_t *pcmd, *prsp;
ENTER();
@@ -266,8 +258,6 @@ int as10x_cmd_get_tps(struct as10x_bus_adapter_t *adap,
struct as10x_tps *ptps)
(uint8_t *) prsp,
sizeof(prsp->body.get_tps.rsp) +
HEADER_SIZE);
- } else {
- error = AS10X_CMD_ERROR;
}
if (error < 0)
@@ -305,7 +295,7 @@ out:
int as10x_cmd_get_demod_stats(struct as10x_bus_adapter_t *adap,
struct as10x_demod_stats *pdemod_stats)
{
- int error;
+ int error = AS10X_CMD_ERROR;
struct as10x_cmd_t *pcmd, *prsp;
ENTER();
@@ -330,8 +320,6 @@ int as10x_cmd_get_demod_stats(struct as10x_bus_adapter_t
*adap,
(uint8_t *) prsp,
sizeof(prsp->body.get_demod_stats.rsp)
+ HEADER_SIZE);
- } else {
- error = AS10X_CMD_ERROR;
}
if (error < 0)
@@ -370,7 +358,7 @@ out:
int as10x_cmd_get_impulse_resp(struct as10x_bus_adapter_t *adap,
uint8_t *is_ready)
{
- int error;
+ int error = AS10X_CMD_ERROR;
struct as10x_cmd_t *pcmd, *prsp;
ENTER();
@@ -395,8 +383,6 @@ int as10x_cmd_get_impulse_resp(struct as10x_bus_adapter_t
*adap,
(uint8_t *) prsp,
sizeof(prsp->body.get_impulse_rsp.rsp)
+ HEADER_SIZE);
- } else {
- error = AS10X_CMD_ERROR;
}
if (error < 0)
--
1.7.9.5
--
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
