On Wed, 26 May 2010, Baruch Siach wrote:
> Hi Guennadi,
>
> Thanks for your comments. A question below.
>
> On Tue, May 25, 2010 at 05:34:52PM +0200, Guennadi Liakhovetski wrote:
> > On Mon, 24 May 2010, Baruch Siach wrote:
>
> [snip]
>
> > > +static void mx2_camera_deactivate(struct mx2_camera_dev *pcdev)
> > > +{
> > > + unsigned long flags;
> > > +
> > > + clk_disable(pcdev->clk_csi);
> > > + writel(0, pcdev->base_csi + CSICR1);
> > > + if (mx27_camera_emma(pcdev))
> > > + writel(0, pcdev->base_emma + PRP_CNTL);
> > > + else if (cpu_is_mx25()) {
> >
> > Missing braces in the "if" case.
>
> Which braces are missing?
Should be
+ if (mx27_camera_emma(pcdev)) {
+ writel(0, pcdev->base_emma + PRP_CNTL);
+ } else if (cpu_is_mx25()) {
... See CodingStyle for details.
> > > + spin_lock_irqsave(&pcdev->lock, flags);
> > > + pcdev->fb1_active = NULL;
> > > + pcdev->fb2_active = NULL;
> > > + writel(0, pcdev->base_csi + CSIDMASA_FB1);
> > > + writel(0, pcdev->base_csi + CSIDMASA_FB2);
> > > + spin_unlock_irqrestore(&pcdev->lock, flags);
> > > + }
> > > +}
>
> [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()) {
> > > + if (pcdev->active != NULL) {
> >
> > In v1 you had
> >
> > > + if (!pcdev->active) {
> >
> > i.e., opposite logic. v2 seems to be wrong.
>
> Right. I'll fix this.
>
> > > + ret = imx_dma_setup_single(pcdev->dma,
> > > + videobuf_to_dma_contig(vb), vb->size,
> > > + (u32)pcdev->base_dma + 0x10,
> > > + DMA_MODE_READ);
> > > + if (ret) {
> > > + vb->state = VIDEOBUF_ERROR;
> > > + wake_up(&vb->done);
> > > + goto out;
> > > + }
> > > +
> > > + vb->state = VIDEOBUF_ACTIVE;
> >
> > This is different from v1 of your patch. I meant not below this if, but 3
> > lines down:
> >
> > > + pcdev->active = buf;
> > > + }
> >
> > ...here. Otherwise, if you don't enter the "active == NULL" if, your state
> > remains at "QUEUED." OTOH, maybe this is deliberate and you want to only
> > set the buffer to "ACTIVE" if it's really becomming active?
>
> Yes, this is intentional.
>
> > > + } else { /* cpu_is_mx25() */
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
