Hi, Jean-Christophe
Thank you for the review.
Jean-Christophe PLAGNIOL-VILLARD wrote on Friday, May 27, 2011 8:06 PM:
>> +/* ISI interrupt service routine */
>> +static irqreturn_t isi_interrupt(int irq, void *dev_id) {
>> + struct atmel_isi *isi = dev_id;
>> + u32 status, mask, pending;
>> + irqreturn_t ret = IRQ_NONE;
>> +
>> + spin_lock(&isi->lock);
>> +
>> + status = isi_readl(isi, ISI_STATUS);
>> + mask = isi_readl(isi, ISI_INTMASK);
>> + pending = status & mask;
>> +
>> + if (pending & ISI_CTRL_SRST) {
>> + complete(&isi->isi_complete);
>> + isi_writel(isi, ISI_INTDIS, ISI_CTRL_SRST);
>> + ret = IRQ_HANDLED;
>> + }
>> + if (pending & ISI_CTRL_DIS) {
>> + complete(&isi->isi_complete);
>> + isi_writel(isi, ISI_INTDIS, ISI_CTRL_DIS);
>> + ret = IRQ_HANDLED;
>> + }
> no else here?
>> +
>> + if (pending & ISI_SR_VSYNC) {
>> + switch (isi->state) {
>> + case ISI_STATE_IDLE:
>> + isi->state = ISI_STATE_READY;
>> + wake_up_interruptible(&isi->capture_wq);
>> + break;
>> + }
> really switch here?
I will remove the switch here.
I think this part of IRQ handling code need to refine a little bit. The SRST
and DIS_DONE is more independent. And other interrupts can compose together.
Following is the latest code, I think is more reasonable.
if (pending & ISI_CTRL_SRST) {
complete(&isi->complete);
isi_writel(isi, ISI_INTDIS, ISI_CTRL_SRST);
ret = IRQ_HANDLED;
} else if (pending & ISI_CTRL_DIS) {
complete(&isi->complete);
isi_writel(isi, ISI_INTDIS, ISI_CTRL_DIS);
ret = IRQ_HANDLED;
} else {
if ((pending & ISI_SR_VSYNC) &&
(isi->state == ISI_STATE_IDLE)) {
isi->state = ISI_STATE_READY;
wake_up_interruptible(&isi->vsync_wq);
ret = IRQ_HANDLED;
}
if (likely(pending & ISI_SR_CXFR_DONE))
ret = atmel_isi_handle_streaming(isi);
}
>> + } else if (likely(pending & ISI_SR_CXFR_DONE)) {
>> + ret = atmel_isi_handle_streaming(isi);
>> + }
>> +
>> + spin_unlock(&isi->lock);
>> +
>> + return ret;
>> +}
>> +
>> +#define WAIT_ISI_RESET 1
>> +#define WAIT_ISI_DISABLE 0
>> +static int atmel_isi_wait_status(int wait_reset, struct atmel_isi
>> +*isi)
>I thinkhave teh atmel_isti first parameter is better
I will fix it.
>> +{
>> + unsigned long timeout;
>> + /*
>> + * The reset or disable will only succeed if we have a
>> + * pixel clock from the camera.
>> + */
>> + init_completion(&isi->isi_complete);
>> +
>> + if (wait_reset) {
>> + isi_writel(isi, ISI_INTEN, ISI_CTRL_SRST);
>> + isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
>> + } else {
>> + isi_writel(isi, ISI_INTEN, ISI_CTRL_DIS);
>> + isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>> + }
>> +
>> + timeout = wait_for_completion_timeout(&isi->isi_complete,
>> + msecs_to_jiffies(100));
>> + if (timeout == 0)
>> + return -ETIMEDOUT;
>> +
>> + return 0;
>> +}
>> +
>> +/* ------------------------------------------------------------------
>> + Videobuf operations
>> +
>> +------------------------------------------------------------------*/
>> +static int queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
>> + unsigned int *nplanes, unsigned long sizes[],
>> + void *alloc_ctxs[])
>> +{
>> + struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
>> + struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
>> + struct atmel_isi *isi = ici->priv;
>> + unsigned long size;
>> + int ret, bytes_per_line;
>> +
>> + /* Reset ISI */
>> + ret = atmel_isi_wait_status(WAIT_ISI_RESET, isi);
>> + if (ret < 0) {
>> + dev_err(icd->dev.parent, "Reset ISI timed out\n");
>> + return ret;
>> + }
>> + /* Disable all interrupts */
>> + isi_writel(isi, ISI_INTDIS, ~0UL);
>> +
>> + bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
>> + icd->current_fmt->host_fmt);
>> +
>> + if (bytes_per_line < 0)
>> + return bytes_per_line;
>> +
>> + size = bytes_per_line * icd->user_height;
>> +
>> + if (*nbuffers == 0)
>> + *nbuffers = MAX_BUFFER_NUMS;
>> + if (*nbuffers > MAX_BUFFER_NUMS)
> else here
I will add it.
>> + *nbuffers = MAX_BUFFER_NUMS;
>> +
>> + if (size * *nbuffers > VID_LIMIT_BYTES)
>> + *nbuffers = VID_LIMIT_BYTES / size;
>> +
>> + *nplanes = 1;
>> + sizes[0] = size;
>> + alloc_ctxs[0] = isi->alloc_ctx;
>> +
>> + isi->sequence = 0;
>> + isi->active = NULL;
>> +
>> + dev_dbg(icd->dev.parent, "%s, count=%d, size=%ld\n", __func__,
>> + *nbuffers, size);
>> +
>> + return 0;
>> +}
>> +
>> +static int buffer_init(struct vb2_buffer *vb) {
>> + struct frame_buffer *buf = container_of(vb, struct frame_buffer,
>> +vb);
>> +
>> + buf->p_fb_desc = NULL;
>> + buf->fb_desc_phys = 0;
> memset 0?
OK.
>> + INIT_LIST_HEAD(&buf->list);
>> +
>> + return 0;
>> +}
>> +
>otherwise the patch look good
>if you fix the upper issue
>Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagn...@jcrosoft.com>
Thank you very much. I will send out version3 soon.
Best Regards,
Josh Wu
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
