Re: [New Driver]: usbvideo2 webcam core + pac207 driver using it.

On 09:00 Fri 04 Apr 2008, Hans de Goede wrote:
>  Brandon Philips wrote:
> > 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.
> 
>  Thanks for the review!
> 
> >> struct pac207_decompress_table_t {
> >> 	u8 is_abs;
> >> 	u8 len;
> >> 	s8 val;
> >> };
> > Why add the _t?
> 
>  So that I can write "struct pac207_decompress_table_t 
>  pac207_decompress_table[256];" further on.

But, why does the struct have a _t on the end of the name?  Usually that
is used for typedefs of structs.

> > This all needs some locking to protect from multi-threaded applications.
> > Otherwise the hardware and data structures could be in two different
> > states.
> 
>  They are all called with the usbvideo2 "core" fileop_mutex lock held, as is 
>  documented in usbvideo2.h

Oops, I see that now.

> >> 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.
> 
>  Good catch! Note that this bug is present in the current in mainline zc0301, 
>  et61x251, and sn9c102 drivers too!!
> 

Ok, I will look at this and submit patches.  Thanks.

>  I'm currently trying to merge my work and the work to port gspca as a whole 
>  to v4l2 of Jean-François Moine, so don't expect a new iteration of this 
>  soon, as I first want to have a clear path for merging these 2 works.

Great.  It would be good to get gspca into the Kernel.

Cheers,

	Brandon

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list