Hi Shuah,
This is my review for au0828-vbi.c with your patch applied.
> /*
> au0828-vbi.c - VBI driver for au0828
>
> Copyright (C) 2010 Devin Heitmueller <dheitmuel...@kernellabs.com>
>
> This work was sponsored by GetWellNetwork Inc.
>
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 2 of the License, or
> (at your option) any later version.
>
> 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.
>
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301, USA.
> */
>
> #include "au0828.h"
>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/init.h>
> #include <linux/slab.h>
>
> static unsigned int vbibufs = 5;
> module_param(vbibufs, int, 0644);
> MODULE_PARM_DESC(vbibufs, "number of vbi buffers, range 2-32");
Drop this vbibufs argument. It's bogus.
>
> /* ------------------------------------------------------------------ */
>
> static int vbi_queue_setup(struct vb2_queue *vq, const struct v4l2_format
> *fmt,
> unsigned int *nbuffers, unsigned int *nplanes,
> unsigned int sizes[], void *alloc_ctxs[])
> {
> struct au0828_dev *dev = vb2_get_drv_priv(vq);
> unsigned long size;
>
> if (fmt)
> size = fmt->fmt.pix.sizeimage;
It's a VBI format, so the size is
fmt->fmt.vbi.samples_per_line *
(fmt->fmt.vbi.count[0] + fmt->fmt.vbi.count[1]);
> else
> size = dev->vbi_width * dev->vbi_height * 2;
>
> if (0 == *nbuffers)
> *nbuffers = 32;
> if (*nbuffers < 2)
> *nbuffers = 2;
> if (*nbuffers > 32)
> *nbuffers = 32;
Remove these checks, they are not needed.
But if a fmt is passed in, then you *do* need to check if the new format size
is enough to store the vbi data. If not, return -EINVAL.
Test with v4l2-compliance -V0 -s.
>
> *nplanes = 1;
> sizes[0] = size;
>
> return 0;
> }
>
> static int vbi_buffer_prepare(struct vb2_buffer *vb)
> {
> struct au0828_dev *dev = vb2_get_drv_priv(vb->vb2_queue);
> struct au0828_buffer *buf = container_of(vb, struct au0828_buffer, vb);
> unsigned long size;
>
> size = dev->vbi_width * dev->vbi_height * 2;
>
> if (vb2_plane_size(vb, 0) < size) {
> pr_err("%s data will not fit into plane (%lu < %lu)\n",
> __func__, vb2_plane_size(vb, 0), size);
> return -EINVAL;
> }
> vb2_set_plane_payload(&buf->vb, 0, size);
>
> return 0;
> }
>
> static void
> vbi_buffer_queue(struct vb2_buffer *vb)
> {
> struct au0828_dev *dev = vb2_get_drv_priv(vb->vb2_queue);
> struct au0828_buffer *buf = container_of(vb, struct au0828_buffer, vb);
> struct au0828_dmaqueue *vbiq = &dev->vbiq;
> unsigned long flags = 0;
>
> buf->mem = vb2_plane_vaddr(vb, 0);
> buf->length = vb2_plane_size(vb, 0);
>
> spin_lock_irqsave(&dev->slock, flags);
> list_add_tail(&buf->list, &vbiq->active);
> spin_unlock_irqrestore(&dev->slock, flags);
> }
>
> struct vb2_ops au0828_vbi_qops = {
> .queue_setup = vbi_queue_setup,
> .buf_prepare = vbi_buffer_prepare,
> .buf_queue = vbi_buffer_queue,
> .start_streaming = au0828_start_analog_streaming,
> .stop_streaming = au0828_stop_vbi_streaming,
> .wait_prepare = vb2_ops_wait_prepare,
> .wait_finish = vb2_ops_wait_finish,
> };
Regards,
Hans
--
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
