Alexey Klimov wrote:
+static int vidioc_g_fmt_vid_cap(struct file *file, void *fh,
+ struct v4l2_format *f)
+{
+ struct omap34xxcam_fh *ofh = fh;
+ struct omap34xxcam_videodev *vdev = ofh->vdev;
+
+ if (vdev->vdev_sensor == v4l2_int_device_dummy())
+ return -EINVAL;
+
+ mutex_lock(&vdev->mutex);
+ f->fmt.pix = vdev->pix;
+ mutex_unlock(&vdev->mutex);
H, you are using mutex_lock to lock reading from vdev structure..
Well, i don't if this is right approach. I am used to that mutex_lock is
used to prevent _changing_ of members in structure..
The vdev->mutex is acquired since we want to prevent concurrent access
to vdev->pix. Otherwise it might change while we are reading it, right?
+static int vidioc_s_fmt_vid_cap(struct file *file, void *fh,
+ struct v4l2_format *f)
+{
+ struct omap34xxcam_fh *ofh = fh;
+ struct omap34xxcam_videodev *vdev = ofh->vdev;
+ struct v4l2_pix_format pix_tmp;
+ struct v4l2_fract timeperframe;
+ int rval;
+
+ if (vdev->vdev_sensor == v4l2_int_device_dummy())
+ return -EINVAL;
+
+ mutex_lock(&vdev->mutex);
+ if (vdev->streaming) {
+ rval = -EBUSY;
+ goto out;
+ }
Well, why don't remove goto, place return -EBUSY, and move mutex after
if (vdev->streaming) check ?
The streaming state may change in the meantime. See vidioc_streamon.
It's not very likely but possible as far as I understand.
+static int vidioc_reqbufs(struct file *file, void *fh,
+ struct v4l2_requestbuffers *b)
+{
+ struct omap34xxcam_fh *ofh = fh;
+ struct omap34xxcam_videodev *vdev = ofh->vdev;
+ int rval;
+
+ if (vdev->vdev_sensor == v4l2_int_device_dummy())
+ return -EINVAL;
+
+ mutex_lock(&vdev->mutex);
+ if (vdev->streaming) {
+ mutex_unlock(&vdev->mutex);
+ return -EBUSY;
+ }
If i'm doing this i prefer to place mutex_lock after this
if(vdev->streaming) check.
Same here.
+static int omap34xxcam_device_register(struct v4l2_int_device *s)
+{
+ struct omap34xxcam_videodev *vdev = s->u.slave->master->priv;
+ struct omap34xxcam_hw_config hwc;
+ int rval;
+
+ /* We need to check rval just once. The place is here. */
I didn't understand this comment. You doing nothin in next few lines
with int variable rval(which introduced in this function). Is comment
talking about struct v4l2_int_device *s ?
Yes. If the g_priv() succeeds now it will succeed in future, too. This
comes from the platform data through the slave device.
+ /* Are we the first slave? */
+ if (vdev->slaves == 1) {
+ /* initialize the video_device struct */
+ vdev->vfd = video_device_alloc();
+ if (!vdev->vfd) {
+ dev_err(&vdev->vfd->dev,
+ "could not allocate video device struct\n");
Do i understand you code in right way ?
You call video_device_alloc() to get vdev->vfd. Then if vdev->vfd is
null(empty) you make message dev_err which based on vdev->vfd->dev but
dev->vfd allocating is failed.. If i'm not wrong you message will
provide kernel oops.
One more point here is that you use dev_err(&vdev->vfd->dev before call
to video_device_alloc() in this function.
Indeed. Others hit this already. Thanks.
+static int __init omap34xxcam_init(void)
+{
+ struct omap34xxcam_device *cam;
+ int i;
+
+ cam = kzalloc(sizeof(*cam), GFP_KERNEL);
+ if (!cam) {
+ printk(KERN_ERR "%s: could not allocate memory\n", __func__);
+ goto err;
If kzalloc failed you return -ENODEV; but this is ENOMEM error.
Yes. Will fix.
Thanks again for the comments.
--
Sakari Ailus
sakari.ai...@maxwell.research.nokia.com
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
