Hi Niklas,
Thank you for the patch.
On Thu, May 16, 2019 at 03:14:17AM +0200, Niklas Söderlund 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@xxxxxxxxxxxx>
> Reviewed-by: Ulrich Hecht <uli+renesas@xxxxxxxx>
> ---
> 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 169639416121f204..8e4afa4278fe9d30 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, true);
> + 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, true);
> + if (ret < 0)
> + goto err_open;
>
> - v4l2_ctrl_handler_setup(&vin->ctrl_handler);
> + v4l2_ctrl_handler_setup(&vin->ctrl_handler);
> + }
> }
I wonder if you shouldn't abstract the first open init and last close
cleanup operations in separate functions, with MC and non-MC variants,
as you will need to handle the v4l2_ctrl_handler_setup() failure here,
making error handling more complex.
> -
> 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, false);
> + if (vin->info->use_mc) {
> + v4l2_pipeline_pm_use(&vin->vdev.entity, 0);
> + } else {
> + if (fh_singular)
> + rvin_power_parallel(vin, 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);
> }
--
Regards,
Laurent Pinchart
