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

[Trent Piepho <xyzzy@xxxxxxxxxxxxx>]

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?

> +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.

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