On 12/12/2014 02:42 AM, Hans Verkuil wrote:
> Hi Shuah,
>
> This is my review for au0828-vbi.c with your patch applied.
Thanks. Will fix all of these.
-- Shuah
>
>> /*
>> au0828-vbi.c - VBI driver for au0828
>>
>> Copyright (C) 2010 Devin Heitmueller <dheitmueller@xxxxxxxxxxxxxx>
>>
>> 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
>
--
Shuah Khan
Sr. Linux Kernel Developer
Samsung Open Source Group
Samsung Research America (Silicon Valley)
shuahkh@xxxxxxxxxxxxxxx | (970) 217-8978
--
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
