Saturday 02 October 2010 08:07:28 Guennadi Liakhovetski napisaÅ(a): > Same with this one - let's take it as is and address a couple of clean-ups > later. Guennadi, Thanks for taking them both. BTW, what are your intentions about the last patch from my series still left not commented, "SoC Camera: add support for g_parm / s_parm operations", http://www.spinics.net/lists/linux-media/msg22887.html ? > On Thu, 30 Sep 2010, Janusz Krzysztofik wrote: > > +static void omap1_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 omap1_cam_dev *pcdev = ici->priv; > > + struct omap1_cam_buf *buf; > > + u32 mode; > > + > > + list_add_tail(&vb->queue, &pcdev->capture); > > + vb->state = VIDEOBUF_QUEUED; > > + > > + if (pcdev->active) { > > + /* > > + * Capture in progress, so don't touch pcdev->ready even if > > + * empty. Since the transfer of the DMA programming register set > > + * content to the DMA working register set is done automatically > > + * by the DMA hardware, this can pretty well happen while we > > + * are keeping the lock here. Levae fetching it from the queue > > "Leave" Yes, sorry. > > + * to be done when a next DMA interrupt occures instead. > > + */ > > + return; > > + } > > superfluous braces I was instructed once to do a reverse in a patch against ASoC subtree (see http://www.mail-archive.com/linux-omap@xxxxxxxxxxxxxxx/msg14863.html), but TBH, I can't find a clear enough requirement specified in the Documentation/CodingStyle, so I probably change my habits, at least for you subtree ;). > > +static void videobuf_done(struct omap1_cam_dev *pcdev, > > + enum videobuf_state result) > > +{ > > + struct omap1_cam_buf *buf = pcdev->active; > > + struct videobuf_buffer *vb; > > + struct device *dev = pcdev->icd->dev.parent; > > + > > + if (WARN_ON(!buf)) { > > + suspend_capture(pcdev); > > + disable_capture(pcdev); > > + return; > > + } > > + > > + if (result == VIDEOBUF_ERROR) > > + suspend_capture(pcdev); > > + > > + vb = &buf->vb; > > + if (waitqueue_active(&vb->done)) { > > + if (!pcdev->ready && result != VIDEOBUF_ERROR) { > > + /* > > + * No next buffer has been entered into the DMA > > + * programming register set on time (could be done only > > + * while the previous DMA interurpt was processed, not > > + * later), so the last DMA block, be it a whole buffer > > + * if in CONTIG or its last sgbuf if in SG mode, is > > + * about to be reused by the just autoreinitialized DMA > > + * engine, and overwritten with next frame data. Best we > > + * can do is stopping the capture as soon as possible, > > + * hopefully before the next frame start. > > + */ > > + suspend_capture(pcdev); > > + } > > superfluous braces ditto. I'll address the issues when ready with my forementioned corner case fixes. Thanks, Janusz > 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 -- 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
