On Mon, 19 Jan 2009 23:22:33 +0000 Adam Baker <linux@xxxxxxxxxxxxxxxx> wrote: > Add initial support for cameras based on the SQ Technologies SQ-905 > chipset (USB ID 2770:9120) to V4L2 using the gspca infrastructure. [snip] > --- > Following all the comments when I posted this for review Theodore and > I have produced 2 new versions. The most critical comment last time > was that we were using the system work queue inappropriately so this > version creates its own work queue. The alternative version that I > will post shortly avoids a work queue altogether by using > asynchronous USB commands but in order to do so has increased the > code size. > > I'll leave it to the assembled list expertise to say which option is > preferable. [snip] Hello Adam and Theodore, I looked at your two versions, and I think the first one (work queue) is the simplest. So, I am ready to put your driver in my repository for inclusion in a next linux kernel. I have just a few remarks and a request. - There are still small CodingStyle errors. - Why do you need the function name in the debug messages? - In sd_init, you should better convert the 4 bytes to u32 and do a switch. - On disconnection, the function sd_stopN is not called, so the workqueue may be still running. - At streamon time (sd_start), you allocate the buffer and send a command. This may be done in the workqueue function. This function may also do the buffer free and send the stop command on exit. Re-thinking the streaming part gives: . streamon (sd_start) . init_completion() . start the workqueue (dev->streaming is not useful) . workqueue function . allocate the transfer buffer (pointer in the stack) . send 'start capture' . read loop - don't forget: - to test gspca_dev->streaming: it may be streamoff, close or disconnect - to protect to usb_control_msg by the gspca_dev->usb_lock mutex: this will permit to handle future webcam controls. . on streamoff or USB error . free the transfer buffer . complete() . streamoff . sd_stopN: non useful . sd_stop0: . wait_for_completion . dev->work_thread = NULL Now, the request: some guys asked for support of their webcams based on sq930x chips. A SANE backend driver exists, written by Gerard Klaver (http://gkall.hobby.nl/sq930x.html). May you have a look and say if handling these chips may be done in your driver? Regards. -- Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ -- 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
