| Message ID | 1238172245.4200.10.camel@tux.localhost (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
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
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 ?
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
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. */