[PATCH 0/8] rcar-vin: Merge Gen2 and Gen3 file operations

[PATCH 0/8] rcar-vin: Merge Gen2 and Gen3 file operations

[Niklas Söderlund]

Hi,

This series aims to merge the two different set of file operations used 
in the rcar-vin driver. One set was used on Renesas Gen2 boards while 
the other set was used on Gen3. Main difference between the two is that 
Gen2 uses a device node centric world view while Gen3 subscribes to the 
media controller way of looking at things ;-)

After refactoring out a lot of code left over from the Gen2 mode which 
stems from the drivers origin in soc_camera it became apparent that a 
lot of code could me removed my merging the two sets.

Tested on both Gen2 and Gen3 no regressions found.

Niklas Söderlund (8):
  rcar-vin: Do not call pm_runtime_{resume,suspend}()
  rcar-vin: Remove unneeded calls to pm_runtime_{enable,disable}
  rcar-vin: Allow interrupting lock when trying to open the video device
  rcar-vin: Do not sync subdevice format when opening the video device
  rcar-vin: Move pm_runtime_{get,put} out of helpers
  rcar-vin: Merge helpers dealing with powering the parallel subdevice
  rcar-vin: Fold rvin_initialize_device() into rvin_open()
  rcar-vin: Merge Gen2 and Gen3 file operations

 drivers/media/platform/rcar-vin/rcar-v4l2.c | 188 ++++----------------
 1 file changed, 38 insertions(+), 150 deletions(-)

-- 
2.21.0

[PATCH 1/8] rcar-vin: Do not call pm_runtime_{resume,suspend}()

[Niklas Söderlund]

The driver do not implement runtime resume and suspend function so there
is little point in trying to call them. This is a leftover from the
drivers soc_camera beginnings.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 7cbdcbf9b090c638..b821ea01786eb1ff 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -798,9 +798,6 @@ static int rvin_initialize_device(struct file *file)
 		return ret;
 
 	pm_runtime_enable(&vin->vdev.dev);
-	ret = pm_runtime_resume(&vin->vdev.dev);
-	if (ret < 0 && ret != -ENOSYS)
-		goto eresume;
 
 	/*
 	 * Try to configure with default parameters. Notice: this is the
@@ -817,7 +814,6 @@ static int rvin_initialize_device(struct file *file)
 	return 0;
 esfmt:
 	pm_runtime_disable(&vin->vdev.dev);
-eresume:
 	rvin_power_off(vin);
 
 	return ret;
@@ -868,7 +864,6 @@ static int rvin_release(struct file *file)
 	 * Then de-initialize hw module.
 	 */
 	if (fh_singular) {
-		pm_runtime_suspend(&vin->vdev.dev);
 		pm_runtime_disable(&vin->vdev.dev);
 		rvin_power_off(vin);
 	}
-- 
2.21.0

[PATCH 2/8] rcar-vin: Remove unneeded calls to pm_runtime_{enable,disable}

[Niklas Söderlund]

Runtime PM is already enable unconditionally when the driver is probed
and disabled when it's removed. There is no point in doing it again for
Gen2 when opening and closing the video device.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index b821ea01786eb1ff..0841f1a0bfd7ba3a 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -797,8 +797,6 @@ static int rvin_initialize_device(struct file *file)
 	if (ret < 0)
 		return ret;
 
-	pm_runtime_enable(&vin->vdev.dev);
-
 	/*
 	 * Try to configure with default parameters. Notice: this is the
 	 * very first open, so, we cannot race against other calls,
@@ -813,7 +811,6 @@ static int rvin_initialize_device(struct file *file)
 
 	return 0;
 esfmt:
-	pm_runtime_disable(&vin->vdev.dev);
 	rvin_power_off(vin);
 
 	return ret;
@@ -863,10 +860,8 @@ static int rvin_release(struct file *file)
 	 * If this was the last open file.
 	 * Then de-initialize hw module.
 	 */
-	if (fh_singular) {
-		pm_runtime_disable(&vin->vdev.dev);
+	if (fh_singular)
 		rvin_power_off(vin);
-	}
 
 	mutex_unlock(&vin->lock);
 
-- 
2.21.0

[PATCH 3/8] rcar-vin: Allow interrupting lock when trying to open the video device

[Niklas Söderlund]

The user should be allowed to break waiting for the lock when opening
the video device.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 0841f1a0bfd7ba3a..f67cef97b89a3bd4 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -821,7 +821,9 @@ static int rvin_open(struct file *file)
 	struct rvin_dev *vin = video_drvdata(file);
 	int ret;
 
