| Message ID | 20190704015817.17083-5-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rcar-vin: Add support for RGB formats with alpha | expand |
Hi Niklas, Thank you for the patch. On Thu, Jul 04, 2019 at 03:58:17AM +0200, Niklas Söderlund wrote: > Now that both Gen2 (device centric) and Gen3 (media device centric) > modes of this driver have controls it make sens to call s/sens/sense/ > v4l2_ctrl_handler_setup() unconditionally when opening the video device. Not only does it make sense, but it's required by 3/4. I think you should explain why in the commit message. Apart from that, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/media/platform/rcar-vin/rcar-v4l2.c | 30 ++++++++++----------- > 1 file changed, 15 insertions(+), 15 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c > index f8b6ec4408b2f5fa..cbf5d8cd6db32d77 100644 > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c > @@ -789,26 +789,26 @@ static int rvin_open(struct file *file) > if (ret) > goto err_unlock; > > - if (vin->info->use_mc) { > + 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; > + else if (v4l2_fh_is_singular_file(file)) > + ret = rvin_power_parallel(vin, true); > + > + if (ret < 0) > + goto err_open; > + > + ret = v4l2_ctrl_handler_setup(&vin->ctrl_handler); > + if (ret) > + goto err_power; > > - ret = v4l2_ctrl_handler_setup(&vin->ctrl_handler); > - if (ret) > - goto err_parallel; > - } > - } > mutex_unlock(&vin->lock); > > return 0; > -err_parallel: > - rvin_power_parallel(vin, false); > +err_power: > + if (vin->info->use_mc) > + v4l2_pipeline_pm_use(&vin->vdev.entity, 0); > + else if (v4l2_fh_is_singular_file(file)) > + rvin_power_parallel(vin, false); > err_open: > v4l2_fh_release(file); > err_unlock:
Hi Niklas, On 04/07/2019 02:58, Niklas Söderlund wrote: > Now that both Gen2 (device centric) and Gen3 (media device centric) s/Gen2 (device centric)/Gen2 (video device centric)/ ? (only if you feel it helps clarify the distinction). > modes of this driver have controls it make sens to call s/sens/sense/ > v4l2_ctrl_handler_setup() unconditionally when opening the video device. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/media/platform/rcar-vin/rcar-v4l2.c | 30 ++++++++++----------- > 1 file changed, 15 insertions(+), 15 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c > index f8b6ec4408b2f5fa..cbf5d8cd6db32d77 100644 > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c > @@ -789,26 +789,26 @@ static int rvin_open(struct file *file) > if (ret) > goto err_unlock; > > - if (vin->info->use_mc) { > + 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; > + else if (v4l2_fh_is_singular_file(file)) > + ret = rvin_power_parallel(vin, true); > + > + if (ret < 0) > + goto err_open; > + > + ret = v4l2_ctrl_handler_setup(&vin->ctrl_handler); > + if (ret) > + goto err_power; > > - ret = v4l2_ctrl_handler_setup(&vin->ctrl_handler); > - if (ret) > - goto err_parallel; > - } > - } > mutex_unlock(&vin->lock); > > return 0; It was already here before this patch, but a new line would be nice here to separate the error paths... Otherwise, Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > -err_parallel: > - rvin_power_parallel(vin, false); > +err_power: > + if (vin->info->use_mc) > + v4l2_pipeline_pm_use(&vin->vdev.entity, 0); > + else if (v4l2_fh_is_singular_file(file)) > + rvin_power_parallel(vin, false); > err_open: > v4l2_fh_release(file); > err_unlock: >
diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c index f8b6ec4408b2f5fa..cbf5d8cd6db32d77 100644 --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c @@ -789,26 +789,26 @@ static int rvin_open(struct file *file) if (ret) goto err_unlock; - if (vin->info->use_mc) { + 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; + else if (v4l2_fh_is_singular_file(file)) + ret = rvin_power_parallel(vin, true); + + if (ret < 0) + goto err_open; + + ret = v4l2_ctrl_handler_setup(&vin->ctrl_handler); + if (ret) + goto err_power; - ret = v4l2_ctrl_handler_setup(&vin->ctrl_handler); - if (ret) - goto err_parallel; - } - } mutex_unlock(&vin->lock); return 0; -err_parallel: - rvin_power_parallel(vin, false); +err_power: + if (vin->info->use_mc) + v4l2_pipeline_pm_use(&vin->vdev.entity, 0); + else if (v4l2_fh_is_singular_file(file)) + rvin_power_parallel(vin, false); err_open: v4l2_fh_release(file); err_unlock:
Now that both Gen2 (device centric) and Gen3 (media device centric) modes of this driver have controls it make sens to call v4l2_ctrl_handler_setup() unconditionally when opening the video device. Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- drivers/media/platform/rcar-vin/rcar-v4l2.c | 30 ++++++++++----------- 1 file changed, 15 insertions(+), 15 deletions(-)