[PATCH v3 1/3] mx2_camera: Add soc_camera support for i.MX25/i.MX27

[Guennadi Liakhovetski]

Hi Baruch

On Wed, 26 May 2010, Baruch Siach wrote:

> This is the soc_camera support developed by Sascha Hauer for the i.MX27.  Alan
> Carvalho de Assis modified the original driver to get it working on more recent
> kernels. I modified it further to add support for i.MX25. This driver has been
> tested on i.MX25 and i.MX27 based platforms.

I hoped, this would be the final version, but if I'm not mistaken, you've 
introduced an error, which we better fix before committing. And as we 
anyway will likely need a v4, I'll also ask you to improve a couple of 
stylistic issues, which otherwise I'd just fix myself with your 
permission.

> Signed-off-by: Baruch Siach <baruch at tkos.co.il>
> ---
>  arch/arm/plat-mxc/include/mach/memory.h  |    4 +-
>  arch/arm/plat-mxc/include/mach/mx2_cam.h |   46 +
>  drivers/media/video/Kconfig              |   13 +
>  drivers/media/video/Makefile             |    1 +
>  drivers/media/video/mx2_camera.c         | 1488 ++++++++++++++++++++++++++++++
>  5 files changed, 1550 insertions(+), 2 deletions(-)
>  create mode 100644 arch/arm/plat-mxc/include/mach/mx2_cam.h
>  create mode 100644 drivers/media/video/mx2_camera.c
> 
> diff --git a/arch/arm/plat-mxc/include/mach/memory.h b/arch/arm/plat-mxc/include/mach/memory.h
> index c4b40c3..5803836 100644
> --- a/arch/arm/plat-mxc/include/mach/memory.h
> +++ b/arch/arm/plat-mxc/include/mach/memory.h

[snip]

> +static void mx2_videobuf_queue(struct videobuf_queue *vq,
> +			       struct videobuf_buffer *vb)
> +{
> +	struct soc_camera_device *icd = vq->priv_data;
> +	struct soc_camera_host *ici =
> +		to_soc_camera_host(icd->dev.parent);
> +	struct mx2_camera_dev *pcdev = ici->priv;
> +	struct mx2_buffer *buf = container_of(vb, struct mx2_buffer, vb);
> +	unsigned long flags;
> +	int ret;
> +
> +	dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__,
> +		vb, vb->baddr, vb->bsize);
> +
> +	spin_lock_irqsave(&pcdev->lock, flags);
> +
> +	vb->state = VIDEOBUF_QUEUED;
> +	list_add_tail(&vb->queue, &pcdev->capture);
> +
> +	if (mx27_camera_emma(pcdev))
> +		goto out;
> +	else if (cpu_is_mx27()) {

One of the minor ones - please, add braces in the "if" case.

[snip]

> +static irqreturn_t mx27_camera_emma_irq(int irq_emma, void *data)
> +{
> +	struct mx2_camera_dev *pcdev = data;
> +	unsigned int status = readl(pcdev->base_emma + PRP_INTRSTATUS);
> +	struct mx2_buffer *buf;
> +
> +	if ((status & (3 << 5)) == (3 << 5)
> +			&& !list_empty(&pcdev->active_bufs)) {
> +		/*
> +		 * Both buffers have triggered, process the one we're expecting
> +		 * to first
> +		 */
> +		buf = list_entry(pcdev->active_bufs.next,
> +			struct mx2_buffer, vb.queue);
> +		mx27_camera_frame_done_emma(pcdev, buf->bufnum, VIDEOBUF_DONE);
> +	}
> +	if (status & (1 << 6))
> +		mx27_camera_frame_done_emma(pcdev, 0, VIDEOBUF_DONE);
> +	if (status & (1 << 5))
> +		mx27_camera_frame_done_emma(pcdev, 1, VIDEOBUF_DONE);

Now, this is the important one. In my review of v2 I proposed the above 
fix for the both-bits-set case. But, I think, your implementation is not 
correct. Don't you have to clear the expected buffer number, so that you 
don't process it twice? Something like

		status &= ~(1 << 6 - buf->bufnum);

anywhere inside the first of the three ifs?

> +	if (status & (1 << 7)) {

Bit 7 is overflow. A correct handling could be resetting the buffer, 
returning an error frame and continuing with the next one. However, I 
understand, that you do not have a chance to implement this properly 
now. So, please, at least add a "FIXME" comment, explaining, what should 
be done here. Besides, error states are normally checked before normal 
data processing. So, either mention this in the comment too, or move this 
above the buffer completion processing.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/