Re: [Linux-uvc-devel] Comments about the driver

[Laurent P. <lau...@sk...>]

Hi Luc,

Thanks for your comments. I fixed the locking issue, added the missing static 
keywords and will add a counter to open() to temporarly fix the multiple open 
issue (I should implement multiple open support though).

As for your other comments, I answered inline.

> > /*
> >  *      usbvideo.c  --  USB Video Class driver
> >  *
> >  *      Copyright (C) 2005
>
> Now you can update the year :-)

Done. Happy New Year :-)

> union without name can't be build with gcc2.95 is it ok ?

As long as I don't receive complains about it, yes :-)

> > static int uvc_alloc_buffers(struct uvc_video_queue *queue, unsigned int
> > nbuffers, unsigned int buflength)
> > {
> > 	unsigned int bufsize = PAGE_ALIGN(buflength);
> > 	unsigned int i;
> > 	void *mem = NULL;
> > 	int ret;
> >
[...]
> > 	for (i = 0; i < nbuffers; ++i) {
> > 		memset(&queue->buffer[i], 0, sizeof queue->buffer[i]);
> > 		queue->buffer[i].size = bufsize;
> > 		queue->buffer[i].buf.index = i;
> > 		queue->buffer[i].buf.m.offset = i * bufsize;
>
> Is any buffersize aligned on a page boudary ?

Not sure to understand what you mean. bufsize is always page aligned as I 
initialized it to PAGE_ALIGN(buflength).

> > static int uvc_dequeue_buffer(struct uvc_video_queue *queue,
> > 		struct v4l2_buffer *v4l2_buf, int nonblocking)
> > {
> > 	struct uvc_buffer *buf;
> > 	int ret = 0;
> >
> > 	if (v4l2_buf->type != V4L2_BUF_TYPE_VIDEO_CAPTURE ||
> > 	    v4l2_buf->memory != V4L2_MEMORY_MMAP)
> > 		return -EINVAL;
> >
> > 	down(&queue->lock);
> > 	if (list_empty(&queue->mainqueue)) {
> > 		ret = -EINVAL;
> > 		goto done;
> > 	}
> >
> > 	buf = list_entry(queue->mainqueue.next, struct uvc_buffer, stream);
> > 	if ((ret = uvc_queue_waiton(buf, nonblocking)) < 0)
> > 		goto done;
> >
> > 	switch (buf->state) {
> > 	case UVC_BUF_STATE_ERROR:
> > 		ret = -EIO;
>
>                 ^^^^ ? need a goto done or is it wanted ?

It's wanted. A buffer marked as erroneous by the driver must still be removed 
from the list, otherwise it will always stay there and no buffer will ever be 
dequeued anymore.

> > static int uvc_set_video_ctrl(struct uvc_video_device *video,
> > 	struct uvc_streaming_control *ctrl, int probe)
> > {
> > 	__u8 data[34];
>
>         is it not posible to create a structure and access it ?
>         i think it will more readable than data[2] ?

Yes, I can do that. I wasn't sure it was worth it, as data is just a block of 
bytes to be passed to the device.

Laurent Pinchart