On 22:53 Fri 28 Mar 2008, Hans de Goede wrote:
> I'm currently posting these as .c files for easy reading and
> compilation / testing, but I still hope to get a lot of feedback / a
> thorough review, esp of the core <-> pac207 split version as I hope
> to submit that as a patch for mainline inclusion soon.
The driver look pretty good. Comments inline.
> struct pac207_decompress_table_t {
> u8 is_abs;
> u8 len;
> s8 val;
> };
Why add the _t?
> int pac207_read_reg(struct usbvideo2_device* cam, u16 index)
> {
> struct usb_device* udev = cam->usbdev;
> u8* buff = cam->control_buffer;
> int res;
>
> res = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), 0x00,
> USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_INTERFACE,
> 0x00, index, buff, 1, USBVIDEO2_CTRL_TIMEOUT);
> if (res < 0)
> DBG(1, "Failed to read a register (index 0x%04X, error %d)",
> index, res);
>
> return (res >= 0) ? (int)(*buff) : res;
Why not do the obvious thing and return from the if (res < 0) statement?
> /*****************************************************************************/
>
> /* auto gain and exposure algorithm based on the knee algorithm described here:
> http://ytse.tricolour.net/docs/LowLightOptimization.html */
URL is dead.
> #define CLIP(color) (unsigned char)(((color)>0xFF)?0xff:(((color)<0)?0:(color)))
Add a comment about what this is doing? Could you just do it as a
static function instead?
> static int
> pac207_vidioc_s_ctrl(struct file *file, void *fh, struct v4l2_control *a)
> {
> struct usbvideo2_device* cam = fh;
> struct pac207_data *data = cam->cam_data;
> int new_value = a->value;
> int err;
>
> if ((err = pac207_vidioc_g_ctrl(file, fh, a)))
> return err;
>
> if (a->value == new_value)
> return 0;
This all needs some locking to protect from multi-threaded applications.
Otherwise the hardware and data structures could be in two different
states.
> /* don't allow mucking with gain / exposure when using autogain */
> if (data->autogain && (a->id == V4L2_CID_GAIN ||
> a->id == V4L2_CID_EXPOSURE))
> return -EINVAL;
>
> switch (a->id) {
> case V4L2_CID_BRIGHTNESS:
> data->brightness = new_value;
> pac207_write_reg(cam, 0x0008, data->brightness);
> /* give brightness change time to take effect before
> doing autogain based on the new brightness */
> data->autogain_ignore_frames =
> PAC207_AUTOGAIN_IGNORE_FRAMES;
> break;
>
> case V4L2_CID_EXPOSURE:
> data->exposure = new_value;
> pac207_write_reg(cam, 0x0002, data->exposure);
> break;
>
> case V4L2_CID_AUTOGAIN:
> data->autogain = new_value;
> /* when switching to autogain set defaults to make sure
> we are on a valid point of the autogain gain /
> exposure knee graph, and give this change time to
> take effect before doing autogain. */
> if (data->autogain) {
> data->exposure = PAC207_EXPOSURE_DEFAULT;
> data->gain = PAC207_GAIN_DEFAULT;
> data->autogain_ignore_frames =
> PAC207_AUTOGAIN_IGNORE_FRAMES;
> pac207_write_reg(cam, 0x0002, data->exposure);
> pac207_write_reg(cam, 0x000e, data->gain);
> }
> break;
>
> case V4L2_CID_GAIN:
> data->gain = new_value;
> pac207_write_reg(cam, 0x000e, data->gain);
> break;
>
> /* no default needed already checked in pac207_vidioc_g_ctrl */
> }
>
> pac207_write_reg(cam, 0x13, 0x01); /* load registers to sensor */
> pac207_write_reg(cam, 0x1c, 0x01); /* not documented */
>
> return 0;
> }
> static void usbvideo2_urb_complete(struct urb *urb)
> {
> struct usbvideo2_device* cam = urb->context;
> struct usbvideo2_frame_t** f;
> int i, ret;
>
> switch (urb->status) {
> case 0:
> break;
> case -ENOENT: /* usb_kill_urb() called. */
> case -ECONNRESET: /* usb_unlink_urb() called. */
> case -ESHUTDOWN: /* The endpoint is being disabled. */
> return;
> default:
> goto resubmit_urb;
> }
>
> f = &cam->frame_current;
>
> if (!(*f)) {
> if (list_empty(&cam->inqueue))
> goto resubmit_urb;
>
> (*f) = list_entry(cam->inqueue.next, struct usbvideo2_frame_t,
> frame);
> }
Don't you want to take a spinlock here? Most accesses of inqueue seem
to take a spinlock.
> static ssize_t
> usbvideo2_read(struct file* filp, char __user * buf, size_t count, loff_t* f_pos)
> {
> struct usbvideo2_device *cam = filp->private_data;
> struct usbvideo2_frame_t *f;
> unsigned long lock_flags;
> long timeout;
> int err = 0;
>
>
> if (cam->disconnected) {
> err = -ENODEV;
> goto out;
> }
>
> if (cam->io == IO_MMAP) {
> DBG(2, "Close and open the device again to choose the read "
> "method");
> err = -EBUSY;
> goto out;
> }
>
> static void usbvideo2_vm_close(struct vm_area_struct* vma)
> {
> /* NOTE: buffers are not freed here */
Why is that worth noting?
> struct usbvideo2_frame_t* f = vma->vm_private_data;
> f->vma_use_count--;
> }
>
>
> static int
> usbvideo2_vidioc_reqbufs(struct file *file, void *fh, struct v4l2_requestbuffers *b)
> {
> u32 i;
> struct usbvideo2_device* cam = fh;
>
> if (b->type != V4L2_BUF_TYPE_VIDEO_CAPTURE ||
> b->memory != V4L2_MEMORY_MMAP)
> return -EINVAL;
>
> if (cam->io == IO_READ) {
> DBG(2, "Close and open the device again to choose the "
> "mmap I/O method");
> return -EBUSY;
> }
Again, why is close then open required?
> static int
> usbvideo2_vidioc_dqbuf(struct file *file, void *fh, struct v4l2_buffer *b)
> {
> struct usbvideo2_device* cam = fh;
> struct usbvideo2_frame_t *f;
> unsigned long lock_flags;
> long timeout;
>
> if (b->type != V4L2_BUF_TYPE_VIDEO_CAPTURE || cam->io != IO_MMAP ||
> cam->stream == STREAM_OFF)
> return -EINVAL;
>
> if (list_empty(&cam->outqueue)) {
> if (file->f_flags & O_NONBLOCK)
> return -EAGAIN;
>
> timeout = wait_event_interruptible_timeout(cam->wait_frame,
> !list_empty(&cam->outqueue) ||
> cam->disconnected,
> msecs_to_jiffies(USBVIDEO2_FRAME_TIMEOUT) );
> if (cam->disconnected)
> return -ENODEV;
>
> if (timeout <= 0)
> return (timeout < 0)? timeout : -EIO;
> }
Where is the locking? What happens if two threads call dqbuf on this
device at the same time?
> if (cam->funcs->frame_dequeued)
> cam->funcs->frame_dequeued(cam);
>
> spin_lock_irqsave(&cam->queue_lock, lock_flags);
You will probably hit LIST_POISON on a second thread because of the list
being empty when it comes through.
> f = list_entry(cam->outqueue.next, struct usbvideo2_frame_t, frame);
> list_del(cam->outqueue.next);
> spin_unlock_irqrestore(&cam->queue_lock, lock_flags);
>
> f->state = F_UNUSED;
>
> memcpy(b, &f->buf, sizeof(*b));
> if (f->vma_use_count)
> b->flags |= V4L2_BUF_FLAG_MAPPED;
>
> return 0;
> }
> struct usbvideo2_frame_t {
> void* bufmem;
> struct v4l2_buffer buf;
> enum usbvideo2_frame_state state;
> struct list_head frame;
> unsigned long vma_use_count;
> };
Why the _t on the end?
Thanks,
Brandon
_______________________________________________
Fedora-kernel-list mailing list
Fedora-kernel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/fedora-kernel-list
