Hello,

On 04/13/2013 01:52 AM, Scott Jiang wrote:
This is a bridge driver for blackfin diplay device.
It can work with ppi or eppi interface. DV timings
are supported.

Signed-off-by: Scott Jiang<scott.jiang.li...@gmail.com>
---
  drivers/media/platform/blackfin/Kconfig        |   15 +-
  drivers/media/platform/blackfin/Makefile       |    1 +
  drivers/media/platform/blackfin/bfin_display.c | 1151 ++++++++++++++++++++++++
  include/media/blackfin/bfin_display.h          |   38 +
  4 files changed, 1203 insertions(+), 2 deletions(-)
  create mode 100644 drivers/media/platform/blackfin/bfin_display.c
  create mode 100644 include/media/blackfin/bfin_display.h

diff --git a/drivers/media/platform/blackfin/Kconfig 
b/drivers/media/platform/blackfin/Kconfig
index cc23997..8a8fd75 100644
--- a/drivers/media/platform/blackfin/Kconfig
+++ b/drivers/media/platform/blackfin/Kconfig
@@ -9,7 +9,18 @@ config VIDEO_BLACKFIN_CAPTURE
          To compile this driver as a module, choose M here: the
          module will be called bfin_capture.

+config VIDEO_BLACKFIN_DISPLAY
+       tristate "Blackfin Video Display Driver"
+       depends on VIDEO_V4L2&&  BLACKFIN&&  I2C
+       select VIDEOBUF2_DMA_CONTIG
+       help
+         V4L2 bridge driver for Blackfin video display device.

Shouldn't it just be "V4L2 output driver", why are you calling it "bridge" ?

+         Choose PPI or EPPI as its interface.
+
+         To compile this driver as a module, choose M here: the
+         module will be called bfin_display.
+
  config VIDEO_BLACKFIN_PPI
        tristate
-       depends on VIDEO_BLACKFIN_CAPTURE
-       default VIDEO_BLACKFIN_CAPTURE
+       depends on VIDEO_BLACKFIN_CAPTURE || VIDEO_BLACKFIN_DISPLAY
+       default VIDEO_BLACKFIN_CAPTURE || VIDEO_BLACKFIN_DISPLAY
diff --git a/drivers/media/platform/blackfin/Makefile 
b/drivers/media/platform/blackfin/Makefile
index 30421bc..015c8f0 100644
--- a/drivers/media/platform/blackfin/Makefile
+++ b/drivers/media/platform/blackfin/Makefile
@@ -1,2 +1,3 @@
  obj-$(CONFIG_VIDEO_BLACKFIN_CAPTURE) += bfin_capture.o
+obj-$(CONFIG_VIDEO_BLACKFIN_DISPLAY) += bfin_display.o
  obj-$(CONFIG_VIDEO_BLACKFIN_PPI)     += ppi.o
