[RFC PATCH 0/7] Simplify first open/last close checks

[RFC PATCH 0/7] Simplify first open/last close checks

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This patch series makes a number of API improvements to simplify checking
if an open or close of a filehandle was the first or last.

Regards,

	Hans

Hans Verkuil (7):
  v4l2-fh: change int to bool for v4l2_fh_is_singular(_file)
  v4l2-fh: v4l2_fh_add/del now return whether it was the first or last
    fh.
  v4l2-fh: add v4l2_fh_open_is_first and v4l2_fh_release_is_last
  am437x-vpfe/fimc-capture: always return 0 on close
  vb2: _vb2_fop_release returns if this was the last fh.
  am437x/exynos4-is/marvell-ccic/sh_vou: simplify release()
  cpia2/si470x: simplify open

 drivers/media/platform/am437x/am437x-vpfe.c      | 16 ++------
 drivers/media/platform/exynos4-is/fimc-capture.c |  8 +---
 drivers/media/platform/marvell-ccic/mcam-core.c  |  5 +--
 drivers/media/platform/sh_vou.c                  |  5 +--
 drivers/media/radio/si470x/radio-si470x-i2c.c    | 21 ++++-------
 drivers/media/usb/cpia2/cpia2_v4l.c              |  5 +--
 drivers/media/v4l2-core/v4l2-fh.c                | 41 +++++++++++++--------
 drivers/media/v4l2-core/videobuf2-core.c         |  7 ++--
 include/media/v4l2-fh.h                          | 47 ++++++++++++++++++------
 include/media/videobuf2-core.h                   |  2 +-
 10 files changed, 82 insertions(+), 75 deletions(-)

-- 
2.1.4

[RFC PATCH 1/7] v4l2-fh: change int to bool for v4l2_fh_is_singular(_file)

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This function really returns a bool, so use that as the return type
instead of int.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-fh.c |  6 +++---
 include/media/v4l2-fh.h           | 10 +++++-----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c
index c97067a..20c0a0c 100644
--- a/drivers/media/v4l2-core/v4l2-fh.c
+++ b/drivers/media/v4l2-core/v4l2-fh.c
@@ -110,13 +110,13 @@ int v4l2_fh_release(struct file *filp)
 }
 EXPORT_SYMBOL_GPL(v4l2_fh_release);
 
-int v4l2_fh_is_singular(struct v4l2_fh *fh)
+bool v4l2_fh_is_singular(struct v4l2_fh *fh)
 {
 	unsigned long flags;
-	int is_singular;
+	bool is_singular;
 
 	if (fh == NULL || fh->vdev == NULL)
-		return 0;
+		return false;
 	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
 	is_singular = list_is_singular(&fh->list);
 	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
index 8035167..13dcaae 100644
--- a/include/media/v4l2-fh.h
+++ b/include/media/v4l2-fh.h
@@ -91,15 +91,15 @@ void v4l2_fh_exit(struct v4l2_fh *fh);
  */
 int v4l2_fh_release(struct file *filp);
 /*
- * Returns 1 if this filehandle is the only filehandle opened for the
- * associated video_device. If fh is NULL, then it returns 0.
+ * Returns true if this filehandle is the only filehandle opened for the
+ * associated video_device. If fh is NULL, then it returns false.
  */
-int v4l2_fh_is_singular(struct v4l2_fh *fh);
+bool v4l2_fh_is_singular(struct v4l2_fh *fh);
 /*
  * Helper function with struct file as argument. If filp->private_data is
- * NULL, then it will return 0.
+ * NULL, then it will return false.
  */
-static inline int v4l2_fh_is_singular_file(struct file *filp)
+static inline bool v4l2_fh_is_singular_file(struct file *filp)
 {
 	return v4l2_fh_is_singular(filp->private_data);
 }
-- 
2.1.4

[RFC PATCH 2/7] v4l2-fh: v4l2_fh_add/del now return whether it was the first or last fh.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Simplify driver code by letting v4l2_fh_add/del return true if the
filehandle was the first open or the last close.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-fh.c | 10 ++++++++--
 include/media/v4l2-fh.h           | 10 ++++++----
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c
index 20c0a0c..325e378 100644
--- a/drivers/media/v4l2-core/v4l2-fh.c
+++ b/drivers/media/v4l2-core/v4l2-fh.c
@@ -52,14 +52,17 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
 }
 EXPORT_SYMBOL_GPL(v4l2_fh_init);
 
