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@xxxxxxxxxxxx>
> Reviewed-by: Ulrich Hecht <uli+renesas@xxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxx>
> + 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;
--
Regards,
Laurent Pinchart
