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@xxxxxxxxxx>
> ---
> 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/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