-void v4l2_fh_add(struct v4l2_fh *fh)
+bool v4l2_fh_add(struct v4l2_fh *fh)
 {
 	unsigned long flags;
+	bool is_first;
 
 	v4l2_prio_open(fh->vdev->prio, &fh->prio);
 	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+	is_first = list_empty(&fh->vdev->fh_list);
 	list_add(&fh->list, &fh->vdev->fh_list);
 	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+	return is_first;
 }
 EXPORT_SYMBOL_GPL(v4l2_fh_add);
 
@@ -77,14 +80,17 @@ int v4l2_fh_open(struct file *filp)
 }
 EXPORT_SYMBOL_GPL(v4l2_fh_open);
 
-void v4l2_fh_del(struct v4l2_fh *fh)
+bool v4l2_fh_del(struct v4l2_fh *fh)
 {
 	unsigned long flags;
+	bool is_last;
 
 	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
 	list_del_init(&fh->list);
+	is_last = list_empty(&fh->vdev->fh_list);
 	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
 	v4l2_prio_close(fh->vdev->prio, fh->prio);
+	return is_last;
 }
 EXPORT_SYMBOL_GPL(v4l2_fh_del);
 
diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
index 13dcaae..17215fc 100644
--- a/include/media/v4l2-fh.h
+++ b/include/media/v4l2-fh.h
@@ -60,9 +60,10 @@ struct v4l2_fh {
 void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev);
 /*
  * Add the fh to the list of file handles on a video_device. The file
- * handle must be initialised first.
+ * handle must be initialised first. This function will return true
+ * if the new filehandle was the first filehandle added and false otherwise.
  */
-void v4l2_fh_add(struct v4l2_fh *fh);
+bool v4l2_fh_add(struct v4l2_fh *fh);
 /*
  * Can be used as the open() op of v4l2_file_operations.
  * It allocates a v4l2_fh and inits and adds it to the video_device associated
@@ -73,9 +74,10 @@ int v4l2_fh_open(struct file *filp);
  * Remove file handle from the list of file handles. Must be called in
  * v4l2_file_operations->release() handler if the driver uses v4l2_fh.
  * On error filp->private_data will be NULL, otherwise it will point to
- * the v4l2_fh struct.
+ * the v4l2_fh struct. This function will return true if this was the
+ * last open filehandle and false otherwise.
  */
-void v4l2_fh_del(struct v4l2_fh *fh);
+bool v4l2_fh_del(struct v4l2_fh *fh);
 /*
  * Release resources related to a file handle. Parts of the V4L2
  * framework using the v4l2_fh must release their resources here, too.
-- 
2.1.4

[RFC PATCH 3/7] v4l2-fh: add v4l2_fh_open_is_first and v4l2_fh_release_is_last

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Add new helper functions that report back if this was the first open
or last close.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-fh.c | 25 ++++++++++++++-----------
 include/media/v4l2-fh.h           | 27 +++++++++++++++++++++++++--
 2 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c
index 325e378..ac9c028 100644
--- a/drivers/media/v4l2-core/v4l2-fh.c
+++ b/drivers/media/v4l2-core/v4l2-fh.c
@@ -66,19 +66,21 @@ bool v4l2_fh_add(struct v4l2_fh *fh)
 }
 EXPORT_SYMBOL_GPL(v4l2_fh_add);
 
-int v4l2_fh_open(struct file *filp)
+bool v4l2_fh_open_is_first(struct file *filp, int *err)
 {
 	struct video_device *vdev = video_devdata(filp);
 	struct v4l2_fh *fh = kzalloc(sizeof(*fh), GFP_KERNEL);
 
+	*err = fh ? -ENOMEM : 0;
+
 	filp->private_data = fh;
-	if (fh == NULL)
-		return -ENOMEM;
-	v4l2_fh_init(fh, vdev);
-	v4l2_fh_add(fh);
-	return 0;
+	if (fh) {
+		v4l2_fh_init(fh, vdev);
+		return v4l2_fh_add(fh);
+	}
+	return false;
 }
-EXPORT_SYMBOL_GPL(v4l2_fh_open);
+EXPORT_SYMBOL_GPL(v4l2_fh_open_is_first);
 
 bool v4l2_fh_del(struct v4l2_fh *fh)
 {
@@ -103,18 +105,19 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
 }
 EXPORT_SYMBOL_GPL(v4l2_fh_exit);
 
-int v4l2_fh_release(struct file *filp)
+bool v4l2_fh_release_is_last(struct file *filp)
 {
 	struct v4l2_fh *fh = filp->private_data;
+	bool is_last = false;
 
 	if (fh) {
-		v4l2_fh_del(fh);
+		is_last = v4l2_fh_del(fh);
 		v4l2_fh_exit(fh);
 		kfree(fh);
 	}
-	return 0;
+	return is_last;
 }
-EXPORT_SYMBOL_GPL(v4l2_fh_release);
+EXPORT_SYMBOL_GPL(v4l2_fh_release_is_last);
 
 bool v4l2_fh_is_singular(struct v4l2_fh *fh)
 {
diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
index 17215fc..e937866 100644
--- a/include/media/v4l2-fh.h
+++ b/include/media/v4l2-fh.h
@@ -65,11 +65,23 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev);
  */
 bool v4l2_fh_add(struct v4l2_fh *fh);
 /*
+ * It allocates a v4l2_fh and inits and adds it to the video_device associated
+ * with the file pointer. In addition it returns true if this was the first
+ * open and false otherwise. The error code is returned in *err.
+ */
+bool v4l2_fh_open_is_first(struct file *filp, int *err);
+/*
  * Can be used as the open() op of v4l2_file_operations.
  * It allocates a v4l2_fh and inits and adds it to the video_device associated
  * with the file pointer.
  */
-int v4l2_fh_open(struct file *filp);
+static inline int v4l2_fh_open(struct file *filp)
+{
+	int err;
+
+	v4l2_fh_open_is_first(filp, &err);
+	return err;
+}
 /*
  * Remove file handle from the list of file handles. Must be called in
  * v4l2_file_operations->release() handler if the driver uses v4l2_fh.
@@ -86,12 +98,23 @@ bool v4l2_fh_del(struct v4l2_fh *fh);
  */
 void v4l2_fh_exit(struct v4l2_fh *fh);
 /*
+ * It deletes and exits the v4l2_fh associated with the file pointer and
+ * frees it. It will do nothing if filp->private_data (the pointer to the
+ * v4l2_fh struct) is NULL. This function returns true if this was the
+ * last open file handle and false otherwise.
+ */
+bool v4l2_fh_release_is_last(struct file *filp);
+/*
  * Can be used as the release() op of v4l2_file_operations.
  * It deletes and exits the v4l2_fh associated with the file pointer and
  * frees it. It will do nothing if filp->private_data (the pointer to the
  * v4l2_fh struct) is NULL. This function always returns 0.
  */
-int v4l2_fh_release(struct file *filp);
+static inline int v4l2_fh_release(struct file *filp)
+{
+	v4l2_fh_release_is_last(filp);
+	return 0;
+}
 /*
  * Returns true if this filehandle is the only filehandle opened for the
  * associated video_device. If fh is NULL, then it returns false.
-- 
2.1.4

[RFC PATCH 4/7] am437x-vpfe/fimc-capture: always return 0 on close

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

When the filehandle is closed always return 0 and ignore the return
code from _vb2_fop_release().

Currently _vb2_fop_release() always returns 0, but this will change in
the next patch where _vb2_fop_release() will return a boolean telling the
caller if this was the last open filehandle that is closed.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/am437x/am437x-vpfe.c      | 5 ++---
 drivers/media/platform/exynos4-is/fimc-capture.c | 5 ++---
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index c8447fa..210c779 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -1187,7 +1187,6 @@ static int vpfe_release(struct file *file)
 {
 	struct vpfe_device *vpfe = video_drvdata(file);
 	bool fh_singular;
-	int ret;
 
 	mutex_lock(&vpfe->lock);
 
@@ -1195,7 +1194,7 @@ static int vpfe_release(struct file *file)
 	fh_singular = v4l2_fh_is_singular_file(file);
 
 	/* the release helper will cleanup any on-going streaming */