diff --git a/drivers/media/platform/blackfin/bfin_display.c 
b/drivers/media/platform/blackfin/bfin_display.c
new file mode 100644
index 0000000..d971d7b
--- /dev/null
+++ b/drivers/media/platform/blackfin/bfin_display.c
@@ -0,0 +1,1151 @@
+/*
+ * Analog Devices video display driver

Sounds a bit too generic.

+ *
+ * Copyright (c) 2011 Analog Devices Inc.

2011 - 2013 ?

+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include<linux/completion.h>
+#include<linux/delay.h>
+#include<linux/errno.h>
+#include<linux/fs.h>
+#include<linux/i2c.h>
+#include<linux/init.h>
+#include<linux/interrupt.h>
+#include<linux/io.h>
+#include<linux/mm.h>
+#include<linux/module.h>
+#include<linux/platform_device.h>
+#include<linux/slab.h>
+#include<linux/time.h>
+#include<linux/types.h>
+
+#include<media/v4l2-chip-ident.h>
+#include<media/v4l2-common.h>
+#include<media/v4l2-ctrls.h>
+#include<media/v4l2-device.h>
+#include<media/v4l2-ioctl.h>
+#include<media/videobuf2-dma-contig.h>
+
+#include<asm/dma.h>
+
+#include<media/blackfin/bfin_display.h>
+#include<media/blackfin/ppi.h>
+
+#define DISPLAY_DRV_NAME        "bfin_display"
+#define DISP_MIN_NUM_BUF        2
+
+struct disp_format {
+       char *desc;
+       u32 pixelformat;
+       enum v4l2_mbus_pixelcode mbus_code;
+       int bpp; /* bits per pixel */
+       int dlen; /* data length for ppi in bits */
+};
+
+struct disp_buffer {
+       struct vb2_buffer vb;
+       struct list_head list;
+};
+
+struct disp_device {
+       /* capture device instance */

Shouldn't it be "output device..." ?

+       struct v4l2_device v4l2_dev;
+       /* v4l2 control handler */
+       struct v4l2_ctrl_handler ctrl_handler;

This handler seems to be unused, I couldn't find any code adding controls
to it. Any initialization of this handler is a dead code now. You probably
want to move that bits to a patch actually adding any controls.

+       /* device node data */
+       struct video_device *video_dev;
+       /* sub device instance */
+       struct v4l2_subdev *sd;
+       /* capture config */
+       struct bfin_display_config *cfg;
+       /* ppi interface */
+       struct ppi_if *ppi;
+       /* current output */
+       unsigned int cur_output;
+       /* current selected standard */
+       v4l2_std_id std;
+       /* current selected dv_timings */
+       struct v4l2_dv_timings dv_timings;
+       /* used to store pixel format */
+       struct v4l2_pix_format fmt;
+       /* bits per pixel*/
+       int bpp;
+       /* data length for ppi in bits */
+       int dlen;
+       /* used to store encoder supported format */
+       struct disp_format *enc_formats;
+       /* number of encoder formats array */
+       int num_enc_formats;
+       /* pointing to current video buffer */
+       struct disp_buffer *cur_frm;
+       /* buffer queue used in videobuf2 */
+       struct vb2_queue buffer_queue;
+       /* allocator-specific contexts for each plane */
+       struct vb2_alloc_ctx *alloc_ctx;
+       /* queue of filled frames */
+       struct list_head dma_queue;
+       /* used in videobuf2 callback */
+       spinlock_t lock;
+       /* used to access display device */
+       struct mutex mutex;
+};
+
+struct disp_fh {
+       struct v4l2_fh fh;
+       /* indicates whether this file handle is doing IO */
+       bool io_allowed;
+};

This structure should not be needed when you use the vb2 helpers. Please see
below for more details.

+static const struct disp_format disp_formats[] = {
+       {
+               .desc        = "YCbCr 4:2:2 Interleaved UYVY 8bits",
+               .pixelformat = V4L2_PIX_FMT_UYVY,
+               .mbus_code   = V4L2_MBUS_FMT_UYVY8_2X8,
+               .bpp         = 16,
+               .dlen        = 8,
+       },
+       {
+               .desc        = "YCbCr 4:2:2 Interleaved YUYV 8bits",
+               .pixelformat = V4L2_PIX_FMT_YUYV,
+               .mbus_code   = V4L2_MBUS_FMT_YUYV8_2X8,
+               .bpp         = 16,
+               .dlen        = 8,
+       },
+       {
+               .desc        = "YCbCr 4:2:2 Interleaved UYVY 16bits",
+               .pixelformat = V4L2_PIX_FMT_UYVY,
+               .mbus_code   = V4L2_MBUS_FMT_UYVY8_1X16,
+               .bpp         = 16,
+               .dlen        = 16,
+       },
+       {
+               .desc        = "RGB 565",
+               .pixelformat = V4L2_PIX_FMT_RGB565,
+               .mbus_code   = V4L2_MBUS_FMT_RGB565_2X8_LE,
+               .bpp         = 16,
+               .dlen        = 8,
+       },
+       {
+               .desc        = "RGB 444",
+               .pixelformat = V4L2_PIX_FMT_RGB444,
+               .mbus_code   = V4L2_MBUS_FMT_RGB444_2X8_PADHI_LE,
+               .bpp         = 16,
+               .dlen        = 8,
+       },
+
+};
+#define DISP_MAX_FMTS ARRAY_SIZE(disp_formats)
+
+static irqreturn_t disp_isr(int irq, void *dev_id);

