Hi Scott,
On 04/28/2013 11:20 PM, Scott Jiang wrote:
> This is a V4L2 driver for Blackfin video display (E)PPI interface.
> This module is common for BF537/BF561/BF548/BF609.
>
> Signed-off-by: Scott Jiang <scott.jiang.linux@xxxxxxxxx>
>From a quick review this patch looks good to me. Feel free to add
Reviewed-by: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
Just couple remarks below.
> +static const struct bfin_disp_format bfin_disp_formats[] = {
> + {
> + .desc = "YCbCr 4:2:2 Interleaved UYVY 8bits",
This string will like get truncated, since buffer is only 32 character
long, including trailing 0. It could be verified with e.g. v4l2-ctl
how much of this string actually shows up in user space.
> + .pixelformat = V4L2_PIX_FMT_UYVY,
> + .mbus_code = V4L2_MBUS_FMT_UYVY8_2X8,
> + .bpp = 16,
> + .dlen = 8,
> + },
> + {
> + .desc = "YCbCr 4:2:2 Interleaved YUYV 8bits",
Ditto.
> + .pixelformat = V4L2_PIX_FMT_YUYV,
> + .mbus_code = V4L2_MBUS_FMT_YUYV8_2X8,
> + .bpp = 16,
> + .dlen = 8,
> + },
> + {
> + .desc = "YCbCr 4:2:2 Interleaved UYVY 16bits",
Ditto.
> + .pixelformat = V4L2_PIX_FMT_UYVY,
> + .mbus_code = V4L2_MBUS_FMT_UYVY8_1X16,
> + .bpp = 16,
> + .dlen = 16,
> + },
[...]
> +static irqreturn_t bfin_disp_isr(int irq, void *dev_id)
> +{
> + struct ppi_if *ppi = dev_id;
> + struct bfin_disp_device *disp = ppi->priv;
> + struct vb2_buffer *vb = &disp->cur_frm->vb;
> + dma_addr_t addr;
> +
> + spin_lock(&disp->lock);
> +
> + if (!list_empty(&disp->dma_queue)) {
> + v4l2_get_timestamp(&vb->v4l2_buf.timestamp);
> + vb->v4l2_buf.flags |= V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
There is no need to set this flag for each buffer, it will be done internally
in videbuf2, if timestamp_type type is set before calling vb2_queue_init().
> + vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
> + disp->cur_frm = list_entry(disp->dma_queue.next,
> + struct bfin_disp_buffer, list);
> + list_del(&disp->cur_frm->list);
> + }
> +
> + clear_dma_irqstat(ppi->info->dma_ch);
> +
> + addr = vb2_dma_contig_plane_dma_addr(&disp->cur_frm->vb, 0);
> + ppi->ops->update_addr(ppi, (unsigned long)addr);
> + ppi->ops->start(ppi);
> +
> + spin_unlock(&disp->lock);
> +
> + return IRQ_HANDLED;
> +}
> +static int bfin_disp_probe(struct platform_device *pdev)
> +{
> + struct bfin_disp_device *disp;
> + struct video_device *vfd;
> + struct i2c_adapter *i2c_adap;
> + struct bfin_display_config *config;
> + struct vb2_queue *q;
> + struct disp_route *route;
> + int ret;
> +
> + config = pdev->dev.platform_data;
> + if (!config || !config->num_outputs) {
> + v4l2_err(pdev->dev.driver, "Invalid board config\n");
> + return -ENODEV;
> + }
> +
> + disp = kzalloc(sizeof(*disp), GFP_KERNEL);
devm_kzalloc() ?
> + if (!disp)
> + return -ENOMEM;
[...]
> +
> + /* initialize queue */
> + q = &disp->buffer_queue;
> + q->type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
> + q->io_modes = VB2_MMAP;
> + q->drv_priv = disp;
> + q->buf_struct_size = sizeof(struct bfin_disp_buffer);
> + q->ops = &bfin_disp_video_qops;
> + q->mem_ops = &vb2_dma_contig_memops;
> + /* provide a mutex to vb2 queue */
> + q->lock = &disp->qlock;
And the timestamp type set up:
q->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> + ret = vb2_queue_init(q);
> + if (ret) {
> + v4l2_err(&disp->v4l2_dev,
> + "Unable to init videobuf2 queue\n");
> + goto err_free_handler;
> + }
Thanks,
Sylwester
--
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
