Re: [PATCH] Add support for sq905 based cameras to gspca

[Jean-Francois Moine <moinejf@free.fr>]

On Mon, 19 Jan 2009 23:22:33 +0000
Adam Baker <linux@baker-net.org.uk> 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/