* [question] about open/release and vidioc_g_input/vidioc_s_input functions
@ 2009-03-23 23:14 Alexey Klimov
2009-03-24 7:06 ` Hans Verkuil
0 siblings, 1 reply; 8+ messages in thread
From: Alexey Klimov @ 2009-03-23 23:14 UTC (permalink / raw)
To: linux-media
Cc: Mauro Carvalho Chehab, Hans Verkuil, Douglas Schilling Landgraf
Hello, all
After last convertion of radio drivers to use v4l2_device we have such
code in many radio drivers:
(it's radio-terratec.c for example)
...
static int terratec_open(struct file *file)
{
return 0;
}
static int terratec_release(struct file *file)
{
return 0;
}
...
and
...
static int vidioc_g_input(struct file *filp, void *priv, unsigned int
*i)
{
*i = 0;
return 0;
}
static int vidioc_s_input(struct file *filp, void *priv, unsigned int i)
{
return i ? -EINVAL : 0;
}
...
Such code used in many radio-drivers as i understand.
Is it good to place this empty and almost empty functions in:
(here i see two variants)
1) In header file that be in linux/drivers/media/radio/ directory.
Later, we can move some generic/or repeating code in this header.
2) In any v4l header. What header may contain this ?
?
For what ? Well, as i understand we can decrease amount of lines and
provide this simple generic functions. It's like
video_device_release_empty function behaviour. Maybe not only radio
drivers can use such vidioc_g_input and vidioc_s_input.
Is it worth ?
--
Best regards, Klimov Alexey
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [question] about open/release and vidioc_g_input/vidioc_s_input functions
2009-03-23 23:14 [question] about open/release and vidioc_g_input/vidioc_s_input functions Alexey Klimov
@ 2009-03-24 7:06 ` Hans Verkuil
2009-03-24 9:58 ` Laurent Pinchart
2009-03-27 16:44 ` Alexey Klimov
0 siblings, 2 replies; 8+ messages in thread
From: Hans Verkuil @ 2009-03-24 7:06 UTC (permalink / raw)
To: Alexey Klimov
Cc: linux-media, Mauro Carvalho Chehab, Douglas Schilling Landgraf
On Tuesday 24 March 2009 00:14:07 Alexey Klimov wrote:
> Hello, all
>
> After last convertion of radio drivers to use v4l2_device we have such
> code in many radio drivers:
> (it's radio-terratec.c for example)
>
> ...
> static int terratec_open(struct file *file)
> {
> return 0;
> }
>
> static int terratec_release(struct file *file)
> {
> return 0;
> }
> ...
>
> and
>
> ...
> static int vidioc_g_input(struct file *filp, void *priv, unsigned int
> *i)
> {
> *i = 0;
> return 0;
> }
>
> static int vidioc_s_input(struct file *filp, void *priv, unsigned int i)
> {
> return i ? -EINVAL : 0;
> }
> ...
>
> Such code used in many radio-drivers as i understand.
>
> Is it good to place this empty and almost empty functions in:
> (here i see two variants)
>
> 1) In header file that be in linux/drivers/media/radio/ directory.
> Later, we can move some generic/or repeating code in this header.
>
> 2) In any v4l header. What header may contain this ?
>
> ?
>
> For what ? Well, as i understand we can decrease amount of lines and
> provide this simple generic functions. It's like
> video_device_release_empty function behaviour. Maybe not only radio
> drivers can use such vidioc_g_input and vidioc_s_input.
>
> Is it worth ?
I don't think it is worth doing this for g/s_input. I think it is useful to
have them here: it makes it very clear that there is just a single input
and the overhead in both lines and actual bytes is minimal.
But for the empty open and release functions you could easily handle that in
v4l2-dev.c: if you leave the open and release callbacks to NULL, then
v4l2_open and v4l2_release can just return 0. That would be nice.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [question] about open/release and vidioc_g_input/vidioc_s_input functions
2009-03-24 7:06 ` Hans Verkuil
@ 2009-03-24 9:58 ` Laurent Pinchart
2009-03-27 16:44 ` Alexey Klimov
1 sibling, 0 replies; 8+ messages in thread
From: Laurent Pinchart @ 2009-03-24 9:58 UTC (permalink / raw)
To: Hans Verkuil
Cc: Alexey Klimov, linux-media, Mauro Carvalho Chehab,
Douglas Schilling Landgraf
On Tuesday 24 March 2009 08:06:39 Hans Verkuil wrote:
> On Tuesday 24 March 2009 00:14:07 Alexey Klimov wrote:
> > Hello, all
> >
> > After last convertion of radio drivers to use v4l2_device we have such
> > code in many radio drivers:
> > (it's radio-terratec.c for example)
> >
> > ...
> > static int terratec_open(struct file *file)
> > {
> > return 0;
> > }
> >
> > static int terratec_release(struct file *file)
> > {
> > return 0;
> > }
> > ...
> >
> > and
> >
> > ...
> > static int vidioc_g_input(struct file *filp, void *priv, unsigned int
> > *i)
> > {
> > *i = 0;
> > return 0;
> > }
> >
> > static int vidioc_s_input(struct file *filp, void *priv, unsigned int i)
> > {
> > return i ? -EINVAL : 0;
> > }
> > ...
> >
> > Such code used in many radio-drivers as i understand.
> >
> > Is it good to place this empty and almost empty functions in:
> > (here i see two variants)
> >
> > 1) In header file that be in linux/drivers/media/radio/ directory.
> > Later, we can move some generic/or repeating code in this header.
> >
> > 2) In any v4l header. What header may contain this ?
> >
> > ?
> >
> > For what ? Well, as i understand we can decrease amount of lines and
> > provide this simple generic functions. It's like
> > video_device_release_empty function behaviour. Maybe not only radio
> > drivers can use such vidioc_g_input and vidioc_s_input.
> >
> > Is it worth ?
>
> I don't think it is worth doing this for g/s_input. I think it is useful to
> have them here: it makes it very clear that there is just a single input
> and the overhead in both lines and actual bytes is minimal.
What about making that the default NULL g_input/s_input callbacks handling in
video_ioctl2 ? Devices with a single input wouldn't need to define
g_input/s_input callbacks at all.
> But for the empty open and release functions you could easily handle that
> in v4l2-dev.c: if you leave the open and release callbacks to NULL, then
> v4l2_open and v4l2_release can just return 0. That would be nice.
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [question] about open/release and vidioc_g_input/vidioc_s_input functions
2009-03-24 7:06 ` Hans Verkuil
2009-03-24 9:58 ` Laurent Pinchart
@ 2009-03-27 16:44 ` Alexey Klimov
2009-03-27 16:50 ` Hans Verkuil
1 sibling, 1 reply; 8+ messages in thread
From: Alexey Klimov @ 2009-03-27 16:44 UTC (permalink / raw)
To: Hans Verkuil
Cc: linux-media, Mauro Carvalho Chehab, Douglas Schilling Landgraf
Hello, Hans
On Tue, 2009-03-24 at 08:06 +0100, Hans Verkuil wrote:
> On Tuesday 24 March 2009 00:14:07 Alexey Klimov wrote:
> > Hello, all
> >
> > ...
> > static int terratec_open(struct file *file)
> > {
> > return 0;
> > }
> >
> > static int terratec_release(struct file *file)
> > {
> > return 0;
> > }
> > ...
> >
> > ...
> >
> > Such code used in many radio-drivers as i understand.
> >
> > Is it good to place this empty and almost empty functions in:
> > (here i see two variants)
> >
> > 1) In header file that be in linux/drivers/media/radio/ directory.
> > Later, we can move some generic/or repeating code in this header.
> >
> > 2) In any v4l header. What header may contain this ?
> >
> > ?
> >
> > For what ? Well, as i understand we can decrease amount of lines and
> > provide this simple generic functions. It's like
> > video_device_release_empty function behaviour. Maybe not only radio
> > drivers can use such vidioc_g_input and vidioc_s_input.
> >
> > Is it worth ?
>
> I don't think it is worth doing this for g/s_input. I think it is useful to
> have them here: it makes it very clear that there is just a single input
> and the overhead in both lines and actual bytes is minimal.
>
> But for the empty open and release functions you could easily handle that in
> v4l2-dev.c: if you leave the open and release callbacks to NULL, then
> v4l2_open and v4l2_release can just return 0. That would be nice.
>
> Regards,
>
> Hans
>
May i ask help with this ?
Hans, should it be looks like:
diff -r 56cf0f1772f7 linux/drivers/media/radio/radio-terratec.c
--- a/linux/drivers/media/radio/radio-terratec.c Mon Mar 23 19:18:34 2009 -0300
+++ b/linux/drivers/media/radio/radio-terratec.c Fri Mar 27 19:32:38 2009 +0300
@@ -333,20 +333,8 @@
return a->index ? -EINVAL : 0;
}
-static int terratec_open(struct file *file)
-{
- return 0;
-}
-
-static int terratec_release(struct file *file)
-{
- return 0;
-}
-
static const struct v4l2_file_operations terratec_fops = {
.owner = THIS_MODULE,
- .open = terratec_open,
- .release = terratec_release,
.ioctl = video_ioctl2,
};
diff -r 56cf0f1772f7 linux/drivers/media/video/v4l2-dev.c
--- a/linux/drivers/media/video/v4l2-dev.c Mon Mar 23 19:18:34 2009 -0300
+++ b/linux/drivers/media/video/v4l2-dev.c Fri Mar 27 19:32:38 2009 +0300
@@ -264,7 +264,10 @@
/* and increase the device refcount */
video_get(vdev);
mutex_unlock(&videodev_lock);
- ret = vdev->fops->open(filp);
+ if (vdev->fops->open == NULL)
+ ret = 0;
+ else
+ ret = vdev->fops->open(filp);
/* decrease the refcount in case of an error */
if (ret)
video_put(vdev);
@@ -275,7 +278,12 @@
static int v4l2_release(struct inode *inode, struct file *filp)
{
struct video_device *vdev = video_devdata(filp);
- int ret = vdev->fops->release(filp);
+ int ret;
+
+ if (vdev->fops->release == NULL)
+ ret = 0;
+ else
+ ret = vdev->fops->release(filp);
/* decrease the refcount unconditionally since the release()
return value is ignored. */
?
Or in v4l2_open function i can check if vdev->fops->open == NULL before
video_get(vdev); (increasing the device refcount), and if it's NULL then
unlock_mutex and return 0 ?
And the same in v4l2_release - just return 0 in the begining of function
in case vdev->fops->release == NULL ?
What approach is better ?
--
Best regards, Klimov Alexey
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [question] about open/release and vidioc_g_input/vidioc_s_input functions
2009-03-27 16:44 ` Alexey Klimov
@ 2009-03-27 16:50 ` Hans Verkuil
2009-03-27 17:34 ` Alexey Klimov
0 siblings, 1 reply; 8+ messages in thread
From: Hans Verkuil @ 2009-03-27 16:50 UTC (permalink / raw)
To: Alexey Klimov
Cc: linux-media, Mauro Carvalho Chehab, Douglas Schilling Landgraf
On Friday 27 March 2009 17:44:05 Alexey Klimov wrote:
> Hello, Hans
>
> On Tue, 2009-03-24 at 08:06 +0100, Hans Verkuil wrote:
> > On Tuesday 24 March 2009 00:14:07 Alexey Klimov wrote:
> > > Hello, all
> > >
> > > ...
> > > static int terratec_open(struct file *file)
> > > {
> > > return 0;
> > > }
> > >
> > > static int terratec_release(struct file *file)
> > > {
> > > return 0;
> > > }
> > > ...
> > >
> > > ...
> > >
> > > Such code used in many radio-drivers as i understand.
> > >
> > > Is it good to place this empty and almost empty functions in:
> > > (here i see two variants)
> > >
> > > 1) In header file that be in linux/drivers/media/radio/ directory.
> > > Later, we can move some generic/or repeating code in this header.
> > >
> > > 2) In any v4l header. What header may contain this ?
> > >
> > > ?
> > >
> > > For what ? Well, as i understand we can decrease amount of lines and
> > > provide this simple generic functions. It's like
> > > video_device_release_empty function behaviour. Maybe not only radio
> > > drivers can use such vidioc_g_input and vidioc_s_input.
> > >
> > > Is it worth ?
> >
> > I don't think it is worth doing this for g/s_input. I think it is
> > useful to have them here: it makes it very clear that there is just a
> > single input and the overhead in both lines and actual bytes is
> > minimal.
> >
> > But for the empty open and release functions you could easily handle
> > that in v4l2-dev.c: if you leave the open and release callbacks to
> > NULL, then v4l2_open and v4l2_release can just return 0. That would be
> > nice.
> >
> > Regards,
> >
> > Hans
>
> May i ask help with this ?
> Hans, should it be looks like:
>
> diff -r 56cf0f1772f7 linux/drivers/media/radio/radio-terratec.c
> --- a/linux/drivers/media/radio/radio-terratec.c Mon Mar 23 19:18:34 2009
> -0300 +++ b/linux/drivers/media/radio/radio-terratec.c Fri Mar 27
> 19:32:38 2009 +0300 @@ -333,20 +333,8 @@
> return a->index ? -EINVAL : 0;
> }
>
> -static int terratec_open(struct file *file)
> -{
> - return 0;
> -}
> -
> -static int terratec_release(struct file *file)
> -{
> - return 0;
> -}
> -
> static const struct v4l2_file_operations terratec_fops = {
> .owner = THIS_MODULE,
> - .open = terratec_open,
> - .release = terratec_release,
> .ioctl = video_ioctl2,
> };
>
> diff -r 56cf0f1772f7 linux/drivers/media/video/v4l2-dev.c
> --- a/linux/drivers/media/video/v4l2-dev.c Mon Mar 23 19:18:34 2009 -0300
> +++ b/linux/drivers/media/video/v4l2-dev.c Fri Mar 27 19:32:38 2009 +0300
> @@ -264,7 +264,10 @@
> /* and increase the device refcount */
> video_get(vdev);
> mutex_unlock(&videodev_lock);
> - ret = vdev->fops->open(filp);
> + if (vdev->fops->open == NULL)
> + ret = 0;
> + else
> + ret = vdev->fops->open(filp);
> /* decrease the refcount in case of an error */
> if (ret)
> video_put(vdev);
> @@ -275,7 +278,12 @@
> static int v4l2_release(struct inode *inode, struct file *filp)
> {
> struct video_device *vdev = video_devdata(filp);
> - int ret = vdev->fops->release(filp);
> + int ret;
> +
> + if (vdev->fops->release == NULL)
> + ret = 0;
> + else
> + ret = vdev->fops->release(filp);
>
> /* decrease the refcount unconditionally since the release()
> return value is ignored. */
>
> ?
>
> Or in v4l2_open function i can check if vdev->fops->open == NULL before
> video_get(vdev); (increasing the device refcount), and if it's NULL then
> unlock_mutex and return 0 ?
> And the same in v4l2_release - just return 0 in the begining of function
> in case vdev->fops->release == NULL ?
>
> What approach is better ?
This is simpler:
diff -r 2e0c6ff1bda3 linux/drivers/media/video/v4l2-dev.c
--- a/linux/drivers/media/video/v4l2-dev.c Mon Mar 23 19:01:18 2009
+0100
+++ b/linux/drivers/media/video/v4l2-dev.c Fri Mar 27 17:47:51 2009
+0100
@@ -250,7 +250,7 @@
static int v4l2_open(struct inode *inode, struct file *filp)
{
struct video_device *vdev;
- int ret;
+ int ret = 0;
/* Check if the video device is available */
mutex_lock(&videodev_lock);
@@ -264,7 +264,9 @@
/* and increase the device refcount */
video_get(vdev);
mutex_unlock(&videodev_lock);
- ret = vdev->fops->open(filp);
+ if (vdev->fops->open)
+ ret = vdev->fops->open(filp);
+
/* decrease the refcount in case of an error */
if (ret)
video_put(vdev);
@@ -275,7 +277,10 @@
static int v4l2_release(struct inode *inode, struct file *filp)
{
struct video_device *vdev = video_devdata(filp);
- int ret = vdev->fops->release(filp);
+ int ret = 0;
+
+ if (vdev->fops->release)
+ ret = vdev->fops->release(filp);
/* decrease the refcount unconditionally since the release()
return value is ignored. */
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [question] about open/release and vidioc_g_input/vidioc_s_input functions
2009-03-27 16:50 ` Hans Verkuil
@ 2009-03-27 17:34 ` Alexey Klimov
2009-03-27 17:45 ` Hans Verkuil
0 siblings, 1 reply; 8+ messages in thread
From: Alexey Klimov @ 2009-03-27 17:34 UTC (permalink / raw)
To: Hans Verkuil
Cc: linux-media, Mauro Carvalho Chehab, Douglas Schilling Landgraf
On Fri, Mar 27, 2009 at 7:50 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On Friday 27 March 2009 17:44:05 Alexey Klimov wrote:
>> Hello, Hans
>>
>> On Tue, 2009-03-24 at 08:06 +0100, Hans Verkuil wrote:
>> > On Tuesday 24 March 2009 00:14:07 Alexey Klimov wrote:
>> > > Hello, all
>> > >
>> > > ...
>> > > static int terratec_open(struct file *file)
>> > > {
>> > > return 0;
>> > > }
>> > >
>> > > static int terratec_release(struct file *file)
>> > > {
>> > > return 0;
>> > > }
>> > > ...
>> > >
>> > > ...
>> > >
>> > > Such code used in many radio-drivers as i understand.
>> > >
>> > > Is it good to place this empty and almost empty functions in:
>> > > (here i see two variants)
>> > >
>> > > 1) In header file that be in linux/drivers/media/radio/ directory.
>> > > Later, we can move some generic/or repeating code in this header.
>> > >
>> > > 2) In any v4l header. What header may contain this ?
>> > >
>> > > ?
>> > >
>> > > For what ? Well, as i understand we can decrease amount of lines and
>> > > provide this simple generic functions. It's like
>> > > video_device_release_empty function behaviour. Maybe not only radio
>> > > drivers can use such vidioc_g_input and vidioc_s_input.
>> > >
>> > > Is it worth ?
>> >
>> > I don't think it is worth doing this for g/s_input. I think it is
>> > useful to have them here: it makes it very clear that there is just a
>> > single input and the overhead in both lines and actual bytes is
>> > minimal.
>> >
>> > But for the empty open and release functions you could easily handle
>> > that in v4l2-dev.c: if you leave the open and release callbacks to
>> > NULL, then v4l2_open and v4l2_release can just return 0. That would be
>> > nice.
>> >
>> > Regards,
>> >
>> > Hans
>>
>> May i ask help with this ?
>> Hans, should it be looks like:
>>
>> diff -r 56cf0f1772f7 linux/drivers/media/radio/radio-terratec.c
>> --- a/linux/drivers/media/radio/radio-terratec.c Mon Mar 23 19:18:34 2009
>> -0300 +++ b/linux/drivers/media/radio/radio-terratec.c Fri Mar 27
>> 19:32:38 2009 +0300 @@ -333,20 +333,8 @@
>> return a->index ? -EINVAL : 0;
>> }
>>
>> -static int terratec_open(struct file *file)
>> -{
>> - return 0;
>> -}
>> -
>> -static int terratec_release(struct file *file)
>> -{
>> - return 0;
>> -}
>> -
>> static const struct v4l2_file_operations terratec_fops = {
>> .owner = THIS_MODULE,
>> - .open = terratec_open,
>> - .release = terratec_release,
>> .ioctl = video_ioctl2,
>> };
>>
>> diff -r 56cf0f1772f7 linux/drivers/media/video/v4l2-dev.c
>> --- a/linux/drivers/media/video/v4l2-dev.c Mon Mar 23 19:18:34 2009 -0300
>> +++ b/linux/drivers/media/video/v4l2-dev.c Fri Mar 27 19:32:38 2009 +0300
>> @@ -264,7 +264,10 @@
>> /* and increase the device refcount */
>> video_get(vdev);
>> mutex_unlock(&videodev_lock);
>> - ret = vdev->fops->open(filp);
>> + if (vdev->fops->open == NULL)
>> + ret = 0;
>> + else
>> + ret = vdev->fops->open(filp);
>> /* decrease the refcount in case of an error */
>> if (ret)
>> video_put(vdev);
>> @@ -275,7 +278,12 @@
>> static int v4l2_release(struct inode *inode, struct file *filp)
>> {
>> struct video_device *vdev = video_devdata(filp);
>> - int ret = vdev->fops->release(filp);
>> + int ret;
>> +
>> + if (vdev->fops->release == NULL)
>> + ret = 0;
>> + else
>> + ret = vdev->fops->release(filp);
>>
>> /* decrease the refcount unconditionally since the release()
>> return value is ignored. */
>>
>> ?
>>
>> Or in v4l2_open function i can check if vdev->fops->open == NULL before
>> video_get(vdev); (increasing the device refcount), and if it's NULL then
>> unlock_mutex and return 0 ?
>> And the same in v4l2_release - just return 0 in the begining of function
>> in case vdev->fops->release == NULL ?
>>
>> What approach is better ?
>
> This is simpler:
>
> diff -r 2e0c6ff1bda3 linux/drivers/media/video/v4l2-dev.c
> --- a/linux/drivers/media/video/v4l2-dev.c Mon Mar 23 19:01:18 2009
> +0100
> +++ b/linux/drivers/media/video/v4l2-dev.c Fri Mar 27 17:47:51 2009
> +0100
> @@ -250,7 +250,7 @@
> static int v4l2_open(struct inode *inode, struct file *filp)
> {
> struct video_device *vdev;
> - int ret;
> + int ret = 0;
>
> /* Check if the video device is available */
> mutex_lock(&videodev_lock);
> @@ -264,7 +264,9 @@
> /* and increase the device refcount */
> video_get(vdev);
> mutex_unlock(&videodev_lock);
> - ret = vdev->fops->open(filp);
> + if (vdev->fops->open)
> + ret = vdev->fops->open(filp);
> +
> /* decrease the refcount in case of an error */
> if (ret)
> video_put(vdev);
> @@ -275,7 +277,10 @@
> static int v4l2_release(struct inode *inode, struct file *filp)
> {
> struct video_device *vdev = video_devdata(filp);
> - int ret = vdev->fops->release(filp);
> + int ret = 0;
> +
> + if (vdev->fops->release)
> + ret = vdev->fops->release(filp);
>
> /* decrease the refcount unconditionally since the release()
> return value is ignored. */
Looks like you already did right patch ;-)
I don't know what to do, should i repost this like patch ?
--
Best regards, Klimov Alexey
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [question] about open/release and vidioc_g_input/vidioc_s_input functions
2009-03-27 17:34 ` Alexey Klimov
@ 2009-03-27 17:45 ` Hans Verkuil
0 siblings, 0 replies; 8+ messages in thread
From: Hans Verkuil @ 2009-03-27 17:45 UTC (permalink / raw)
To: Alexey Klimov
Cc: linux-media, Mauro Carvalho Chehab, Douglas Schilling Landgraf
On Friday 27 March 2009 18:34:01 Alexey Klimov wrote:
> On Fri, Mar 27, 2009 at 7:50 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > On Friday 27 March 2009 17:44:05 Alexey Klimov wrote:
> >> Hello, Hans
> >>
> >> On Tue, 2009-03-24 at 08:06 +0100, Hans Verkuil wrote:
> >> > On Tuesday 24 March 2009 00:14:07 Alexey Klimov wrote:
> >> > > Hello, all
> >> > >
> >> > > ...
> >> > > static int terratec_open(struct file *file)
> >> > > {
> >> > > return 0;
> >> > > }
> >> > >
> >> > > static int terratec_release(struct file *file)
> >> > > {
> >> > > return 0;
> >> > > }
> >> > > ...
> >> > >
> >> > > ...
> >> > >
> >> > > Such code used in many radio-drivers as i understand.
> >> > >
> >> > > Is it good to place this empty and almost empty functions in:
> >> > > (here i see two variants)
> >> > >
> >> > > 1) In header file that be in linux/drivers/media/radio/ directory.
> >> > > Later, we can move some generic/or repeating code in this header.
> >> > >
> >> > > 2) In any v4l header. What header may contain this ?
> >> > >
> >> > > ?
> >> > >
> >> > > For what ? Well, as i understand we can decrease amount of lines
> >> > > and provide this simple generic functions. It's like
> >> > > video_device_release_empty function behaviour. Maybe not only
> >> > > radio drivers can use such vidioc_g_input and vidioc_s_input.
> >> > >
> >> > > Is it worth ?
> >> >
> >> > I don't think it is worth doing this for g/s_input. I think it is
> >> > useful to have them here: it makes it very clear that there is just
> >> > a single input and the overhead in both lines and actual bytes is
> >> > minimal.
> >> >
> >> > But for the empty open and release functions you could easily handle
> >> > that in v4l2-dev.c: if you leave the open and release callbacks to
> >> > NULL, then v4l2_open and v4l2_release can just return 0. That would
> >> > be nice.
> >> >
> >> > Regards,
> >> >
> >> > Hans
> >>
> >> May i ask help with this ?
> >> Hans, should it be looks like:
> >>
> >> diff -r 56cf0f1772f7 linux/drivers/media/radio/radio-terratec.c
> >> --- a/linux/drivers/media/radio/radio-terratec.c Mon Mar 23
> >> 19:18:34 2009 -0300 +++ b/linux/drivers/media/radio/radio-terratec.c
> >> Fri Mar 27 19:32:38 2009 +0300 @@ -333,20 +333,8 @@
> >> return a->index ? -EINVAL : 0;
> >> }
> >>
> >> -static int terratec_open(struct file *file)
> >> -{
> >> - return 0;
> >> -}
> >> -
> >> -static int terratec_release(struct file *file)
> >> -{
> >> - return 0;
> >> -}
> >> -
> >> static const struct v4l2_file_operations terratec_fops = {
> >> .owner = THIS_MODULE,
> >> - .open = terratec_open,
> >> - .release = terratec_release,
> >> .ioctl = video_ioctl2,
> >> };
> >>
> >> diff -r 56cf0f1772f7 linux/drivers/media/video/v4l2-dev.c
> >> --- a/linux/drivers/media/video/v4l2-dev.c Mon Mar 23 19:18:34 2009
> >> -0300 +++ b/linux/drivers/media/video/v4l2-dev.c Fri Mar 27
> >> 19:32:38 2009 +0300 @@ -264,7 +264,10 @@
> >> /* and increase the device refcount */
> >> video_get(vdev);
> >> mutex_unlock(&videodev_lock);
> >> - ret = vdev->fops->open(filp);
> >> + if (vdev->fops->open == NULL)
> >> + ret = 0;
> >> + else
> >> + ret = vdev->fops->open(filp);
> >> /* decrease the refcount in case of an error */
> >> if (ret)
> >> video_put(vdev);
> >> @@ -275,7 +278,12 @@
> >> static int v4l2_release(struct inode *inode, struct file *filp)
> >> {
> >> struct video_device *vdev = video_devdata(filp);
> >> - int ret = vdev->fops->release(filp);
> >> + int ret;
> >> +
> >> + if (vdev->fops->release == NULL)
> >> + ret = 0;
> >> + else
> >> + ret = vdev->fops->release(filp);
> >>
> >> /* decrease the refcount unconditionally since the release()
> >> return value is ignored. */
> >>
> >> ?
> >>
> >> Or in v4l2_open function i can check if vdev->fops->open == NULL
> >> before video_get(vdev); (increasing the device refcount), and if it's
> >> NULL then unlock_mutex and return 0 ?
> >> And the same in v4l2_release - just return 0 in the begining of
> >> function in case vdev->fops->release == NULL ?
> >>
> >> What approach is better ?
> >
> > This is simpler:
> >
> > diff -r 2e0c6ff1bda3 linux/drivers/media/video/v4l2-dev.c
> > --- a/linux/drivers/media/video/v4l2-dev.c Mon Mar 23 19:01:18
> > 2009 +0100
> > +++ b/linux/drivers/media/video/v4l2-dev.c Fri Mar 27 17:47:51
> > 2009 +0100
> > @@ -250,7 +250,7 @@
> > static int v4l2_open(struct inode *inode, struct file *filp)
> > {
> > struct video_device *vdev;
> > - int ret;
> > + int ret = 0;
> >
> > /* Check if the video device is available */
> > mutex_lock(&videodev_lock);
> > @@ -264,7 +264,9 @@
> > /* and increase the device refcount */
> > video_get(vdev);
> > mutex_unlock(&videodev_lock);
> > - ret = vdev->fops->open(filp);
> > + if (vdev->fops->open)
> > + ret = vdev->fops->open(filp);
> > +
> > /* decrease the refcount in case of an error */
> > if (ret)
> > video_put(vdev);
> > @@ -275,7 +277,10 @@
> > static int v4l2_release(struct inode *inode, struct file *filp)
> > {
> > struct video_device *vdev = video_devdata(filp);
> > - int ret = vdev->fops->release(filp);
> > + int ret = 0;
> > +
> > + if (vdev->fops->release)
> > + ret = vdev->fops->release(filp);
> >
> > /* decrease the refcount unconditionally since the release()
> > return value is ignored. */
>
> Looks like you already did right patch ;-)
> I don't know what to do, should i repost this like patch ?
Just turn it into a patch series with this as the first patch and fixing the
radio drivers that can use it.
Here's my SoB for this one:
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [question] about open/release and vidioc_g_input/vidioc_s_input functions
@ 2009-03-24 10:24 Hans Verkuil
0 siblings, 0 replies; 8+ messages in thread
From: Hans Verkuil @ 2009-03-24 10:24 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Alexey Klimov, linux-media, Mauro Carvalho Chehab,
Douglas Schilling Landgraf
> On Tuesday 24 March 2009 08:06:39 Hans Verkuil wrote:
>> On Tuesday 24 March 2009 00:14:07 Alexey Klimov wrote:
>> > Hello, all
>> >
>> > After last convertion of radio drivers to use v4l2_device we have such
>> > code in many radio drivers:
>> > (it's radio-terratec.c for example)
>> >
>> > ...
>> > static int terratec_open(struct file *file)
>> > {
>> > return 0;
>> > }
>> >
>> > static int terratec_release(struct file *file)
>> > {
>> > return 0;
>> > }
>> > ...
>> >
>> > and
>> >
>> > ...
>> > static int vidioc_g_input(struct file *filp, void *priv, unsigned int
>> > *i)
>> > {
>> > *i = 0;
>> > return 0;
>> > }
>> >
>> > static int vidioc_s_input(struct file *filp, void *priv, unsigned int
>> i)
>> > {
>> > return i ? -EINVAL : 0;
>> > }
>> > ...
>> >
>> > Such code used in many radio-drivers as i understand.
>> >
>> > Is it good to place this empty and almost empty functions in:
>> > (here i see two variants)
>> >
>> > 1) In header file that be in linux/drivers/media/radio/ directory.
>> > Later, we can move some generic/or repeating code in this header.
>> >
>> > 2) In any v4l header. What header may contain this ?
>> >
>> > ?
>> >
>> > For what ? Well, as i understand we can decrease amount of lines and
>> > provide this simple generic functions. It's like
>> > video_device_release_empty function behaviour. Maybe not only radio
>> > drivers can use such vidioc_g_input and vidioc_s_input.
>> >
>> > Is it worth ?
>>
>> I don't think it is worth doing this for g/s_input. I think it is useful
>> to
>> have them here: it makes it very clear that there is just a single input
>> and the overhead in both lines and actual bytes is minimal.
>
> What about making that the default NULL g_input/s_input callbacks handling
> in
> video_ioctl2 ? Devices with a single input wouldn't need to define
> g_input/s_input callbacks at all.
If you leave s/g_input NULL then you have a device without an input at
all. That's perfectly valid for output-only devices.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2009-03-27 17:45 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-03-23 23:14 [question] about open/release and vidioc_g_input/vidioc_s_input functions Alexey Klimov
2009-03-24 7:06 ` Hans Verkuil
2009-03-24 9:58 ` Laurent Pinchart
2009-03-27 16:44 ` Alexey Klimov
2009-03-27 16:50 ` Hans Verkuil
2009-03-27 17:34 ` Alexey Klimov
2009-03-27 17:45 ` Hans Verkuil
-- strict thread matches above, loose matches on Subject: below --
2009-03-24 10:24 Hans Verkuil
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox