[PATCH v16 01/13] davinci vpbe: V4L2 display driver for DM644X SoC
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon May 2 05:58:23 EDT 2011
Hi Manjunath,
On Tuesday 26 April 2011 16:47:45 Hadli, Manjunath wrote:
> Laurent,
> Can you please review the patches with your suggestions from :
> http://git.linuxtv.org/mhadli/v4l-dvb-davinci_devices.git?a=shortlog;h=refs
> /heads/forkhilman2 and let me know if you think all your suggestions are
> taken care of?
>
> The patch you reviewed was :
>
> http://git.linuxtv.org/mhadli/v4l-dvb-davinci_devices.git?a=commitdiff;h=69
> f60ed7577ab9184ceabd7efbe5bb3453bf7ef1;hp=a400604f47c339831880c50eda6f6b032
> 21579e3
I've reviewed the same patch, here are my comments.
> +/*
> + * vpbe_display_isr()
> + * ISR function. It changes status of the displayed buffer, takes next
buffer
> + * from the queue and sets its address in VPBE registers
> + */
> +static void vpbe_display_isr(unsigned int event, struct vpbe_display
*disp_obj)
> +{
> + struct osd_state *osd_device = disp_obj->osd_device;
> + struct timespec timevalue;
> + struct vpbe_layer *layer;
> + unsigned long addr;
> + int fid;
> + int i;
> +
> + ktime_get_ts(&timevalue);
> +
> + for (i = 0; i < VPBE_DISPLAY_MAX_DEVICES; i++) {
> + layer = disp_obj->dev[i];
> + /* If streaming is started in this layer */
> + if (!layer->started)
> + continue;
What about moving everything above to venc_isr(), and having this function
handle a single layer only ? It will lower the max indentation level. I also
wonder whether you couldn't share some code between the non-interlaced and
the interlaced cases by reorganizing the function body (the fid == 1 code
looks quite similar to the non-interlaced code).
[snip]
> +/**
> + * vpbe_try_format()
> + * If user application provides width and height, and have bytesperline set
> + * to zero, driver calculates bytesperline and sizeimage based on hardware
> + * limits. If application likes to add pads at the end of each line and
> + * end of the buffer , it can set bytesperline to line size and sizeimage
to
> + * bytesperline * height of the buffer. If driver fills zero for active
> + * video width and height, and has requested user bytesperline and
sizeimage,
> + * width and height is adjusted to maximum display limit or buffer width
> + * height which ever is lower
This still sounds a bit cryptic to me.
vpbe_try_format() should return a format closest to what the user requested:
- If the pixel format is invalid, select a default value (done)
- If the field is invalid or not specified, select a default value (partly
done, you don't check for default values)
- If width and/or height are invalid (including being set to 0), select
default values (partly done, you compute width/height based on bytesperline
and sizeimage when they're set to 0, and I don't understand why)
- If bytesperline is invalid (smaller than the minimum value according to the
selected width, or larger than the maximum allowable value), fix it
- If sizeimage is invalid (smaller than the minimum value according the the
selected height and bytesperline), fix it
Is there a need to allow sizeimage values different than
height * bytesperline ?
> + */
[snip]
> +static int vpbe_display_querycap(struct file *file, void *priv,
> + struct v4l2_capability *cap)
> +{
> + struct vpbe_fh *fh = file->private_data;
> + struct vpbe_device *vpbe_dev = fh->disp_dev->vpbe_dev;
> +
> + cap->version = VPBE_DISPLAY_VERSION_CODE;
> + cap->capabilities = V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_STREAMING;
> + strlcpy(cap->driver, VPBE_DISPLAY_DRIVER, sizeof(cap->driver));
> + strlcpy(cap->bus_info, "platform", sizeof(cap->bus_info));
> + /* check the name of davinci device */
> + if (vpbe_dev->cfg->module_name != NULL)
module_name can't be NULL, as it's declared as a char[32].
> + strlcpy(cap->card, vpbe_dev->cfg->module_name,
> + sizeof(cap->card));
> +
> + return 0;
> +}
[snip]
> +static int vpbe_display_g_fmt(struct file *file, void *priv,
> + struct v4l2_format *fmt)
> +{
> + struct vpbe_fh *fh = file->private_data;
> + struct vpbe_layer *layer = fh->layer;
> + struct vpbe_device *vpbe_dev = fh->disp_dev->vpbe_dev;
> +
> + v4l2_dbg(1, debug, &vpbe_dev->v4l2_dev,
> + "VIDIOC_G_FMT, layer id = %d\n",
> + layer->device_id);
> +
> + /* If buffer type is video output */
> + if (V4L2_BUF_TYPE_VIDEO_OUTPUT == fmt->type) {
> + /* Fill in the information about format */
> + fmt->fmt.pix = layer->pix_fmt;
> + } else {
> + v4l2_err(&vpbe_dev->v4l2_dev, "invalid type\n");
> + return -EINVAL;
> + }
You should do it the other way around. Return -EINVAL when the type isn't
OUTPUT, and remove the else. This will increase code readability by decreasing
the max indentation level in the common case.
> +
> + return 0;
> +}
[snip]
> +/**
> + * vpbe_display_enum_output - enumerate outputs
> + *
> + * Enumerates the outputs available at the vpbe display
> + * returns the status, -EINVAL if end of output list
> + */
> +static int vpbe_display_enum_output(struct file *file, void *priv,
> + struct v4l2_output *output)
> +{
> + struct vpbe_fh *fh = priv;
> + struct vpbe_device *vpbe_dev = fh->disp_dev->vpbe_dev;
> + int ret;
> +
> + v4l2_dbg(1, debug, &vpbe_dev->v4l2_dev, "VIDIOC_ENUM_OUTPUT\n");
> +
> + /* Enumerate outputs */
> +
> + if (NULL != vpbe_dev->ops.enum_outputs) {
> + ret = vpbe_dev->ops.enum_outputs(vpbe_dev, output);
> + if (ret) {
> + v4l2_dbg(1, debug, &vpbe_dev->v4l2_dev,
> + "Failed to enumerate outputs\n");
> + return -EINVAL;
> + }
> + } else {
> + return -EINVAL;
> + }
Other way around here too please.
> +
> + return 0;
> +}
> +
> +/**
> + * vpbe_display_s_output - Set output to
> + * the output specified by the index
> + */
> +static int vpbe_display_s_output(struct file *file, void *priv,
> + unsigned int i)
> +{
> + struct vpbe_fh *fh = priv;
> + struct vpbe_layer *layer = fh->layer;
> + struct vpbe_device *vpbe_dev = fh->disp_dev->vpbe_dev;
> + int ret;
> +
> + v4l2_dbg(1, debug, &vpbe_dev->v4l2_dev, "VIDIOC_S_OUTPUT\n");
> + /* If streaming is started, return error */
> + if (layer->started) {
> + v4l2_err(&vpbe_dev->v4l2_dev, "Streaming is started\n");
> + return -EBUSY;
> + }
> + if (NULL != vpbe_dev->ops.set_output) {
> + ret = vpbe_dev->ops.set_output(vpbe_dev, i);
> + if (ret) {
> + v4l2_err(&vpbe_dev->v4l2_dev,
> + "Failed to set output for sub devices\n");
> + return -EINVAL;
> + }
> + } else {
> + return -EINVAL;
> + }
And here too.
> +
> + return 0;
> +}
[snip]
> +static __devinit int init_vpbe_layer(int i, struct vpbe_display *disp_dev,
> + struct platform_device *pdev)
> +{
> + struct vpbe_layer *vpbe_display_layer = NULL;
> + struct video_device *vbd = NULL;
> + int k;
> + int err;
> +
> + /* Allocate memory for four plane display objects */
> +
> + disp_dev->dev[i] =
> + kmalloc(sizeof(struct vpbe_layer), GFP_KERNEL);
You can use kzalloc() and avoid several initializations to 0 below.
> +
> + /* If memory allocation fails, return error */
> + if (!disp_dev->dev[i]) {
> + printk(KERN_ERR "ran out of memory\n");
> + err = -ENOMEM;
> + goto free_mem;
> + }
> + spin_lock_init(&disp_dev->dev[i]->irqlock);
> + mutex_init(&disp_dev->dev[i]->opslock);
> +
> + /* Get the pointer to the layer object */
> + vpbe_display_layer = disp_dev->dev[i];
> + /* Allocate memory for video device */
> + vbd = video_device_alloc();
There's no need to allocate the device dynamically, you can embed struct
video_device into struct vpbe_layer (i.e. replace struct video_device
*video_dev with struct video_device video_dev inside struct vpbe_layer)
(and feel free to rename video_dev to something shorter if needed)
> + if (vbd == NULL) {
> + v4l2_err(&disp_dev->vpbe_dev->v4l2_dev,
> + "ran out of memory\n");
> + err = -ENOMEM;
> + goto free_mem;
> + }
> + /* Initialize field of video device */
> + vbd->release = video_device_release;
You should then use video_device_release_empty instead of
video_device_release.
> + vbd->fops = &vpbe_fops;
> + vbd->ioctl_ops = &vpbe_ioctl_ops;
> + vbd->minor = -1;
> + vbd->v4l2_dev = &disp_dev->vpbe_dev->v4l2_dev;
> + vbd->lock = &vpbe_display_layer->opslock;
> +
> + if (disp_dev->vpbe_dev->current_timings.timings_type &
> + VPBE_ENC_STD) {
> + vbd->tvnorms = (V4L2_STD_525_60 | V4L2_STD_625_50);
> + vbd->current_norm =
> + disp_dev->vpbe_dev->
> + current_timings.timings.std_id;
> + } else
> + vbd->current_norm = 0;
> +
> + snprintf(vbd->name, sizeof(vbd->name),
> + "DaVinci_VPBE Display_DRIVER_V%d.%d.%d",
> + (VPBE_DISPLAY_VERSION_CODE >> 16) & 0xff,
> + (VPBE_DISPLAY_VERSION_CODE >> 8) & 0xff,
> + (VPBE_DISPLAY_VERSION_CODE) & 0xff);
> +
> + /* Set video_dev to the video device */
> + vpbe_display_layer->video_dev = vbd;
> + vpbe_display_layer->device_id = i;
> +
> + vpbe_display_layer->layer_info.id =
> + ((i == VPBE_DISPLAY_DEVICE_0) ? WIN_VID0 : WIN_VID1);
> +
> + /* Initialize field of the display layer objects */
> + vpbe_display_layer->usrs = 0;
> + vpbe_display_layer->io_usrs = 0;
> + vpbe_display_layer->started = 0;
> +
> + /* Initialize prio member of layer object */
> + v4l2_prio_init(&vpbe_display_layer->prio);
> +
> + return 0;
> +
> +free_mem:
> + for (k = 0; k < i-1; k++) {
> + /* Get the pointer to the layer object */
> + vpbe_display_layer = disp_dev->dev[k];
> + /* Release video device */
> + video_device_release(vpbe_display_layer->video_dev);
> + vpbe_display_layer->video_dev = NULL;
> + /* free layer memory */
> + kfree(disp_dev->dev[k]);
> + }
This should be moved to the error cleanup part of vpbe_display_probe(). A
function that registers a single device shouldn't clean other devices up in
case of error.
> +
> + return -ENODEV;
> +}
> +
> +static __devinit int register_devices(int i, struct vpbe_display *disp_dev,
> + struct platform_device *pdev) {
This registers a single device, so I would call it vpbe_register_device.
> + struct vpbe_layer *vpbe_display_layer = NULL;
> + int err;
> + int k;
> +
> + vpbe_display_layer = disp_dev->dev[i];
Please pass disp_dev->dev[i] to the function instead of i.
> + v4l2_info(&disp_dev->vpbe_dev->v4l2_dev,
> + "Trying to register VPBE display device.\n");
> + v4l2_info(&disp_dev->vpbe_dev->v4l2_dev,
> + "layer=%x,layer->video_dev=%x\n",
> + (int)vpbe_display_layer,
> + (int)&vpbe_display_layer->video_dev);
> +
> + err = video_register_device(vpbe_display_layer->video_dev,
> + VFL_TYPE_GRABBER,
> + -1);
> + if (err)
> + goto video_register_failed;
> +
> + vpbe_display_layer->disp_dev = disp_dev;
> + /* set the driver data in platform device */
> + platform_set_drvdata(pdev, disp_dev);
> + video_set_drvdata(vpbe_display_layer->video_dev,
> + vpbe_display_layer);
> +
> + return 0;
> +
> +video_register_failed:
> + for (k = 0; k < i-1; k++)
> + video_unregister_device(vpbe_display_layer->video_dev);
> +
> + for (k = 0; k < VPBE_DISPLAY_MAX_DEVICES; k++) {
> + /* Get the pointer to the layer object */
> + vpbe_display_layer = disp_dev->dev[k];
> + /* Release video device */
> + video_device_release(vpbe_display_layer->video_dev);
> + /* Unregister video device */
> + video_unregister_device(vpbe_display_layer->video_dev);
> + vpbe_display_layer->video_dev = NULL;
> + /* free layer memory */
> + kfree(disp_dev->dev[k]);
> + }
This should be moved to the error cleanup part of vpbe_display_probe() as
well.
> + return -ENODEV;
> +}
> +
> +
> +
> +/*
> + * vpbe_display_probe()
> + * This function creates device entries by register itself to the V4L2
driver
> + * and initializes fields of each layer objects
> + */
> +static __devinit int vpbe_display_probe(struct platform_device *pdev)
> +{
> + struct vpbe_display *disp_dev;
> + struct resource *res;
> + int i;
> + int err;
> + int irq;
> +
> + printk(KERN_DEBUG "vpbe_display_probe\n");
> + /* Allocate memory for vpbe_display */
> + disp_dev = kzalloc(sizeof(struct vpbe_display), GFP_KERNEL);
> + if (!disp_dev) {
> + printk(KERN_ERR "ran out of memory\n");
> + return -ENOMEM;
> + }
> +
> + spin_lock_init(&disp_dev->dma_queue_lock);
> + /*
> + * Scan all the platform devices to find the vpbe
> + * controller device and get the vpbe_dev object
> + */
> + err = bus_for_each_dev(&platform_bus_type, NULL, disp_dev,
> + vpbe_device_get);
> + if (err < 0)
> + return err;
> + /* Initialize the vpbe display controller */
> + if (NULL != disp_dev->vpbe_dev->ops.initialize) {
> + err = disp_dev->vpbe_dev->ops.initialize(&pdev->dev,
> + disp_dev->vpbe_dev);
> + if (err) {
> + v4l2_err(&disp_dev->vpbe_dev->v4l2_dev,
> + "Error initing vpbe\n");
> + err = -ENOMEM;
> + goto probe_out;
> + }
> + }
> +
> + for (i = 0; i < VPBE_DISPLAY_MAX_DEVICES; i++) {
> + if (init_vpbe_layer(i, disp_dev, pdev)) {
> + err = -ENODEV;
> + goto probe_out;
> + }
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> + if (!res) {
> + v4l2_err(&disp_dev->vpbe_dev->v4l2_dev,
> + "Unable to get VENC interrupt resource\n");
> + err = -ENODEV;
> + goto probe_out;
> + }
> +
> + irq = res->start;
> + if (request_irq(irq, venc_isr, IRQF_DISABLED, VPBE_DISPLAY_DRIVER,
> + disp_dev)) {
> + v4l2_err(&disp_dev->vpbe_dev->v4l2_dev,
> + "Unable to request interrupt\n");
> + err = -ENODEV;
> + goto probe_out;
> + }
> +
> + for (i = 0; i < VPBE_DISPLAY_MAX_DEVICES; i++) {
> + if (register_devices(i, disp_dev, pdev)) {
> + err = -ENODEV;
> + goto probe_out;
> + }
> + }
> +
> + printk(KERN_DEBUG "Successfully completed the probing of vpbe v4l2
device\n");
> + return 0;
> +
> +probe_out:
You need to unregister the IRQ handler (and move the cleanup code from the
two previous functions here).
> + kfree(disp_dev);
> + return err;
> +}
[snip]
> +/* Function for module initialization and cleanup */
> +module_init(vpbe_display_init);
> +module_exit(vpbe_display_cleanup);
> +
> +MODULE_DESCRIPTION("TI DMXXX VPBE Display controller");
What about "TI DM644x/DM355/DM365" then ? DMXXX makes it look like it supports
all DaVinci chips.
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Texas Instruments");
--
Regards,
Laurent Pinchart
More information about the linux-arm-kernel
mailing list