-	mutex_lock(&vin->lock);
+	ret = mutex_lock_interruptible(&vin->lock);
+	if (ret)
+		return ret;
 
 	file->private_data = vin;
 
-- 
2.21.0

[PATCH 4/8] rcar-vin: Do not sync subdevice format when opening the video device

[Niklas Söderlund]

The format is already synced when the subdevice is bound, there is no
need to do do it every time the video device is opened.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 25 ---------------------
 1 file changed, 25 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index f67cef97b89a3bd4..71651c5a69483367 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -782,38 +782,13 @@ static int rvin_initialize_device(struct file *file)
 	struct rvin_dev *vin = video_drvdata(file);
 	int ret;
 
-	struct v4l2_format f = {
-		.type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
-		.fmt.pix = {
-			.width		= vin->format.width,
-			.height		= vin->format.height,
-			.field		= vin->format.field,
-			.colorspace	= vin->format.colorspace,
-			.pixelformat	= vin->format.pixelformat,
-		},
-	};
-
 	ret = rvin_power_on(vin);
 	if (ret < 0)
 		return ret;
 
-	/*
-	 * Try to configure with default parameters. Notice: this is the
-	 * very first open, so, we cannot race against other calls,
-	 * apart from someone else calling open() simultaneously, but
-	 * .host_lock is protecting us against it.
-	 */
-	ret = rvin_s_fmt_vid_cap(file, NULL, &f);
-	if (ret < 0)
-		goto esfmt;
-
 	v4l2_ctrl_handler_setup(&vin->ctrl_handler);
 
 	return 0;
-esfmt:
-	rvin_power_off(vin);
-
-	return ret;
 }
 
 static int rvin_open(struct file *file)
-- 
2.21.0

[PATCH 5/8] rcar-vin: Move pm_runtime_{get,put} out of helpers

[Niklas Söderlund]

The helpers rvin_power_{on,off} deals with both VIN and the parallel
subdevice power. This makes it hard to merge the Gen2 and Gen3
open/release functions. Move the VIN power handling directly to the
open/release functions to prepare for the merge.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 34 +++++++++++++--------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 71651c5a69483367..5a9658b7d848fc86 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -754,8 +754,6 @@ static int rvin_power_on(struct rvin_dev *vin)
 	int ret;
 	struct v4l2_subdev *sd = vin_to_source(vin);
 
-	pm_runtime_get_sync(vin->v4l2_dev.dev);
-
 	ret = v4l2_subdev_call(sd, core, s_power, 1);
 	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
 		return ret;
@@ -768,9 +766,6 @@ static int rvin_power_off(struct rvin_dev *vin)
 	struct v4l2_subdev *sd = vin_to_source(vin);
 
 	ret = v4l2_subdev_call(sd, core, s_power, 0);
-
-	pm_runtime_put(vin->v4l2_dev.dev);
-
 	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
 		return ret;
 
@@ -802,20 +797,31 @@ static int rvin_open(struct file *file)
 
 	file->private_data = vin;
 
+	ret = pm_runtime_get_sync(vin->dev);
+	if (ret < 0)
+		goto err_unlock;
+
 	ret = v4l2_fh_open(file);
 	if (ret)
-		goto unlock;
+		goto err_pm;
 
-	if (!v4l2_fh_is_singular_file(file))
-		goto unlock;
-
-	if (rvin_initialize_device(file)) {
-		v4l2_fh_release(file);
-		ret = -ENODEV;
+	if (v4l2_fh_is_singular_file(file)) {
+		if (rvin_initialize_device(file)) {
+			ret = -ENODEV;
+			goto err_open;
+		}
 	}
 
-unlock:
 	mutex_unlock(&vin->lock);
+
+	return 0;
+err_open:
+	v4l2_fh_release(file);
+err_pm:
+	pm_runtime_put(vin->dev);
+err_unlock:
+	mutex_unlock(&vin->lock);
+
 	return ret;
 }
 
@@ -840,6 +846,8 @@ static int rvin_release(struct file *file)
 	if (fh_singular)
 		rvin_power_off(vin);
 
+	pm_runtime_put(vin->dev);
+
 	mutex_unlock(&vin->lock);
 
 	return ret;
-- 
2.21.0

[PATCH 6/8] rcar-vin: Merge helpers dealing with powering the parallel subdevice

[Niklas Söderlund]

