On Fri, Mar 27, 2009 at 7:50 PM, Hans Verkuil <hverkuil@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
