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

login
register
mail settings
Submitter Niklas Söderlund
Date May 16, 2019, 1:14 a.m.
Message ID <20190516011417.10590-6-niklas.soderlund+renesas@ragnatech.se>
Download mbox | patch
Permalink /patch/56174/
State New
Delegated to: Hans Verkuil
Headers show

Comments

Niklas Söderlund - May 16, 2019, 1:14 a.m.
The helpers rvin_power_{on,off} deal 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>
Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 34 +++++++++++++--------
 1 file changed, 21 insertions(+), 13 deletions(-)
Laurent Pinchart - May 16, 2019, 11:39 a.m.
Hi Niklas,

Thank you for the patch.

On Thu, May 16, 2019 at 03:14:14AM +0200, Niklas Söderlund wrote:
> The helpers rvin_power_{on,off} deal 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>
> Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
> ---
>  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);

I like operating on vin->dev much more than on vin->v4l2_dev.dev, even
if the two point to the same platform device.

> +	if (ret < 0)
> +		goto err_unlock;

Would it be useful to do this before locking the mutex, as PM runtime
doesn't need to be protected by that lock, to minimise the time spent
with the mutex held ?

> +
>  	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;

Should you propagate the return value of rvin_initialize_device ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +			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);
> +

This one could also be moved after mutex_unlock().

>  	mutex_unlock(&vin->lock);
>  
>  	return ret;

Patch

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;

Privacy Policy