Couldn't the functions be reordered so this declaration can be avoided ?

+static int disp_open(struct file *file)
+{
+       struct disp_device *disp = video_drvdata(file);
+       struct video_device *vfd = disp->video_dev;
+       struct disp_fh *disp_fh;
+
+       if (!disp->sd) {
+               v4l2_err(&disp->v4l2_dev, "No sub device registered\n");
+               return -ENODEV;
+       }
+
+       disp_fh = kzalloc(sizeof(*disp_fh), GFP_KERNEL);
+       if (!disp_fh) {
+               v4l2_err(&disp->v4l2_dev,
+                        "unable to allocate memory for file handle object\n");

k*alloc functions already log any errors, you could just return -ENOMEM,
without printing anything. There is similar occurrence in disp_probe.

Also it might be a good idea to make your function and data structure names
more specific to this device, e.g. s/disp_*/bfin_disp_*.

+               return -ENOMEM;
+       }
+
+       v4l2_fh_init(&disp_fh->fh, vfd);
+
+       /* store pointer to v4l2_fh in private_data member of file */
+       file->private_data =&disp_fh->fh;
+       v4l2_fh_add(&disp_fh->fh);
+       disp_fh->io_allowed = false;
+       return 0;
+}

+static int disp_reqbufs(struct file *file, void *priv,
+                       struct v4l2_requestbuffers *req_buf)
+{
+       struct disp_device *disp = video_drvdata(file);
+       struct vb2_queue *vq =&disp->buffer_queue;
+       struct v4l2_fh *fh = file->private_data;
+       struct disp_fh *disp_fh = container_of(fh, struct disp_fh, fh);
+
+       if (vb2_is_busy(vq))
+               return -EBUSY;
+
+       disp_fh->io_allowed = true;
+
+       return vb2_reqbufs(vq, req_buf);
+}
+
+static int disp_querybuf(struct file *file, void *priv,
+                               struct v4l2_buffer *buf)
+{
+       struct disp_device *disp = video_drvdata(file);
+
+       return vb2_querybuf(&disp->buffer_queue, buf);
+}
+
+static int disp_qbuf(struct file *file, void *priv,
+                       struct v4l2_buffer *buf)
+{
+       struct disp_device *disp = video_drvdata(file);
+       struct v4l2_fh *fh = file->private_data;
+       struct disp_fh *disp_fh = container_of(fh, struct disp_fh, fh);
+
+       if (!disp_fh->io_allowed)
+               return -EBUSY;
+
+       return vb2_qbuf(&disp->buffer_queue, buf);
+}
+
+static int disp_dqbuf(struct file *file, void *priv,
+                       struct v4l2_buffer *buf)
+{
+       struct disp_device *disp = video_drvdata(file);
+       struct v4l2_fh *fh = file->private_data;
+       struct disp_fh *disp_fh = container_of(fh, struct disp_fh, fh);
+
+       if (!disp_fh->io_allowed)
+               return -EBUSY;
+
+       return vb2_dqbuf(&disp->buffer_queue,
+                               buf, file->f_flags&  O_NONBLOCK);
+}

I would suggest you have a look at the videobuf2 ioctl/fop helpers. Lots of
boilerplate code can be removed when you use them. For an example see:

http://git.linuxtv.org/snawrocki/samsung.git/commitdiff/38b7d67224965bf09eaa3ce147bbebc7fa089411

+static int disp_probe(struct platform_device *pdev)
+{
+       struct 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) {
+               v4l2_err(pdev->dev.driver, "Unable to get board config\n");
+               return -ENODEV;
+       }
+
+       disp = kzalloc(sizeof(*disp), GFP_KERNEL);
+       if (!disp) {
+               v4l2_err(pdev->dev.driver, "Unable to alloc disp\n");
+               return -ENOMEM;
+       }
+
+       disp->cfg = config;
+
+       disp->ppi = ppi_create_instance(config->ppi_info);
+       if (!disp->ppi) {
+               v4l2_err(pdev->dev.driver, "Unable to create ppi\n");
+               ret = -ENODEV;
+               goto err_free_dev;
+       }
+       disp->ppi->priv = disp;
+
+       disp->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
+       if (IS_ERR(disp->alloc_ctx)) {
+               ret = PTR_ERR(disp->alloc_ctx);
+               goto err_free_ppi;
+       }
+
+       vfd = video_device_alloc();

Instead of this allocation you could embed struct video_device instance
in struct disp_device...

+       if (!vfd) {
+               ret = -ENOMEM;
+               v4l2_err(pdev->dev.driver, "Unable to alloc video device\n");
+               goto err_cleanup_ctx;
+       }
+
+       /* initialize field of video device */
+       vfd->release    = video_device_release;

..and make this
        vfd->release    = video_device_release_empty;

+       vfd->fops       =&disp_fops;
+       vfd->ioctl_ops  =&disp_ioctl_ops;
+       vfd->tvnorms    = 0;
+       vfd->v4l2_dev   =&disp->v4l2_dev;
+       vfd->vfl_dir    = VFL_DIR_TX;
+       set_bit(V4L2_FL_USE_FH_PRIO,&vfd->flags);
+       strncpy(vfd->name, DISPLAY_DRV_NAME, sizeof(vfd->name));
+       disp->video_dev = vfd;
+
+       ret = v4l2_device_register(&pdev->dev,&disp->v4l2_dev);
+       if (ret) {
+               v4l2_err(pdev->dev.driver,
+                               "Unable to register v4l2 device\n");
+               goto err_release_vdev;
+       }
+       v4l2_info(&disp->v4l2_dev, "v4l2 device registered\n");
+
+       disp->v4l2_dev.ctrl_handler =&disp->ctrl_handler;
+       ret = v4l2_ctrl_handler_init(&disp->ctrl_handler, 0);
+       if (ret) {
+               v4l2_err(&disp->v4l2_dev,
+                               "Unable to init control handler\n");
+               goto err_unreg_v4l2;
+       }
+
+       spin_lock_init(&disp->lock);
+       /* 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 disp_buffer);
+       q->ops =&disp_video_qops;
+       q->mem_ops =&vb2_dma_contig_memops;
+
+       ret = vb2_queue_init(q);
+       if (ret) {
+               v4l2_err(&disp->v4l2_dev,
+                               "Unable to init videobuf2 queue\n");
+               goto err_free_handler;
+       }
+
+       mutex_init(&disp->mutex);
+
+       /* init video dma queues */
+       INIT_LIST_HEAD(&disp->dma_queue);
+
+       vfd->lock =&disp->mutex;
+
+       /* register video device */
+       ret = video_register_device(disp->video_dev, VFL_TYPE_GRABBER, -1);

The video device should be registered as a last step, only when all
resources it uses are already initialized.