The two power helpers are now only dealing with the parallel subdevice,
merge them into a single rvin_power_parallel() helper to reduce code
duplication.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 5a9658b7d848fc86..3b4624c117aaba18 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -749,23 +749,13 @@ static const struct v4l2_ioctl_ops rvin_mc_ioctl_ops = {
  * File Operations
  */
 
-static int rvin_power_on(struct rvin_dev *vin)
+static int rvin_power_parallel(struct rvin_dev *vin, int on)
 {
-	int ret;
 	struct v4l2_subdev *sd = vin_to_source(vin);
-
-	ret = v4l2_subdev_call(sd, core, s_power, 1);
-	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
-		return ret;
-	return 0;
-}
-
-static int rvin_power_off(struct rvin_dev *vin)
-{
+	int power = on ? 1 : 0;
 	int ret;
-	struct v4l2_subdev *sd = vin_to_source(vin);
 
-	ret = v4l2_subdev_call(sd, core, s_power, 0);
+	ret = v4l2_subdev_call(sd, core, s_power, power);
 	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
 		return ret;
 
@@ -777,7 +767,7 @@ static int rvin_initialize_device(struct file *file)
 	struct rvin_dev *vin = video_drvdata(file);
 	int ret;
 
-	ret = rvin_power_on(vin);
+	ret = rvin_power_parallel(vin, 1);
 	if (ret < 0)
 		return ret;
 
@@ -844,7 +834,7 @@ static int rvin_release(struct file *file)
 	 * Then de-initialize hw module.
 	 */
 	if (fh_singular)
-		rvin_power_off(vin);
+		rvin_power_parallel(vin, 0);
 
 	pm_runtime_put(vin->dev);
 
-- 
2.21.0

[PATCH 7/8] rcar-vin: Fold rvin_initialize_device() into rvin_open()

[Niklas Söderlund]

The function no longer serve a purpose as most tasks it performed have
been refactored, fold what remains it into the only caller.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 3b4624c117aaba18..58d0b59ee1247c8f 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -762,20 +762,6 @@ static int rvin_power_parallel(struct rvin_dev *vin, int on)
 	return 0;
 }
 
-static int rvin_initialize_device(struct file *file)
-{
-	struct rvin_dev *vin = video_drvdata(file);
-	int ret;
-
-	ret = rvin_power_parallel(vin, 1);
-	if (ret < 0)
-		return ret;
-
-	v4l2_ctrl_handler_setup(&vin->ctrl_handler);
-
-	return 0;
-}
-
 static int rvin_open(struct file *file)
 {
 	struct rvin_dev *vin = video_drvdata(file);
@@ -796,10 +782,11 @@ static int rvin_open(struct file *file)
 		goto err_pm;
 
 	if (v4l2_fh_is_singular_file(file)) {
-		if (rvin_initialize_device(file)) {
-			ret = -ENODEV;
+		ret = rvin_power_parallel(vin, 1);
+		if (ret < 0)
 			goto err_open;
-		}
+
+		v4l2_ctrl_handler_setup(&vin->ctrl_handler);
 	}
 
 	mutex_unlock(&vin->lock);
-- 
2.21.0

[PATCH 8/8] rcar-vin: Merge Gen2 and Gen3 file operations

[Niklas Söderlund]

After the rework of the Gen2 file operations it's now trivial to merge
the Gen2 and Gen3 versions.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 96 ++++-----------------
 1 file changed, 16 insertions(+), 80 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 58d0b59ee1247c8f..2c9cae4b7abe925c 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -781,14 +781,19 @@ static int rvin_open(struct file *file)
 	if (ret)
 		goto err_pm;
 
-	if (v4l2_fh_is_singular_file(file)) {
-		ret = rvin_power_parallel(vin, 1);
+	if (vin->info->use_mc) {
+		ret = v4l2_pipeline_pm_use(&vin->vdev.entity, 1);
 		if (ret < 0)
 			goto err_open;
+	} else {
+		if (v4l2_fh_is_singular_file(file)) {
+			ret = rvin_power_parallel(vin, 1);
+			if (ret < 0)
+				goto err_open;
 
-		v4l2_ctrl_handler_setup(&vin->ctrl_handler);
+			v4l2_ctrl_handler_setup(&vin->ctrl_handler);
+		}
 	}
-
 	mutex_unlock(&vin->lock);
 
 	return 0;
@@ -816,12 +821,12 @@ static int rvin_release(struct file *file)
 	/* the release helper will cleanup any on-going streaming */
 	ret = _vb2_fop_release(file, NULL);
 
-	/*
-	 * If this was the last open file.
-	 * Then de-initialize hw module.
-	 */
-	if (fh_singular)
-		rvin_power_parallel(vin, 0);
+	if (vin->info->use_mc) {
+		v4l2_pipeline_pm_use(&vin->vdev.entity, 0);
+	} else {
+		if (fh_singular)
+			rvin_power_parallel(vin, 0);
+	}
 
 	pm_runtime_put(vin->dev);
 
@@ -840,74 +845,6 @@ static const struct v4l2_file_operations rvin_fops = {
 	.read		= vb2_fop_read,
 };
 
-/* -----------------------------------------------------------------------------
- * Media controller file operations
- */
-
-static int rvin_mc_open(struct file *file)
-{
-	struct rvin_dev *vin = video_drvdata(file);
-	int ret;
-
-	ret = mutex_lock_interruptible(&vin->lock);
-	if (ret)
-		return ret;
-
-	ret = pm_runtime_get_sync(vin->dev);
-	if (ret < 0)
-		goto err_unlock;
-
-	ret = v4l2_pipeline_pm_use(&vin->vdev.entity, 1);
-	if (ret < 0)
-		goto err_pm;
-
-	file->private_data = vin;
-
-	ret = v4l2_fh_open(file);
-	if (ret)
-		goto err_v4l2pm;
-
-	mutex_unlock(&vin->lock);
-
-	return 0;
-err_v4l2pm:
-	v4l2_pipeline_pm_use(&vin->vdev.entity, 0);
-err_pm:
-	pm_runtime_put(vin->dev);
-err_unlock:
-	mutex_unlock(&vin->lock);
-
-	return ret;
-}
-
-static int rvin_mc_release(struct file *file)
-{
-	struct rvin_dev *vin = video_drvdata(file);
-	int ret;
-
-	mutex_lock(&vin->lock);
-
-	/* the release helper will cleanup any on-going streaming. */
-	ret = _vb2_fop_release(file, NULL);
-
-	v4l2_pipeline_pm_use(&vin->vdev.entity, 0);
-	pm_runtime_put(vin->dev);
-
-	mutex_unlock(&vin->lock);
-
-	return ret;
-}
-
-static const struct v4l2_file_operations rvin_mc_fops = {
-	.owner		= THIS_MODULE,
-	.unlocked_ioctl	= video_ioctl2,
-	.open		= rvin_mc_open,
-	.release	= rvin_mc_release,
-	.poll		= vb2_fop_poll,
-	.mmap		= vb2_fop_mmap,
-	.read		= vb2_fop_read,
-};
-
 void rvin_v4l2_unregister(struct rvin_dev *vin)
 {
 	if (!video_is_registered(&vin->vdev))
@@ -948,6 +885,7 @@ int rvin_v4l2_register(struct rvin_dev *vin)
 	snprintf(vdev->name, sizeof(vdev->name), "VIN%u output", vin->id);
 	vdev->release = video_device_release_empty;
 	vdev->lock = &vin->lock;
+	vdev->fops = &rvin_fops;
 	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
 		V4L2_CAP_READWRITE;
 
@@ -959,10 +897,8 @@ int rvin_v4l2_register(struct rvin_dev *vin)
 	vin->format.colorspace = RVIN_DEFAULT_COLORSPACE;
 
 	if (vin->info->use_mc) {
-		vdev->fops = &rvin_mc_fops;
 		vdev->ioctl_ops = &rvin_mc_ioctl_ops;
 	} else {
-		vdev->fops = &rvin_fops;
 		vdev->ioctl_ops = &rvin_ioctl_ops;
 		rvin_reset_format(vin);
 	}
-- 
2.21.0

Re: [PATCH 5/8] rcar-vin: Move pm_runtime_{get,put} out of helpers

[Sergei Shtylyov]

Hello!

On 13.04.2019 2:43, Niklas Söderlund wrote:

> The helpers rvin_power_{on,off} deals with both VIN and the parallel

    Deal?

> subdevice power. This makes it hard to merge the Gen2 and Gen3
> open/release functions. Move the VIN power handling directly to the
> open/release functions to prepare for the merge.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
[...]

MBR, Sergei

Re: [PATCH 2/8] rcar-vin: Remove unneeded calls to pm_runtime_{enable,disable}

[Sergei Shtylyov]

On 13.04.2019 2:43, Niklas Söderlund wrote:

> Runtime PM is already enable unconditionally when the driver is probed

    Enabled?

> and disabled when it's removed. There is no point in doing it again for
> Gen2 when opening and closing the video device.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
[...]

MBR, Sergei

Re: [PATCH 7/8] rcar-vin: Fold rvin_initialize_device() into rvin_open()

[Sergei Shtylyov]

On 13.04.2019 2:43, Niklas Söderlund wrote:

> The function no longer serve a purpose as most tasks it performed have
> been refactored, fold what remains it into the only caller.

    What remains of it?

> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
[...]

MBR, Sergei

Re: [PATCH 1/8] rcar-vin: Do not call pm_runtime_{resume,suspend}()

[Ulrich Hecht]

> On April 13, 2019 at 1:43 AM Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> wrote:
> 
> 
> The driver do not implement runtime resume and suspend function so there

"driver does", I presume.

> is little point in trying to call them. This is a leftover from the
> drivers soc_camera beginnings.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 7cbdcbf9b090c638..b821ea01786eb1ff 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -798,9 +798,6 @@ static int rvin_initialize_device(struct file *file)
>  		return ret;
>  
>  	pm_runtime_enable(&vin->vdev.dev);
> -	ret = pm_runtime_resume(&vin->vdev.dev);
> -	if (ret < 0 && ret != -ENOSYS)
> -		goto eresume;
>  
>  	/*
>  	 * Try to configure with default parameters. Notice: this is the
> @@ -817,7 +814,6 @@ static int rvin_initialize_device(struct file *file)
>  	return 0;
>  esfmt:
>  	pm_runtime_disable(&vin->vdev.dev);
> -eresume:
>  	rvin_power_off(vin);
>  
>  	return ret;
> @@ -868,7 +864,6 @@ static int rvin_release(struct file *file)
>  	 * Then de-initialize hw module.
>  	 */
>  	if (fh_singular) {
> -		pm_runtime_suspend(&vin->vdev.dev);
>  		pm_runtime_disable(&vin->vdev.dev);
>  		rvin_power_off(vin);
>  	}
> -- 
> 2.21.0
>

With typo fixed,
Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

Re: [PATCH 2/8] rcar-vin: Remove unneeded calls to pm_runtime_{enable,disable}

[Ulrich Hecht]

> On April 13, 2019 at 1:43 AM Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> wrote:
> 
> 
> Runtime PM is already enable unconditionally when the driver is probed
> and disabled when it's removed. There is no point in doing it again for
> Gen2 when opening and closing the video device.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index b821ea01786eb1ff..0841f1a0bfd7ba3a 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -797,8 +797,6 @@ static int rvin_initialize_device(struct file *file)
>  	if (ret < 0)
>  		return ret;
>  
> -	pm_runtime_enable(&vin->vdev.dev);
> -
>  	/*
>  	 * Try to configure with default parameters. Notice: this is the
>  	 * very first open, so, we cannot race against other calls,
> @@ -813,7 +811,6 @@ static int rvin_initialize_device(struct file *file)
>  
>  	return 0;
>  esfmt:
> -	pm_runtime_disable(&vin->vdev.dev);
>  	rvin_power_off(vin);
>  
>  	return ret;
> @@ -863,10 +860,8 @@ static int rvin_release(struct file *file)
>  	 * If this was the last open file.
>  	 * Then de-initialize hw module.
>  	 */
> -	if (fh_singular) {
> -		pm_runtime_disable(&vin->vdev.dev);
> +	if (fh_singular)
>  		rvin_power_off(vin);
> -	}
>  
>  	mutex_unlock(&vin->lock);
>  
> -- 
> 2.21.0
>

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

Re: [PATCH 3/8] rcar-vin: Allow interrupting lock when trying to open the video device

[Ulrich Hecht]

> On April 13, 2019 at 1:43 AM Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> wrote:
> 
> 
> The user should be allowed to break waiting for the lock when opening
> the video device.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 0841f1a0bfd7ba3a..f67cef97b89a3bd4 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -821,7 +821,9 @@ static int rvin_open(struct file *file)
>  	struct rvin_dev *vin = video_drvdata(file);
>  	int ret;
>  
> -	mutex_lock(&vin->lock);
> +	ret = mutex_lock_interruptible(&vin->lock);
> +	if (ret)
> +		return ret;
>  
>  	file->private_data = vin;
>  
> -- 
> 2.21.0
>

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

Re: [PATCH 4/8] rcar-vin: Do not sync subdevice format when opening the video device

[Ulrich Hecht]

> On April 13, 2019 at 1:43 AM Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> wrote:
> 
> 
> The format is already synced when the subdevice is bound, there is no
> need to do do it every time the video device is opened.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 25 ---------------------
>  1 file changed, 25 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index f67cef97b89a3bd4..71651c5a69483367 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -782,38 +782,13 @@ static int rvin_initialize_device(struct file *file)
>  	struct rvin_dev *vin = video_drvdata(file);
>  	int ret;
>  
> -	struct v4l2_format f = {
> -		.type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
> -		.fmt.pix = {
> -			.width		= vin->format.width,
> -			.height		= vin->format.height,
> -			.field		= vin->format.field,
> -			.colorspace	= vin->format.colorspace,
> -			.pixelformat	= vin->format.pixelformat,
> -		},
> -	};
> -
>  	ret = rvin_power_on(vin);
>  	if (ret < 0)
>  		return ret;
>  
> -	/*
> -	 * Try to configure with default parameters. Notice: this is the
> -	 * very first open, so, we cannot race against other calls,
> -	 * apart from someone else calling open() simultaneously, but
> -	 * .host_lock is protecting us against it.
> -	 */
> -	ret = rvin_s_fmt_vid_cap(file, NULL, &f);
> -	if (ret < 0)
> -		goto esfmt;
> -
>  	v4l2_ctrl_handler_setup(&vin->ctrl_handler);
>  
>  	return 0;
> -esfmt:
> -	rvin_power_off(vin);
> -
> -	return ret;
>  }
>  
>  static int rvin_open(struct file *file)
> -- 
> 2.21.0
>

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

Re: [PATCH 5/8] rcar-vin: Move pm_runtime_{get,put} out of helpers

[Ulrich Hecht]

> On April 13, 2019 at 1:43 AM Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> wrote:
> 
> 
> The helpers rvin_power_{on,off} deals with both VIN and the parallel
> subdevice power. This makes it hard to merge the Gen2 and Gen3
> open/release functions. Move the VIN power handling directly to the
> open/release functions to prepare for the merge.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 34 +++++++++++++--------
>  1 file changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 71651c5a69483367..5a9658b7d848fc86 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -754,8 +754,6 @@ static int rvin_power_on(struct rvin_dev *vin)
>  	int ret;
>  	struct v4l2_subdev *sd = vin_to_source(vin);
>  
> -	pm_runtime_get_sync(vin->v4l2_dev.dev);
> -
>  	ret = v4l2_subdev_call(sd, core, s_power, 1);
>  	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
>  		return ret;
> @@ -768,9 +766,6 @@ static int rvin_power_off(struct rvin_dev *vin)
>  	struct v4l2_subdev *sd = vin_to_source(vin);
>  
>  	ret = v4l2_subdev_call(sd, core, s_power, 0);
> -
> -	pm_runtime_put(vin->v4l2_dev.dev);
> -
>  	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
>  		return ret;
>  
> @@ -802,20 +797,31 @@ static int rvin_open(struct file *file)
>  
>  	file->private_data = vin;
>  
> +	ret = pm_runtime_get_sync(vin->dev);
> +	if (ret < 0)
> +		goto err_unlock;
> +
>  	ret = v4l2_fh_open(file);
>  	if (ret)
> -		goto unlock;
> +		goto err_pm;
>  
> -	if (!v4l2_fh_is_singular_file(file))
> -		goto unlock;
> -
> -	if (rvin_initialize_device(file)) {
> -		v4l2_fh_release(file);
> -		ret = -ENODEV;
> +	if (v4l2_fh_is_singular_file(file)) {
> +		if (rvin_initialize_device(file)) {
> +			ret = -ENODEV;
> +			goto err_open;
> +		}
>  	}
>  
> -unlock:
>  	mutex_unlock(&vin->lock);
> +
> +	return 0;
> +err_open:
> +	v4l2_fh_release(file);
> +err_pm:
> +	pm_runtime_put(vin->dev);
> +err_unlock:
> +	mutex_unlock(&vin->lock);
> +
>  	return ret;
>  }
>  
> @@ -840,6 +846,8 @@ static int rvin_release(struct file *file)
>  	if (fh_singular)
>  		rvin_power_off(vin);
>  
> +	pm_runtime_put(vin->dev);
> +
>  	mutex_unlock(&vin->lock);
>  
>  	return ret;
> -- 
> 2.21.0
>

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

Re: [PATCH 6/8] rcar-vin: Merge helpers dealing with powering the parallel subdevice

[Ulrich Hecht]

> On April 13, 2019 at 1:43 AM Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> wrote:
> 
> 
> The two power helpers are now only dealing with the parallel subdevice,
> merge them into a single rvin_power_parallel() helper to reduce code
> duplication.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 5a9658b7d848fc86..3b4624c117aaba18 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -749,23 +749,13 @@ static const struct v4l2_ioctl_ops rvin_mc_ioctl_ops = {
>   * File Operations
>   */
>  
> -static int rvin_power_on(struct rvin_dev *vin)
> +static int rvin_power_parallel(struct rvin_dev *vin, int on)

"int on" -> "bool on".

>  {
> -	int ret;
>  	struct v4l2_subdev *sd = vin_to_source(vin);
> -
> -	ret = v4l2_subdev_call(sd, core, s_power, 1);
> -	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> -		return ret;
> -	return 0;
> -}
> -
> -static int rvin_power_off(struct rvin_dev *vin)
> -{
> +	int power = on ? 1 : 0;
>  	int ret;
> -	struct v4l2_subdev *sd = vin_to_source(vin);
>  
> -	ret = v4l2_subdev_call(sd, core, s_power, 0);
> +	ret = v4l2_subdev_call(sd, core, s_power, power);
>  	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
>  		return ret;
>  
> @@ -777,7 +767,7 @@ static int rvin_initialize_device(struct file *file)
>  	struct rvin_dev *vin = video_drvdata(file);
>  	int ret;
>  
> -	ret = rvin_power_on(vin);
> +	ret = rvin_power_parallel(vin, 1);

"1" -> "true".

>  	if (ret < 0)
>  		return ret;
>  
> @@ -844,7 +834,7 @@ static int rvin_release(struct file *file)
>  	 * Then de-initialize hw module.
>  	 */
>  	if (fh_singular)
> -		rvin_power_off(vin);
> +		rvin_power_parallel(vin, 0);

"0" -> "false".

>  
>  	pm_runtime_put(vin->dev);
>  
> -- 
> 2.21.0
>

CU
Uli

Re: [PATCH 7/8] rcar-vin: Fold rvin_initialize_device() into rvin_open()

[Ulrich Hecht]

> On April 13, 2019 at 1:43 AM Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> wrote:
> 
> 
> The function no longer serve a purpose as most tasks it performed have
> been refactored, fold what remains it into the only caller.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 21 ++++-----------------
>  1 file changed, 4 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 3b4624c117aaba18..58d0b59ee1247c8f 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -762,20 +762,6 @@ static int rvin_power_parallel(struct rvin_dev *vin, int on)
>  	return 0;
>  }
>  
> -static int rvin_initialize_device(struct file *file)
> -{
> -	struct rvin_dev *vin = video_drvdata(file);
> -	int ret;
> -
> -	ret = rvin_power_parallel(vin, 1);
> -	if (ret < 0)
> -		return ret;
> -
> -	v4l2_ctrl_handler_setup(&vin->ctrl_handler);
> -
> -	return 0;
> -}
> -
>  static int rvin_open(struct file *file)
>  {
>  	struct rvin_dev *vin = video_drvdata(file);
> @@ -796,10 +782,11 @@ static int rvin_open(struct file *file)
>  		goto err_pm;
>  
>  	if (v4l2_fh_is_singular_file(file)) {
> -		if (rvin_initialize_device(file)) {
> -			ret = -ENODEV;
> +		ret = rvin_power_parallel(vin, 1);

"1" -> "true". (See patch 6/8.)

> +		if (ret < 0)
>  			goto err_open;
> -		}
> +
> +		v4l2_ctrl_handler_setup(&vin->ctrl_handler);
>  	}
>  
>  	mutex_unlock(&vin->lock);
> -- 
> 2.21.0
>

With the above fixed,
Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

Re: [PATCH 8/8] rcar-vin: Merge Gen2 and Gen3 file operations

[Ulrich Hecht]

> On April 13, 2019 at 1:43 AM Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> wrote:
> 
> 
> After the rework of the Gen2 file operations it's now trivial to merge
> the Gen2 and Gen3 versions.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 96 ++++-----------------
>  1 file changed, 16 insertions(+), 80 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 58d0b59ee1247c8f..2c9cae4b7abe925c 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -781,14 +781,19 @@ static int rvin_open(struct file *file)
>  	if (ret)
>  		goto err_pm;
>  
> -	if (v4l2_fh_is_singular_file(file)) {
> -		ret = rvin_power_parallel(vin, 1);
> +	if (vin->info->use_mc) {
> +		ret = v4l2_pipeline_pm_use(&vin->vdev.entity, 1);
>  		if (ret < 0)
>  			goto err_open;
> +	} else {
> +		if (v4l2_fh_is_singular_file(file)) {
> +			ret = rvin_power_parallel(vin, 1);

"1" -> "true". (See patch 6/8).

> +			if (ret < 0)
> +				goto err_open;
>  
> -		v4l2_ctrl_handler_setup(&vin->ctrl_handler);
> +			v4l2_ctrl_handler_setup(&vin->ctrl_handler);
> +		}
>  	}
> -
>  	mutex_unlock(&vin->lock);
>  
>  	return 0;
> @@ -816,12 +821,12 @@ static int rvin_release(struct file *file)
>  	/* the release helper will cleanup any on-going streaming */
>  	ret = _vb2_fop_release(file, NULL);
>  
> -	/*
> -	 * If this was the last open file.
> -	 * Then de-initialize hw module.
> -	 */
> -	if (fh_singular)
> -		rvin_power_parallel(vin, 0);
> +	if (vin->info->use_mc) {
> +		v4l2_pipeline_pm_use(&vin->vdev.entity, 0);
> +	} else {
> +		if (fh_singular)
> +			rvin_power_parallel(vin, 0);

"0" -> "false".

> +	}
>  
>  	pm_runtime_put(vin->dev);
>  
> @@ -840,74 +845,6 @@ static const struct v4l2_file_operations rvin_fops = {
>  	.read		= vb2_fop_read,
>  };
>  
> -/* -----------------------------------------------------------------------------
> - * Media controller file operations
> - */
> -
> -static int rvin_mc_open(struct file *file)
> -{
> -	struct rvin_dev *vin = video_drvdata(file);
> -	int ret;
> -
> -	ret = mutex_lock_interruptible(&vin->lock);
> -	if (ret)
> -		return ret;
> -
> -	ret = pm_runtime_get_sync(vin->dev);
> -	if (ret < 0)
> -		goto err_unlock;
> -
> -	ret = v4l2_pipeline_pm_use(&vin->vdev.entity, 1);
> -	if (ret < 0)
> -		goto err_pm;
> -
> -	file->private_data = vin;
> -
> -	ret = v4l2_fh_open(file);
> -	if (ret)
> -		goto err_v4l2pm;
> -
> -	mutex_unlock(&vin->lock);
> -
> -	return 0;
> -err_v4l2pm:
> -	v4l2_pipeline_pm_use(&vin->vdev.entity, 0);
> -err_pm:
> -	pm_runtime_put(vin->dev);
> -err_unlock:
> -	mutex_unlock(&vin->lock);
> -
> -	return ret;
> -}
> -
> -static int rvin_mc_release(struct file *file)
> -{
> -	struct rvin_dev *vin = video_drvdata(file);
> -	int ret;
> -
> -	mutex_lock(&vin->lock);
> -
> -	/* the release helper will cleanup any on-going streaming. */
> -	ret = _vb2_fop_release(file, NULL);
> -
> -	v4l2_pipeline_pm_use(&vin->vdev.entity, 0);
> -	pm_runtime_put(vin->dev);
> -
> -	mutex_unlock(&vin->lock);
> -
> -	return ret;
> -}
> -
> -static const struct v4l2_file_operations rvin_mc_fops = {
> -	.owner		= THIS_MODULE,
> -	.unlocked_ioctl	= video_ioctl2,
> -	.open		= rvin_mc_open,
> -	.release	= rvin_mc_release,
> -	.poll		= vb2_fop_poll,
> -	.mmap		= vb2_fop_mmap,
> -	.read		= vb2_fop_read,
> -};
> -
>  void rvin_v4l2_unregister(struct rvin_dev *vin)
>  {
>  	if (!video_is_registered(&vin->vdev))
> @@ -948,6 +885,7 @@ int rvin_v4l2_register(struct rvin_dev *vin)
>  	snprintf(vdev->name, sizeof(vdev->name), "VIN%u output", vin->id);
>  	vdev->release = video_device_release_empty;
>  	vdev->lock = &vin->lock;
> +	vdev->fops = &rvin_fops;
>  	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
>  		V4L2_CAP_READWRITE;
>  
> @@ -959,10 +897,8 @@ int rvin_v4l2_register(struct rvin_dev *vin)
>  	vin->format.colorspace = RVIN_DEFAULT_COLORSPACE;
>  
>  	if (vin->info->use_mc) {
> -		vdev->fops = &rvin_mc_fops;
>  		vdev->ioctl_ops = &rvin_mc_ioctl_ops;
>  	} else {
> -		vdev->fops = &rvin_fops;
>  		vdev->ioctl_ops = &rvin_ioctl_ops;
>  		rvin_reset_format(vin);
>  	}
> -- 
> 2.21.0
>

With those fixed,
Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli