Re: [PATCH 9/9] omap34xxcam: Add camera driver

[Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>]

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);
> 
> Hmmmm, 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.ailus@maxwell.research.nokia.com