+       if (ret) {
+               v4l2_err(&disp->v4l2_dev,
+                               "Unable to register video device\n");
+               goto err_free_handler;
+       }
+       video_set_drvdata(disp->video_dev, disp);
+       v4l2_info(&disp->v4l2_dev, "video device registered as: %s\n",
+                       video_device_node_name(vfd));
+
+       /* load up the subdevice */
+       i2c_adap = i2c_get_adapter(config->i2c_adapter_id);
+       if (!i2c_adap) {
+               v4l2_err(&disp->v4l2_dev,
+                               "Unable to find i2c adapter\n");
+               goto err_unreg_vdev;
+
+       }
+       disp->sd = v4l2_i2c_new_subdev_board(&disp->v4l2_dev,
+                                                i2c_adap,
+                                               &config->board_info,
+                                                NULL);

nit: I bit strange indentation, you could probably just fit it in 2 lines.

+       if (disp->sd) {
+               int i;
+               if (!config->num_outputs) {
+                       v4l2_err(&disp->v4l2_dev,
+                                       "Unable to work without output\n");
+                       goto err_unreg_vdev;
+               }
+
+               /* update tvnorms from the sub devices */
+               for (i = 0; i<  config->num_outputs; i++)
+                       vfd->tvnorms |= config->outputs[i].std;
+       } else {
+               v4l2_err(&disp->v4l2_dev,
+                               "Unable to register sub device\n");
+               goto err_unreg_vdev;
+       }
+
+       v4l2_info(&disp->v4l2_dev, "v4l2 sub device registered\n");
+
+       /*
+        * explicitly set output, otherwise some boards
+        * may not work at the state as we expected
+        */
+       route =&config->routes[0];
+       ret = v4l2_subdev_call(disp->sd, video, s_routing,
+                               route->output, route->output, 0);
+       if ((ret<  0)&&  (ret != -ENOIOCTLCMD)) {
+               v4l2_err(&disp->v4l2_dev, "Failed to set output\n");
+               goto err_unreg_vdev;
+       }
+       disp->cur_output = 0;
+       /* if this route has specific config, update ppi control */
+       if (route->ppi_control)
+               config->ppi_control = route->ppi_control;
+
+       /* now we can probe the default state */
+       if (config->outputs[0].capabilities&  V4L2_IN_CAP_STD) {
+               v4l2_std_id std;
+               ret = v4l2_subdev_call(disp->sd, core, g_std,&std);
+               if (ret) {
+                       v4l2_err(&disp->v4l2_dev,
+                                       "Unable to get std\n");
+                       goto err_unreg_vdev;
+               }
+               disp->std = std;
+       }
+       if (config->outputs[0].capabilities&  V4L2_IN_CAP_CUSTOM_TIMINGS) {
+               struct v4l2_dv_timings dv_timings;
+               ret = v4l2_subdev_call(disp->sd, video,
+                               g_dv_timings,&dv_timings);
+               if (ret) {
+                       v4l2_err(&disp->v4l2_dev,
+                                       "Unable to get dv timings\n");
+                       goto err_unreg_vdev;
+               }
+               disp->dv_timings = dv_timings;
+       }
+       ret = disp_init_encoder_formats(disp);
+       if (ret) {
+               v4l2_err(&disp->v4l2_dev,
+                               "Unable to create encoder formats table\n");
+               goto err_unreg_vdev;
+       }
+       return 0;
+err_unreg_vdev:
+       video_unregister_device(disp->video_dev);
+       disp->video_dev = NULL;
+err_free_handler:
+       v4l2_ctrl_handler_free(&disp->ctrl_handler);
+err_unreg_v4l2:
+       v4l2_device_unregister(&disp->v4l2_dev);
+err_release_vdev:
+       if (disp->video_dev)
+               video_device_release(disp->video_dev);
+err_cleanup_ctx:
+       vb2_dma_contig_cleanup_ctx(disp->alloc_ctx);
+err_free_ppi:
+       ppi_delete_instance(disp->ppi);
+err_free_dev:
+       kfree(disp);
+       return ret;
+}


Regards,
Sylwester
_______________________________________________
Uclinux-dist-devel mailing list
Uclinux-dist-devel@blackfin.uclinux.org
https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel

Reply via email to