-	ret = _vb2_fop_release(file, NULL);
+	_vb2_fop_release(file, NULL);
 
 	/*
 	 * If this was the last open file.
@@ -1206,7 +1205,7 @@ static int vpfe_release(struct file *file)
 
 	mutex_unlock(&vpfe->lock);
 
-	return ret;
+	return 0;
 }
 
 /*
diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c
index cfebf29..f4458b0 100644
--- a/drivers/media/platform/exynos4-is/fimc-capture.c
+++ b/drivers/media/platform/exynos4-is/fimc-capture.c
@@ -538,7 +538,6 @@ static int fimc_capture_release(struct file *file)
 	struct fimc_dev *fimc = video_drvdata(file);
 	struct fimc_vid_cap *vc = &fimc->vid_cap;
 	bool close = v4l2_fh_is_singular_file(file);
-	int ret;
 
 	dbg("pid: %d, state: 0x%lx", task_pid_nr(current), fimc->state);
 
@@ -549,7 +548,7 @@ static int fimc_capture_release(struct file *file)
 		vc->streaming = false;
 	}
 
-	ret = _vb2_fop_release(file, NULL);
+	_vb2_fop_release(file, NULL);
 
 	if (close) {
 		clear_bit(ST_CAPT_BUSY, &fimc->state);
@@ -564,7 +563,7 @@ static int fimc_capture_release(struct file *file)
 	pm_runtime_put_sync(&fimc->pdev->dev);
 	mutex_unlock(&fimc->lock);
 
-	return ret;
+	return 0;
 }
 
 static const struct v4l2_file_operations fimc_capture_fops = {
-- 
2.1.4

[RFC PATCH 5/7] vb2: _vb2_fop_release returns if this was the last fh.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

_vb2_fop_release now returns true if this was the last open fh.
This will simplify drivers that call it and that need to check if it
was the last close.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 7 ++++---
 include/media/videobuf2-core.h           | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index b866a6b..f348b8a 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -3452,7 +3452,7 @@ int vb2_fop_mmap(struct file *file, struct vm_area_struct *vma)
 }
 EXPORT_SYMBOL_GPL(vb2_fop_mmap);
 
-int _vb2_fop_release(struct file *file, struct mutex *lock)
+bool _vb2_fop_release(struct file *file, struct mutex *lock)
 {
 	struct video_device *vdev = video_devdata(file);
 
@@ -3464,7 +3464,7 @@ int _vb2_fop_release(struct file *file, struct mutex *lock)
 	}
 	if (lock)
 		mutex_unlock(lock);
-	return v4l2_fh_release(file);
+	return v4l2_fh_release_is_last(file);
 }
 EXPORT_SYMBOL_GPL(_vb2_fop_release);
 
@@ -3473,7 +3473,8 @@ int vb2_fop_release(struct file *file)
 	struct video_device *vdev = video_devdata(file);
 	struct mutex *lock = vdev->queue->lock ? vdev->queue->lock : vdev->lock;
 
-	return _vb2_fop_release(file, lock);
+	_vb2_fop_release(file, lock);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(vb2_fop_release);
 
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 22a44c2..7ed4c37 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -644,7 +644,7 @@ int vb2_ioctl_expbuf(struct file *file, void *priv,
 
 int vb2_fop_mmap(struct file *file, struct vm_area_struct *vma);
 int vb2_fop_release(struct file *file);
-int _vb2_fop_release(struct file *file, struct mutex *lock);
+bool _vb2_fop_release(struct file *file, struct mutex *lock);
 ssize_t vb2_fop_write(struct file *file, const char __user *buf,
 		size_t count, loff_t *ppos);
 ssize_t vb2_fop_read(struct file *file, char __user *buf,
-- 
2.1.4

[RFC PATCH 6/7] am437x/exynos4-is/marvell-ccic/sh_vou: simplify release()

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Use the _vb2_fop_release() return code to determine if this was
the last close or not.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/am437x/am437x-vpfe.c      | 13 +++----------
 drivers/media/platform/exynos4-is/fimc-capture.c |  5 +----
 drivers/media/platform/marvell-ccic/mcam-core.c  |  5 +----
 drivers/media/platform/sh_vou.c                  |  5 +----
 4 files changed, 6 insertions(+), 22 deletions(-)

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index 210c779..14b9289 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -1186,21 +1186,14 @@ static int vpfe_initialize_device(struct vpfe_device *vpfe)
 static int vpfe_release(struct file *file)
 {
 	struct vpfe_device *vpfe = video_drvdata(file);
-	bool fh_singular;
 
 	mutex_lock(&vpfe->lock);
 
-	/* Save the singular status before we call the clean-up helper */
-	fh_singular = v4l2_fh_is_singular_file(file);
-
-	/* the release helper will cleanup any on-going streaming */
-	_vb2_fop_release(file, NULL);
-
 	/*
-	 * If this was the last open file.
-	 * Then de-initialize hw module.
+	 * The release helper will cleanup any on-going streaming.
+	 * If this was the last open file, then de-initialize hw module.
 	 */
