Re: [RFC PATCH 1/8] soc_camera V4L2 driver for directly-connected SoC-based cameras

[Guennadi Liakhovetski <g.liakhovetski@xxxxxxxxxxxxxx>]

On Thu, 24 Jan 2008, Trent Piepho wrote:

> On Wed, 23 Jan 2008, Guennadi Liakhovetski wrote:
> > This driver provides an interface between platform-specific camera
> > busses and camera devices. It should be used if the camera is connected
> 
> Are you aware of the existing camera interfaces, like that used between
> cafe_ccic and ov7676, or ov511/w9968cf and ovcamchip, or the OMAP stuff
> from Nokia?

Ok, thanks for the pointers. As for cafe_ccic and w9968cf, as far as I 
understand, they hard code i2c / smbus interface. Whereas my driver 
introduces a generic bus-neutral interface. In my case both cameras are 
indeed controlled over i2c, but this is not camera-internal i2c bus, but a 
system-wide general bus.

As for the OMAP driver, right, it is indeed serving a similar purpose. 
Unfortunately, I based my work on 2.6.23 and there it wasn't in the kernel 
yet. Even now it is incomplete in the mainline, the platform (OMAP) part 
and, I think, actual camera drivers are still only in a separate tree. 
But, yes, if I knew, that such a driver is planned for submission, I would 
try to base on it. I think, we could accept my driver too parallel to the 
one from Nokia. As far as I know, there are no versions of PXA270 camera 
driver for this interface. And I do not think I will get resources to 
switch to the int-device API. But I definitely will have a closer look at 
it. My preference would be to get my driver in the mainline, after 
addressing all issues that come up during the review, and then see how 
both of them evolve...

> > +static int soc_camera_open(struct inode *inode, struct file *file)
> > +{
> > +	struct soc_camera_host *ici;
> > +	struct soc_camera_device *icd;
> > +	struct video_device *vdev;
> > +	struct soc_camera_file *icf;
> > +	int ret;
> > +
> > +	mutex_lock(&video_lock);
> > +	list_for_each_entry(icd, &devices, list) {
> > +		if (icd->vdev && icd->vdev->minor == iminor(inode))
> > +			break;
> > +	}
> > +	mutex_unlock(&video_lock);
> 
> You should be able to avoid doing the inode -> soc_camera_device mapping
> this way.  The v4l2 code has a function to do inode -> video_device, and
> you chould be able to do video_device -> soc_camera_device via a private
> state pointer in video_dev or maybe container_of().
> 
> > +	for (i = 0; i < icd->ops->num_controls; i++)
> > +		if (qc->id && qc->id == icd->ops->controls[i].id) {
> 
> Wouldn't it make sense to move the check of qc->id out of the loop?
> 
> > +	switch (ctrl->id) {
> > +	case V4L2_CID_GAIN:
> > +		if (icd->gain == (unsigned short)~0)
> > +			return -EINVAL;
> > +		ctrl->value = icd->gain;
> 
> Why not have icd->ops->get_control handle these cases and avoid the special
> cases?  There isn't any special case code for gain and exposure in the
> s_ctrl function.
> 
> > +#warning "verify whether device_register will fail, or scan the list yourself for duplicate IDs"
> 
> Seems like something is unfinished.  I think it's a bad idea to try to
> register a device with the same name as an existing device.

Yes, this one as well as a couple of others have to be dealt with before 
the final submission. Will fix other points too.

Thanks a lot for the review!
Guennadi
---
Guennadi Liakhovetski

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@xxxxxxxxxx?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list