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