Hi Guennadi,
On Sat, Jun 19, 2010 at 04:13:31PM +0200, Guennadi Liakhovetski wrote:
> 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.
Tanks for your review. I'll send v4 shortly.
baruch
> 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/
--
~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch@xxxxxxxxxx - tel: +972.2.679.5364, http://www.tkos.co.il -
--
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