-	if (fh_singular)
+	if (_vb2_fop_release(file, NULL))
 		vpfe_ccdc_close(&vpfe->ccdc, vpfe->pdev);
 
 	mutex_unlock(&vpfe->lock);
diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c
index f4458b0..f654b1c 100644
--- a/drivers/media/platform/exynos4-is/fimc-capture.c
+++ b/drivers/media/platform/exynos4-is/fimc-capture.c
@@ -537,7 +537,6 @@ static int fimc_capture_release(struct file *file)
 {
 	struct fimc_dev *fimc = video_drvdata(file);
 	struct fimc_vid_cap *vc = &fimc->vid_cap;
-	bool close = v4l2_fh_is_singular_file(file);
 
 	dbg("pid: %d, state: 0x%lx", task_pid_nr(current), fimc->state);
 
@@ -548,9 +547,7 @@ static int fimc_capture_release(struct file *file)
 		vc->streaming = false;
 	}
 
-	_vb2_fop_release(file, NULL);
-
-	if (close) {
+	if (_vb2_fop_release(file, NULL)) {
 		clear_bit(ST_CAPT_BUSY, &fimc->state);
 		fimc_pipeline_call(&vc->ve, close);
 		clear_bit(ST_CAPT_SUSPENDED, &fimc->state);
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c
index 5e2b4df..13d4f7f 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -1631,12 +1631,9 @@ out:
 static int mcam_v4l_release(struct file *filp)
 {
 	struct mcam_camera *cam = video_drvdata(filp);
-	bool last_open;
 
 	mutex_lock(&cam->s_mutex);
-	last_open = v4l2_fh_is_singular_file(filp);
-	_vb2_fop_release(filp, NULL);
-	if (last_open) {
+	if (_vb2_fop_release(filp, NULL)) {
 		mcam_disable_mipi(cam);
 		mcam_ctlr_power_down(cam);
 		if (cam->buffer_mode == B_vmalloc && alloc_bufs_at_read)
diff --git a/drivers/media/platform/sh_vou.c b/drivers/media/platform/sh_vou.c
index fe5c8ab..da1d78c 100644
--- a/drivers/media/platform/sh_vou.c
+++ b/drivers/media/platform/sh_vou.c
@@ -1157,12 +1157,9 @@ done_open:
 static int sh_vou_release(struct file *file)
 {
 	struct sh_vou_device *vou_dev = video_drvdata(file);
-	bool is_last;
 
 	mutex_lock(&vou_dev->fop_lock);
-	is_last = v4l2_fh_is_singular_file(file);
-	_vb2_fop_release(file, NULL);
-	if (is_last) {
+	if (_vb2_fop_release(file, NULL)) {
 		/* Last close */
 		vou_dev->status = SH_VOU_INITIALISING;
 		sh_vou_reg_a_set(vou_dev, VOUER, 0, 0x101);
-- 
2.1.4

[RFC PATCH 7/7] cpia2/si470x: simplify open

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

With the new v4l2_fh_open_is_first function the check whether a file
open is the first is now much easier. Implement this in these two
drivers.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/radio/si470x/radio-si470x-i2c.c | 21 +++++++--------------
 drivers/media/usb/cpia2/cpia2_v4l.c           |  5 +----
 2 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/media/radio/si470x/radio-si470x-i2c.c b/drivers/media/radio/si470x/radio-si470x-i2c.c
index 471d6a8..aa05ffd 100644
--- a/drivers/media/radio/si470x/radio-si470x-i2c.c
+++ b/drivers/media/radio/si470x/radio-si470x-i2c.c
@@ -182,16 +182,15 @@ static int si470x_get_all_registers(struct si470x_device *radio)
 int si470x_fops_open(struct file *file)
 {
 	struct si470x_device *radio = video_drvdata(file);
-	int retval = v4l2_fh_open(file);
+	int retval;
 
-	if (retval)
-		return retval;
-
-	if (v4l2_fh_is_singular_file(file)) {
+	if (v4l2_fh_open_is_first(file, &retval)) {
 		/* start radio */
 		retval = si470x_start(radio);
-		if (retval < 0)
-			goto done;
+		if (retval < 0) {
+			v4l2_fh_release(file);
+			return retval;
+		}
 
 		/* enable RDS / STC interrupt */
 		radio->registers[SYSCONFIG1] |= SYSCONFIG1_RDSIEN;
@@ -200,10 +199,6 @@ int si470x_fops_open(struct file *file)
 		radio->registers[SYSCONFIG1] |= 0x1 << 2;
 		retval = si470x_set_register(radio, SYSCONFIG1);
 	}
-
-done:
-	if (retval)
-		v4l2_fh_release(file);
 	return retval;
 }
 
@@ -215,11 +210,9 @@ int si470x_fops_release(struct file *file)
 {
 	struct si470x_device *radio = video_drvdata(file);
 
-	if (v4l2_fh_is_singular_file(file))
+	if (v4l2_fh_release_is_last(file))
 		/* stop radio */
 		si470x_stop(radio);
-
-	return v4l2_fh_release(file);
 }
 
 
diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c b/drivers/media/usb/cpia2/cpia2_v4l.c
index 9caea83..30f89ba 100644
--- a/drivers/media/usb/cpia2/cpia2_v4l.c
+++ b/drivers/media/usb/cpia2/cpia2_v4l.c
@@ -88,11 +88,8 @@ static int cpia2_open(struct file *file)
 
 	if (mutex_lock_interruptible(&cam->v4l2_lock))
 		return -ERESTARTSYS;
-	retval = v4l2_fh_open(file);
-	if (retval)
-		goto open_unlock;
 
-	if (v4l2_fh_is_singular_file(file)) {
+	if (v4l2_fh_open_is_first(file, &retval)) {
 		if (cpia2_allocate_buffers(cam)) {
 			v4l2_fh_release(file);
 			retval = -ENOMEM;
-- 
2.1.4

Re: [RFC PATCH 1/7] v4l2-fh: change int to bool for v4l2_fh_is_singular(_file)

[Sakari Ailus]

On Fri, Jul 24, 2015 at 12:21:30PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> This function really returns a bool, so use that as the return type
> instead of int.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [RFC PATCH 2/7] v4l2-fh: v4l2_fh_add/del now return whether it was the first or last fh.

[Sakari Ailus]

On Fri, Jul 24, 2015 at 12:21:31PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Simplify driver code by letting v4l2_fh_add/del return true if the
> filehandle was the first open or the last close.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [RFC PATCH 3/7] v4l2-fh: add v4l2_fh_open_is_first and v4l2_fh_release_is_last

[Sakari Ailus]

Hi Hans,

On Fri, Jul 24, 2015 at 12:21:32PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Add new helper functions that report back if this was the first open
> or last close.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---

...

> @@ -65,11 +65,23 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev);
>   */
>  bool v4l2_fh_add(struct v4l2_fh *fh);
>  /*
> + * It allocates a v4l2_fh and inits and adds it to the video_device associated
> + * with the file pointer. In addition it returns true if this was the first
> + * open and false otherwise. The error code is returned in *err.
> + */
> +bool v4l2_fh_open_is_first(struct file *filp, int *err);

The new interface functions look a tad clumsy to me.

What would you think of returning the singularity value from v4l2_fh_open()
straight away? Negative integers are errors, so zero and positive values are
free.

A few drivers just check if the value is non-zero and then return that
value, but there are just a handful of those.

> +/*
>   * Can be used as the open() op of v4l2_file_operations.
>   * It allocates a v4l2_fh and inits and adds it to the video_device associated
>   * with the file pointer.
>   */
> -int v4l2_fh_open(struct file *filp);
> +static inline int v4l2_fh_open(struct file *filp)
> +{
> +	int err;
> +
> +	v4l2_fh_open_is_first(filp, &err);
> +	return err;
> +}
>  /*
>   * Remove file handle from the list of file handles. Must be called in
>   * v4l2_file_operations->release() handler if the driver uses v4l2_fh.
> @@ -86,12 +98,23 @@ bool v4l2_fh_del(struct v4l2_fh *fh);
>   */
>  void v4l2_fh_exit(struct v4l2_fh *fh);
>  /*
> + * It deletes and exits the v4l2_fh associated with the file pointer and
> + * frees it. It will do nothing if filp->private_data (the pointer to the
> + * v4l2_fh struct) is NULL. This function returns true if this was the
> + * last open file handle and false otherwise.
> + */
> +bool v4l2_fh_release_is_last(struct file *filp);
> +/*
>   * Can be used as the release() op of v4l2_file_operations.
>   * It deletes and exits the v4l2_fh associated with the file pointer and
>   * frees it. It will do nothing if filp->private_data (the pointer to the
>   * v4l2_fh struct) is NULL. This function always returns 0.
>   */
> -int v4l2_fh_release(struct file *filp);
> +static inline int v4l2_fh_release(struct file *filp)
> +{
> +	v4l2_fh_release_is_last(filp);
> +	return 0;
> +}
>  /*
>   * Returns true if this filehandle is the only filehandle opened for the

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [RFC PATCH 3/7] v4l2-fh: add v4l2_fh_open_is_first and v4l2_fh_release_is_last

[Hans Verkuil]

Hi Sakari,

On 07/26/2015 12:42 AM, Sakari Ailus wrote:
> Hi Hans,
> 
> On Fri, Jul 24, 2015 at 12:21:32PM +0200, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Add new helper functions that report back if this was the first open
>> or last close.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
> 
> ...
> 
>> @@ -65,11 +65,23 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev);
>>   */
>>  bool v4l2_fh_add(struct v4l2_fh *fh);
>>  /*
>> + * It allocates a v4l2_fh and inits and adds it to the video_device associated
>> + * with the file pointer. In addition it returns true if this was the first
>> + * open and false otherwise. The error code is returned in *err.
>> + */
>> +bool v4l2_fh_open_is_first(struct file *filp, int *err);
> 
> The new interface functions look a tad clumsy to me.
> 
> What would you think of returning the singularity value from v4l2_fh_open()
> straight away? Negative integers are errors, so zero and positive values are
> free.
> 
> A few drivers just check if the value is non-zero and then return that
> value, but there are just a handful of those.

I don't like messing with that. It can be done because all driver opens go through
either v4l2-dev.c or v4l2-subdev.c and we can convert a return value of >0 to 0. We
have to do that since fs/open.c doesn't check for >0, just != 0.

But that makes our 'open' implementation non-standard, and I feel that that's
both confusing and increases the chances of future bugs (precisely because it is
unexpected).

In pretty much all open() implementations of drivers you will have a 'int err;'
variable or something similar, so being able to do:

	if (v4l2_fh_open_is_first(file, &err)) {
	}

is actually quite efficient (see patch 7/7).

Regards,

	Hans