Messages in this thread | | | Date | Tue, 09 Dec 2014 12:13:20 +0100 | From | Hans Verkuil <> | Subject | Re: [PATCH v5] media: platform: add VPFE capture driver support for AM437X |
| |
On 12/08/14 23:21, Lad, Prabhakar wrote: > From: Benoit Parrot <bparrot@ti.com> > > This patch adds Video Processing Front End (VPFE) driver for > AM437X family of devices > Driver supports the following: > - V4L2 API using MMAP buffer access based on videobuf2 api > - Asynchronous sensor/decoder sub device registration > - DT support > > Signed-off-by: Benoit Parrot <bparrot@ti.com> > Signed-off-by: Darren Etheridge <detheridge@ti.com> > Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com> > --- > Changes for v5: > 1: Fixed review comments pointed out by Hans, fixing race condition. > > v4l2-compliance output:
Thanks!
<snip>
> ---------------------- > > diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c > new file mode 100644 > index 0000000..c2b29a2 > --- /dev/null > +++ b/drivers/media/platform/am437x/am437x-vpfe.c > +/* > + * vpfe_release : This function is based on the vb2_fop_release > + * helper function. > + * It has been augmented to handle module power management, > + * by disabling/enabling h/w module fcntl clock when necessary. > + */ > +static int vpfe_release(struct file *file) > +{ > + struct vpfe_device *vpfe = video_drvdata(file); > + bool close = v4l2_fh_is_singular_file(file); > + int ret; > + > + vpfe_dbg(2, vpfe, "vpfe_release\n"); > + > + mutex_lock(&vpfe->lock);
The v4l2_fh_is_singular_file() call should be inside the lock as well. So:
close = v4l2_fh_is_singular_file(file);
> + > + ret = _vb2_fop_release(file, NULL); > + if (close) > + vpfe_ccdc_close(&vpfe->ccdc, vpfe->pdev); > + > + mutex_unlock(&vpfe->lock); > + > + return ret; > +} > + > +/* > + * vpfe_open : This function is based on the v4l2_fh_open helper function. > + * It has been augmented to handle module power management, > + * by disabling/enabling h/w module fcntl clock when necessary. > + */ > +static int vpfe_open(struct file *file) > +{ > + struct vpfe_device *vpfe = video_drvdata(file); > + int ret; > + > + ret = v4l2_fh_open(file);
Same here, v4l2_fh_open should be inside the lock.
> + if (ret) { > + vpfe_err(vpfe, "v4l2_fh_open failed\n"); > + return ret; > + } > + > + mutex_lock(&vpfe->lock);
So:
ret = v4l2_fh_open(file); if (ret) { vpfe_err(vpfe, "v4l2_fh_open failed\n"); goto unlock; }
> + > + if (!v4l2_fh_is_singular_file(file)) > + goto unlock; > + > + if (vpfe_initialize_device(vpfe)) { > + v4l2_fh_release(file); > + ret = -ENODEV; > + } > + > +unlock: > + mutex_unlock(&vpfe->lock); > + return ret; > +}
Regards,
Hans
| |