Please test the gspca-stv06xx branch

Please test the gspca-stv06xx branch

[Erik Andrén]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

I've reworked the driver somewhat and added initial support for th
pb0100.
Please test with the latest version of the gspca-stv06xx tree and
see if you can get an image. Ekiga works best for me at the moment.

If it doesn't please enable full debug output by supplying the debug
parameter to the gspca_main kernel module and send me the output.
Patches fixing problems are of course even more welcome :)

Regards,
Erik
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)

iEYEARECAAYFAkkrFeEACgkQN7qBt+4UG0FqDACffx7ziaiOfcMX8EpOjGN+nBM+
iecAnRHR+ab6XwxrOabKCblJWoFH7Oo5
=L1hW
-----END PGP SIGNATURE-----

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: Please test the gspca-stv06xx branch

[Chia-I Wu]

[-- Attachment #1: Type: text/plain, Size: 1116 bytes --]

Hi Erik,

On Mon, Nov 24, 2008 at 10:00:17PM +0100, Erik Andrén wrote:
> I've reworked the driver somewhat and added initial support for th
> pb0100.
> Please test with the latest version of the gspca-stv06xx tree and
> see if you can get an image. Ekiga works best for me at the moment.
I am trying to make gspca-stv06xx work with my QuickCam Express
(046d:0840).  It comes with the HDCS 1000 sensor.  So far, I am able to
receive frames using gstreamer (with libv4l).  The colors are wrong
though.

While working on it, I encounter two minor issues:

* stv06xx_write_sensor sends an extra packet unconditionally.  It causes
  the function call return error.
* Turning LED on/off kills the device.  I have to re-plug the device to
  make it work again.

I could put those functions inside an if clause:

	if (udev->descriptor.idProduct != 0x840)
		do_something;

and things work.  But as I do not have other cameras to test, I am not
sure if this is the right way.  Do you have any suggestion?

I will keep working on it.  But you can find a primitive patch and a
sample image in the attachments.

-- 
Regards,
olv

[-- Attachment #2: qc-ex-primitive.patch --]
[-- Type: text/x-diff, Size: 7510 bytes --]

diff -r 4e778359e610 -r 378675539541 linux/drivers/media/video/gspca/stv06xx/stv06xx.c
--- a/linux/drivers/media/video/gspca/stv06xx/stv06xx.c	Mon Nov 24 18:06:49 2008 +0100
+++ b/linux/drivers/media/video/gspca/stv06xx/stv06xx.c	Tue Nov 25 15:55:12 2008 +0800
@@ -127,10 +127,12 @@
 			      STV06XX_URB_MSG_TIMEOUT);
 
 	/* Quickam Web needs an extra packet */
-	buf[0] = 0;
-	err = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
-			      0x04, 0x40, 0x1704, 0, buf, 1,
-			      STV06XX_URB_MSG_TIMEOUT);
+	if (udev->descriptor.idProduct != 0x840) {
+		buf[0] = 0;
+		err = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
+				      0x04, 0x40, 0x1704, 0, buf, 1,
+				      STV06XX_URB_MSG_TIMEOUT);
+	}
 
 	return (err < 0) ? err : 0;
 }
@@ -261,9 +263,11 @@
 		goto out;
 
 	/* Turn on LED */
-	err = stv06xx_write_bridge(sd, STV_LED_CTRL, LED_ON);
-	if (err < 0)
-		goto out;
+	if (sd->gspca_dev.dev->descriptor.idProduct != 0x840) {
+		err = stv06xx_write_bridge(sd, STV_LED_CTRL, LED_ON);
+		if (err < 0)
+			goto out;
+	}
 
 	/* Start isochronous streaming */
 	err = stv06xx_write_bridge(sd, STV_ISO_ENABLE, 1);
@@ -283,9 +287,11 @@
 	struct sd *sd = (struct sd *) gspca_dev;
 
 	/* Turn off LED */
-	err = stv06xx_write_bridge(sd, STV_LED_CTRL, LED_OFF);
-	if (err < 0)
-		goto out;
+	if (sd->gspca_dev.dev->descriptor.idProduct != 0x840) {
+		err = stv06xx_write_bridge(sd, STV_LED_CTRL, LED_OFF);
+		if (err < 0)
+			goto out;
+	}
 
 	/* stop ISO-streaming */
 	err = stv06xx_write_bridge(sd, STV_ISO_ENABLE, 0);
diff -r 4e778359e610 -r 378675539541 linux/drivers/media/video/gspca/stv06xx/stv06xx_hdcs.c
--- a/linux/drivers/media/video/gspca/stv06xx/stv06xx_hdcs.c	Mon Nov 24 18:06:49 2008 +0100
+++ b/linux/drivers/media/video/gspca/stv06xx/stv06xx_hdcs.c	Tue Nov 25 15:55:12 2008 +0800
@@ -57,16 +57,132 @@
 	return -ENODEV;
 }
 
+static int hdcs_set_exposure(struct sd *sd, int val)
+{
+	unsigned int rowexp;		/* rowexp,srowexp = 15 bits (0..32767) */
+	unsigned int srowexp;		/* sub-row exposure (smaller is brighter) */
+	unsigned int max_srowexp;	/* Maximum srowexp value + 1 */
+	int width = 360;
+
+	/* Absolute black at srowexp=2672,width=360; 2616, width=352; 1896, width=256 for hdcs1000 */
+
+	val *= 16;		/* 16 seems to be the smallest change that actually affects brightness */
+	max_srowexp = width * 15 / 2 - 104 + 1;
+	srowexp = max_srowexp - (val % max_srowexp) - 1;
+	rowexp  = val / max_srowexp;
+
+	/* Number of rows to expose */
+	stv06xx_write_sensor_b(sd, HDCS_ROWEXPL, rowexp & 0xff);
+	stv06xx_write_sensor_b(sd, HDCS_ROWEXPH, rowexp >> 8);
+
+	if (IS_1020(sd)) {
+		srowexp = 0;	//FIXME:need formula to compute srowexp for HDCS1020!
+		srowexp >>= 2;					/* Bits 0..1 are hardwired to 0 */
+
+		/* Number of pixels to expose */
+		stv06xx_write_sensor_b(sd, HDCS20_SROWEXP, srowexp & 0xff);
+	} else {
+		/* Number of pixels to expose */
+		stv06xx_write_sensor_b(sd, HDCS00_SROWEXPL, srowexp & 0xff);
+		stv06xx_write_sensor_b(sd, HDCS00_SROWEXPH, srowexp >> 8);
+	}
+
+	if (IS_1020(sd)) {
+		/* Reset exposure error flag */
+		stv06xx_write_sensor_b(sd, HDCS20_ERROR, BIT(0));
+	} else {
+		/* Reset exposure error flag */
+		stv06xx_write_sensor_b(sd, HDCS_STATUS, BIT(4));
+	}
+
+	return 0;
+}
+
+static int hdcs_set_gains(struct sd *sd, unsigned int hue, unsigned int sat, unsigned int val)
+{
+	static const unsigned int min_gain = 8;
+	unsigned int rgain, bgain, ggain;
+
+	//qc_hsv2rgb(hue, sat, val, &rgain, &bgain, &ggain);
+	rgain = hue;
+	ggain = sat;
+	bgain = val;
+
+	rgain >>= 8;					/* After this the values are 0..255 */
+	ggain >>= 8;
+	bgain >>= 8;
+
+	rgain = max(rgain, min_gain);			/* Do not allow very small values, they cause bad (low-contrast) image */
+	ggain = max(ggain, min_gain);
+	bgain = max(bgain, min_gain);
+
+	if (rgain > 127) rgain = rgain/2 | BIT(7);	/* Bit 7 doubles the programmed values */
+	if (ggain > 127) ggain = ggain/2 | BIT(7);	/* Double programmed value if necessary */
+	if (bgain > 127) bgain = bgain/2 | BIT(7);
+
+	stv06xx_write_sensor_b(sd, HDCS_ERECPGA, ggain);
+	stv06xx_write_sensor_b(sd, HDCS_EROCPGA, rgain);
+	stv06xx_write_sensor_b(sd, HDCS_ORECPGA, bgain);
+	stv06xx_write_sensor_b(sd, HDCS_OROCPGA, ggain);
+
+	return 0;
+}
+
+static int hdcs_set_size(struct sd *sd, unsigned int width, unsigned int height)
+{
+	/* The datasheet doesn't seem to say this, but HDCS-1000
+	 * has visible windows size of 360x296 pixels, the first upper-left
+	 * visible pixel is at 8,8.
+	 * From Andrey's test image: looks like HDCS-1020 upper-left
+	 * visible pixel is at 24,8 (y maybe even smaller?) and lower-right
+	 * visible pixel at 375,299 (x maybe even larger?)
+	 */
+	unsigned int originx   = IS_1020(sd) ? 24 : 8;		/* First visible pixel */
+	unsigned int maxwidth  = IS_1020(sd) ? 352 : 360;	/* Visible sensor size */
+	unsigned int originy   = 8;
+	unsigned int maxheight = IS_1020(sd) ? 292 : 296;
+	unsigned int x, y;
+	int ret;
+
+	printk("set size\n");
+
+	width  = (width + 3) / 4 * 4;	/* Width must be multiple of 4 */
+	height = (height + 3)/ 4 * 4;	/* Height must be multiple of 4 */
+
+	x = (maxwidth - width) / 2;			/* Center image by computing upper-left corner */
+	y = (maxheight - height) / 2;
+	width /= 4;
+	height /= 4;
+	x = (x + originx) / 4;				/* Must be multiple of 4 (low bits wired to 0) */
+	y = (y + originy) / 4;
+
+	ret = stv06xx_write_sensor_b(sd, GET_CONTROL, 0);
+
+	stv06xx_write_sensor_b(sd, HDCS_FWROW, y);
+	stv06xx_write_sensor_b(sd, HDCS_FWCOL, x);
+	stv06xx_write_sensor_b(sd, HDCS_LWROW, y + height - 1);
+	stv06xx_write_sensor_b(sd, HDCS_LWCOL, x + width - 1);
+	
+	if (1)
+		hdcs_set_exposure(sd, 32768);
+
+	ret = stv06xx_write_sensor_b(sd, GET_CONTROL, HDCS_RUN_ENABLE);
+	printk("set gains: %d\n", ret);
+	hdcs_set_gains(sd, 32768, 32768, 32768);
+
+	return 0;
+}
+
 int hdcs_start(struct sd *sd)
 {
-	int err = stv06xx_write_sensor_b(sd, HDCS_RUN_ENABLE, GET_CONTROL);
+	int err = stv06xx_write_sensor_b(sd, GET_CONTROL, HDCS_RUN_ENABLE);
 	PDEBUG(D_STREAM, "Starting stream");
 	return (err < 0) ? err : 0;
 }
 
 int hdcs_stop(struct sd *sd)
 {
-	int err = stv06xx_write_sensor_b(sd, HDCS_SLEEP_MODE, GET_CONTROL);
+	int err = stv06xx_write_sensor_b(sd, GET_CONTROL, HDCS_SLEEP_MODE);
 	PDEBUG(D_STREAM, "Halting stream");
 	return (err < 0) ? err : 0;
 }
@@ -182,9 +298,17 @@
 
 	/* CONFIG: Bit 3: continous frame capture,
 	   bit 2: stop when frame complete */
-	stv06xx_write_sensor_b(sd, GET_CONTROL, BIT(3));
+	stv06xx_write_sensor_b(sd, GET_CONFIG, BIT(3));
 	/* ADC output resolution to 10 bits */
 	stv06xx_write_sensor_b(sd, HDCS_ADCCTRL, 10);
+
+	stv06xx_write_bridge(sd, STV_ISO_ENABLE, 0);
+	hdcs_stop(sd);
+
+	hdcs_set_size(sd, 360, 296);
+
+	info("hdcs_inited");
+
 	return 0;
 }
 
diff -r 4e778359e610 -r 378675539541 linux/drivers/media/video/gspca/stv06xx/stv06xx_hdcs.h
--- a/linux/drivers/media/video/gspca/stv06xx/stv06xx_hdcs.h	Mon Nov 24 18:06:49 2008 +0100
+++ b/linux/drivers/media/video/gspca/stv06xx/stv06xx_hdcs.h	Tue Nov 25 15:55:12 2008 +0800
@@ -114,6 +114,7 @@
 
 #define IS_870(sd)	((sd)->gspca_dev.dev->descriptor.idProduct == 0x870)
 #define IS_1020(sd)	((sd)->sensor == &stv06xx_sensor_hdcs1020)
+#define GET_CONFIG	(IS_1020(sd) ? HDCS20_CONFIG : HDCS00_CONFIG)
 #define GET_CONTROL	(IS_1020(sd) ? HDCS20_CONTROL : HDCS00_CONTROL)
 
 int hdcs_probe(struct sd *sd);
@@ -126,6 +127,7 @@
 	.name = "HP HDCS-1000/1100",
 	.i2c_flush = 0,
 	.i2c_addr = HDCS_ADDR,
+	.i2c_len = 1,
 
 	.init = hdcs_init,
 	.probe = hdcs_probe,

[-- Attachment #3: qc.png --]
[-- Type: image/png, Size: 84574 bytes --]

[-- Attachment #4: Type: text/plain, Size: 164 bytes --]

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: Please test the gspca-stv06xx branch

[Erik Andrén]

Hi,

2008/11/25 Chia-I Wu <olvaffe@gmail.com>:
> Hi Erik,
>
> On Mon, Nov 24, 2008 at 10:00:17PM +0100, Erik Andrén wrote:
>> I've reworked the driver somewhat and added initial support for th
>> pb0100.
>> Please test with the latest version of the gspca-stv06xx tree and
>> see if you can get an image. Ekiga works best for me at the moment.
> I am trying to make gspca-stv06xx work with my QuickCam Express
> (046d:0840).  It comes with the HDCS 1000 sensor.  So far, I am able to
> receive frames using gstreamer (with libv4l).  The colors are wrong
> though.
>
> While working on it, I encounter two minor issues:
>
> * stv06xx_write_sensor sends an extra packet unconditionally.  It causes
>  the function call return error.

Do you mean the extra packet to register 0x1704?
It's needed for my quickca

> * Turning LED on/off kills the device.  I have to re-plug the device to
>  make it work again.
>
> I could put those functions inside an if clause:
>
>        if (udev->descriptor.idProduct != 0x840)
>                do_something;
>
> and things work.  But as I do not have other cameras to test, I am not
> sure if this is the right way.  Do you have any suggestion?
>
> I will keep working on it.  But you can find a primitive patch and a
> sample image in the attachments.
>

Wow, thanks for the patch.
I'll review and probably commit it later today.

Best regards,
Erik

> --
> Regards,
> olv
>

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: Please test the gspca-stv06xx branch

[Chia-I Wu]

On Tue, Nov 25, 2008 at 12:22:02PM +0100, Erik Andrén wrote:
> > * stv06xx_write_sensor sends an extra packet unconditionally.  It causes
> >  the function call return error.
> Do you mean the extra packet to register 0x1704?
> It's needed for my quickca
Yes.  qc-usb sends an entra packet when GET_PRODUCTID(qc)==0x0850.  Does
your quickcam have product id 0x0850?
> Wow, thanks for the patch.
> I'll review and probably commit it later today.
The patch is primitive and I am planning some refactorings.  I will
submit again when it's ready.  Maybe you could help review it then :)

-- 
Regards,
olv

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: Please test the gspca-stv06xx branch

[Erik Andrén]

2008/11/25 Chia-I Wu <olvaffe@gmail.com>:
> On Tue, Nov 25, 2008 at 12:22:02PM +0100, Erik Andrén wrote:
>> > * stv06xx_write_sensor sends an extra packet unconditionally.  It causes
>> >  the function call return error.
>> Do you mean the extra packet to register 0x1704?
>> It's needed for my quickca
> Yes.  qc-usb sends an entra packet when GET_PRODUCTID(qc)==0x0850.  Does
> your quickcam have product id 0x0850?
>> Wow, thanks for the patch.
>> I'll review and probably commit it later today.
> The patch is primitive and I am planning some refactorings.  I will
> submit again when it's ready.  Maybe you could help review it then :)
>
Focus on the hdcs specific parts, I'll take care of that the LED and
extra packet only is used with the quickcam web.

Regards,
Erik

> --
> Regards,
> olv
>

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: Please test the gspca-stv06xx branch

[Chia-I Wu]

On Thu, Nov 27, 2008 at 11:40:06AM +0100, Hans de Goede wrote:
> Chia-I Wu, I'm afraid this might conflict with your HDCS work, as it is 
> against Erik's latest hg tree, so without your patches. I noticed you 
> were defining your own read/write register functions which really seems 
> the wrong thing todo, hopefully with my new functions you can use those 
> directly, or ?
IMO, it is almost always a good thing that each driver defines its own
wrapping reg read/write functions.  It is less confusing and saves
typings.  It makes the sub-driver loosely coupled with the main driver.
And, the compiler will do the right thing, and optimize them out if
appropriate.

-- 
Regards,
olv

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: Please test the gspca-stv06xx branch

[Erik Andrén]

2008/11/27 Chia-I Wu <olvaffe@gmail.com>:
> On Thu, Nov 27, 2008 at 11:40:06AM +0100, Hans de Goede wrote:
>> Chia-I Wu, I'm afraid this might conflict with your HDCS work, as it is
>> against Erik's latest hg tree, so without your patches. I noticed you
>> were defining your own read/write register functions which really seems
>> the wrong thing todo, hopefully with my new functions you can use those
>> directly, or ?
> IMO, it is almost always a good thing that each driver defines its own
> wrapping reg read/write functions.  It is less confusing and saves
> typings.  It makes the sub-driver loosely coupled with the main driver.
> And, the compiler will do the right thing, and optimize them out if
> appropriate.
>

I agree with Hans on this matter. It convolutes the driver and gives
no real gain.
I've just been converting the gspca-m5602 to use one set of read /
write functions instead of sensor specific ones and it removes a large
amount of code.

What the compiler does is one thing but when dealing with non
performance critical code paths, code simplicity is more important.

Best regards,
Erik

> --
> Regards,
> olv
>

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: Please test the gspca-stv06xx branch

[Chia-I Wu]

On Thu, Nov 27, 2008 at 12:55:21PM +0100, Erik Andrén wrote:
> 2008/11/27 Chia-I Wu <olvaffe@gmail.com>:
> > On Thu, Nov 27, 2008 at 11:40:06AM +0100, Hans de Goede wrote:
> >> Chia-I Wu, I'm afraid this might conflict with your HDCS work, as it is
> >> against Erik's latest hg tree, so without your patches. I noticed you
> >> were defining your own read/write register functions which really seems
> >> the wrong thing todo, hopefully with my new functions you can use those
> >> directly, or ?
> > IMO, it is almost always a good thing that each driver defines its own
> > wrapping reg read/write functions.  It is less confusing and saves
> > typings.  It makes the sub-driver loosely coupled with the main driver.
> > And, the compiler will do the right thing, and optimize them out if
> > appropriate.
> I agree with Hans on this matter. It convolutes the driver and gives
> no real gain.
> I've just been converting the gspca-m5602 to use one set of read /
> write functions instead of sensor specific ones and it removes a large
> amount of code.
> What the compiler does is one thing but when dealing with non
> performance critical code paths, code simplicity is more important.
It is the opposite.  What compiler does good to us is that, instead of
macros, one could define functions.

Other than hdcs_reg_write_seq, you may think the others as simple as,
for example,

#define hdcs_reg_write(hdcs, reg, val) \
	stv06xx_write_sensor_b(hdcs->sd, reg, val)

There should be no complication, and the significance here is that it
gives clear implication that there is no word write
(stv06xx_write_sensor_w) to this device, even though it is available in
the stv06xx.h.

IMO, there should be per-sensor I/O functions.  And the implementations
should be as simple as wrappers (i.e., macros) to the generic ones
provided by the bridge.  Sending exotic usb control messages is the job
of the bridge driver, not the sensor driver's.

The purpose for hdcs_reg_write_seq is a little bit trickier.  According
to the datasheet, HDCS family uses a serial protocol, instead of i2c, to
communicate with STV06xx.  When the first bit of HDCS_ICTRL is cleared,
it allows sequential writes: The beginning address is given once,
followed by a set of values.  The address will be automatically
incremented.  The prototype of hdcs_reg_write_seq models this hardware
characteristic, which might be something not shared by every sensor.

-- 
Regards,
olv

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: Please test the gspca-stv06xx branch

[Chia-I Wu]

On Thu, Nov 27, 2008 at 07:23:19PM +0100, Hans de Goede wrote:
> Exactly, so the functions belong in the bridge code, and if those 
> functions are properly written and prototyped, then they should be 
> directly usable from the sensor without requiring convoluting macros 
> around them. The only reason and acceptable use I see for using macro's 
> here would be to simplify the error handling as is currently done in the 
> other 2 sensor code files.
Each sensor could have a slightly different way to access the registers.
The bridge is responsible to provide means to ease the work needed in
the sensor drivers.

When 046d:0850 tries to write to its registers, it could

* have a wrapper which calls the helper function _AND_ write to reg
  0x1704 of the bridge (the extra packet).  Or,
* simply call the helper function.  The helper function checks the
  product id and send a second control message automatically.

I see the logics belong to the sensor driver and prefer the first
method, while you might see the second method easier to use.

My point is, a wrapper is not a big *NO*.  Instead, it is a good
practice, even if there is no sensor-specific logics involved.
> I greatly appreciate your efforts, but please try to constrain yourself 
> the programming model / pragma's followed through out gspca.
I would like to rebase the patch against current tip, or your changes
once they are merged.

-- 
Regards,
olv

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: Please test the gspca-stv06xx branch

[Erik Andrén]

2008/11/27 Hans de Goede <hdegoede@redhat.com>:
> Erik,
>
> While looking at the pb0100 code I noticed several issues. Attached is a
> patch fixing these. So far I've not gotten further then looking I'm afraid.
>
> The issues are:
>
> stv06xx_write_sensor_w() was calling stv06xx_write_sensor() with a count of
> 2, but that is not correct, there only is one address being passed and
> buf[0x21] should be set to 0, not to 1. Both indicate a count of 1, but
> there are 2 bytes of data being passed!
>
Thanks,
the stv06xx_write_sensor_w() and _b() were added on top as much of the
init is performed with separate function calls for each i2c write.
Later when all sensors are working I'm thinking of moving the data to
a table, this allows multiple i2c writes in the same packet.

> I've solved this be creating 2 separate stv06xx_write_sensor functions:
> typedef u8 u8_pair[2];
> typedef u16 u16_pair[2];
>
> int stv06xx_write_sensor_bytes(struct sd *sd, const u8_pair *data, int len);
> int stv06xx_write_sensor_words(struct sd *sd, const u16_pair *data, int
> len);
>
> These functions get passed one or more u8 / u16  pairs where pair[0] is an
> register-address and pair[1] is the value to write to that register, note
> that for stv06xx_write_sensor_words() the addresses are in reality still 8
> bits, they are gettings stored in an u16 for easier coding.

This is an alternate way of solving the same problem. I think this
approach is more convoluted without any real gain.
First you must multiplex the data and addresses by putting them
together and then demultiplex them in the function.
Not ideal but also not a big deal.

>
> The functions now also take care of putting multiple i2c register writes in
> one
> usb transaction and splitting over multiple if necessary, IMHO this belongs
> here and not in the sensor drivers.

Indeed.

>
> The other fix is that stv06xx_read_sensor_w was being passed an u8 pointer
> and then stored 2 bytes there, where as it was being called from pb0100.c
> with a buffer of only one u8. I've fixed this by making it take a pointer to
> an u16, which seems the right thing todo to me.

Whoops.

>
> While modifying stv06xx_vv6410.c to use the new methods I think I found a
> bug, both set flip functions are modifying VV6410_DATAFORMAT, but they are
> not doing read modify writes, so only the last one will stick, also they are
> masking a boolean, where they should set / clear the bit depending on the
> boolean being true or not.

Thanks for the catch.
>
> Note this patch is only compile tested.
>
> Chia-I Wu, I'm afraid this might conflict with your HDCS work, as it is
> against Erik's latest hg tree, so without your patches. I noticed you were
> defining your own read/write register functions which really seems the wrong
> thing todo, hopefully with my new functions you can use those directly, or ?
>
> Regards,
>
> Hans

Thanks for testing and the patch.
I'll review and commit ASAP.

Regards,
Erik

>
>
>
>
>
> Chia-I Wu wrote:
>>
>> Hi Erik,
>>
>> On Mon, Nov 24, 2008 at 10:00:17PM +0100, Erik Andrén wrote:
>>>
>>> I've reworked the driver somewhat and added initial support for th
>>> pb0100.
>>> Please test with the latest version of the gspca-stv06xx tree and
>>> see if you can get an image. Ekiga works best for me at the moment.
>>
>> I am trying to make gspca-stv06xx work with my QuickCam Express
>> (046d:0840).  It comes with the HDCS 1000 sensor.  So far, I am able to
>> receive frames using gstreamer (with libv4l).  The colors are wrong
>> though.
>>
>> While working on it, I encounter two minor issues:
>>
>> * stv06xx_write_sensor sends an extra packet unconditionally.  It causes
>>  the function call return error.
>> * Turning LED on/off kills the device.  I have to re-plug the device to
>>  make it work again.
>>
>> I could put those functions inside an if clause:
>>
>>        if (udev->descriptor.idProduct != 0x840)
>>                do_something;
>>
>> and things work.  But as I do not have other cameras to test, I am not
>> sure if this is the right way.  Do you have any suggestion?
>>
>> I will keep working on it.  But you can find a primitive patch and a
>> sample image in the attachments.
>>
>>
>>
>> ------------------------------------------------------------------------
>>
>
> diff -r 8b1b8968a794 linux/drivers/media/video/gspca/stv06xx/stv06xx.c
> --- a/linux/drivers/media/video/gspca/stv06xx/stv06xx.c Tue Nov 25 22:19:48
> 2008 +0100
> +++ b/linux/drivers/media/video/gspca/stv06xx/stv06xx.c Thu Nov 27 11:30:56
> 2008 +0100
> @@ -74,67 +74,94 @@
>
>  /* Wraps the normal write sensor function for those cases
>    when you just want to write a byte to a single register */
> -int stv06xx_write_sensor_b(struct sd *sd, u8 address, u8 i2c_data)
> +int stv06xx_write_sensor_b(struct sd *sd, u8 address, u8 value)
>  {
> -       return stv06xx_write_sensor(sd, &address, &i2c_data, 1);
> +       const u8 data[2] = { address, value };
> +       return stv06xx_write_sensor_bytes(sd, &data, 1);
>  }
>
>  /* Wraps the normal write sensor function with a 16 bit word */
> -int stv06xx_write_sensor_w(struct sd *sd, u8 address, u16 i2c_data)
> +int stv06xx_write_sensor_w(struct sd *sd, u8 address, u16 value)
>  {
> -       int err;
> -       u8 data[2];
> -
> -       data[0] = i2c_data & 0xff;
> -       data[1] = i2c_data >> 8;
> -
> -       err = stv06xx_write_sensor(sd, &address, data, 2);
> -
> -       return (err < 0) ? err : 0;
> +       const u16 data[2] = { address, value };
> +       return stv06xx_write_sensor_words(sd, &data, 1);
>  }
>
> -int stv06xx_write_sensor(struct sd *sd, u8 *address,
> -                        u8 *i2c_data, const u8 len)
> +static int stv06xx_write_sensor_finish(struct sd *sd)
>  {
> -       int err, i;
> +       int err = 0;
>        struct usb_device *udev = sd->gspca_dev.dev;
>        __u8 *buf = sd->gspca_dev.usb_buf;
>
> -       if (!len || len > I2C_MAX_COMMANDS)
> -               return -EINVAL;
> -
> -       PDEBUG(D_USBO, "I2C: Command buffer contains %d entries", len);
> -
> -       /* Build the command buffer */
> -       for (i = 0; i < len; i++) {
> -               buf[i] = address[i];
> -               buf[i + 0x10] = i2c_data[i];
> -               PDEBUG(D_USBO, "I2C: Writing 0x%x to address 0x%x",
> -                      buf[i + 0x10], buf[i]);
> -       }
> -
> -       buf[0x20] = sd->sensor->i2c_addr;
> -
> -       /* Number of commands to send - 1 */
> -       buf[0x21] = len - 1;
> -
> -       /* Write cmd */
> -       buf[0x22] = I2C_WRITE_CMD;
> -
> -       err = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
> -                             0x04, 0x40, 0x0400, 0, buf,
> -                             I2C_BUFFER_LENGTH,
> -                             STV06XX_URB_MSG_TIMEOUT);
> -
>        if (IS_850(sd)) {
>                /* Quickam Web needs an extra packet */
> -               buf[0] = 0;
> +               buf[0] = 0; /* Note original qc-usb had 1 here */
>                err = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
>                                      0x04, 0x40, 0x1704, 0, buf, 1,
>                                      STV06XX_URB_MSG_TIMEOUT);
>        }
> +       return err;
> +}
>
> -       return (err < 0) ? err : 0;
> +int stv06xx_write_sensor_bytes(struct sd *sd, const u8_pair *data, int len)
> +{
> +       int err, i, j;
> +       struct usb_device *udev = sd->gspca_dev.dev;
> +       __u8 *buf = sd->gspca_dev.usb_buf;
> +
> +       PDEBUG(D_USBO, "I2C: Command buffer contains %d entries", len);
> +
> +       for (i = 0; i < len; ) {
> +               /* Build the command buffer */
> +               memset(buf, 0, I2C_BUFFER_LENGTH);
> +               for (j = 0; j < I2C_MAX_BYTES && i < len; j++, i++) {
> +                       buf[j] = data[i][0];
> +                       buf[0x10 + j] = data[i][1];
> +                       PDEBUG(D_USBO, "I2C: Writing 0x%02x to reg 0x%02x",
> +                               data[i][1], data[i][0]);
> +               }
> +               buf[0x20] = sd->sensor->i2c_addr;
> +               buf[0x21] = j - 1; /* Number of commands to send - 1 */
> +               buf[0x22] = I2C_WRITE_CMD;
> +               err = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
> +                             0x04, 0x40, 0x0400, 0, buf,
> +                             I2C_BUFFER_LENGTH,
> +                             STV06XX_URB_MSG_TIMEOUT);
> +               if (err < 0)
> +                       return err;
> +       }
> +       return stv06xx_write_sensor_finish(sd);
> +}
> +
> +int stv06xx_write_sensor_words(struct sd *sd, const u16_pair *data, int
> len)
> +{
> +       int err, i, j;
> +       struct usb_device *udev = sd->gspca_dev.dev;
> +       __u8 *buf = sd->gspca_dev.usb_buf;
> +
> +       PDEBUG(D_USBO, "I2C: Command buffer contains %d entries", len);
> +
> +       for (i = 0; i < len; ) {
> +               /* Build the command buffer */
> +               memset(buf, 0, I2C_BUFFER_LENGTH);
> +               for (j = 0; j < I2C_MAX_WORDS && i < len; j++, i++) {
> +                       buf[j] = data[i][0];
> +                       buf[0x10 + j * 2] = data[i][1];
> +                       buf[0x10 + j * 2 + 1] = data[i][1] >> 8;
> +                       PDEBUG(D_USBO, "I2C: Writing 0x%04x to reg 0x%02x",
> +                               data[i][1], data[i][0]);
> +               }
> +               buf[0x20] = sd->sensor->i2c_addr;
> +               buf[0x21] = j - 1; /* Number of commands to send - 1 */
> +               buf[0x22] = I2C_WRITE_CMD;
> +               err = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
> +                             0x04, 0x40, 0x0400, 0, buf,
> +                             I2C_BUFFER_LENGTH,
> +                             STV06XX_URB_MSG_TIMEOUT);
> +               if (err < 0)
> +                       return err;
> +       }
> +       return stv06xx_write_sensor_finish(sd);
>  }
>
>  /* Wraps the normal read sensor function for those cases
> @@ -145,9 +172,15 @@
>  }
>
>  /* Wraps the normal read sensor function and returns a two byte data array
> */
> -int stv06xx_read_sensor_w(struct sd *sd, u8 address, u8 *data)
> +int stv06xx_read_sensor_w(struct sd *sd, u8 address, u16 *value)
>  {
> -       return stv06xx_read_sensor(sd, &address, data, 2);
> +       u8 data[2];
> +       int err;
> +
> +       err = stv06xx_read_sensor(sd, &address, data, 2);
> +       *value = data[0] | (data[1] << 8);
> +
> +       return err;
>  }
>
>  int stv06xx_read_sensor(struct sd *sd, const u8 *address,
> @@ -157,7 +190,7 @@
>        struct usb_device *udev = sd->gspca_dev.dev;
>        __u8 *buf = sd->gspca_dev.usb_buf;
>
> -       if (!len || len > I2C_MAX_COMMANDS)
> +       if (!len || len > I2C_MAX_BYTES)
>                return -EINVAL;
>
>        err = stv06xx_write_bridge(sd, STV_I2C_FLUSH, sd->sensor->i2c_flush);
> @@ -200,7 +233,7 @@
>        return (err < 0) ? err : 0;
>  }
>
> -/* Dumps all sensor registers */
> +/* Dumps all bridge registers */
>  static void stv06xx_dump_bridge(struct sd *sd)
>  {
>        int i;
> diff -r 8b1b8968a794 linux/drivers/media/video/gspca/stv06xx/stv06xx.h
> --- a/linux/drivers/media/video/gspca/stv06xx/stv06xx.h Tue Nov 25 22:19:48
> 2008 +0100
> +++ b/linux/drivers/media/video/gspca/stv06xx/stv06xx.h Thu Nov 27 11:30:56
> 2008 +0100
> @@ -71,7 +71,8 @@
>
>  #define STV06XX_URB_MSG_TIMEOUT                5000
>
> -#define I2C_MAX_COMMANDS               16
> +#define I2C_MAX_BYTES                  16
> +#define I2C_MAX_WORDS                  8
>
>  #define I2C_BUFFER_LENGTH              0x23
>  #define I2C_READ_CMD                   3
> @@ -91,16 +92,19 @@
>        struct sd_desc *desc;
>  };
>
> +typedef u8 u8_pair[2];
> +typedef u16 u16_pair[2];
> +
>  int stv06xx_write_bridge(struct sd *sd, u16 address, u16 i2c_data);
>  int stv06xx_read_bridge(struct sd *sd, u16 address, u8 *i2c_data);
>
> -int stv06xx_write_sensor_b(struct sd *sd, u8 address, u8 i2c_data);
> -int stv06xx_write_sensor_w(struct sd *sd, u8 address, u16 i2c_data);
> -int stv06xx_write_sensor(struct sd *sd, u8 *address,
> -                        u8 *i2c_data, const u8 len);
> +int stv06xx_write_sensor_b(struct sd *sd, u8 address, u8 value);
> +int stv06xx_write_sensor_w(struct sd *sd, u8 address, u16 value);
> +int stv06xx_write_sensor_bytes(struct sd *sd, const u8_pair *data, int
> len);
> +int stv06xx_write_sensor_words(struct sd *sd, const u16_pair *data, int
> len);
>
>  int stv06xx_read_sensor_b(struct sd *sd, u8 address, u8 *data);
> -int stv06xx_read_sensor_w(struct sd *sd, u8 address, u8 *data);
> +int stv06xx_read_sensor_w(struct sd *sd, u8 address, u16 *value);
>  int stv06xx_read_sensor(struct sd *sd, const u8 *address,
>                        u8 *i2c_data, const u8 len);
>
> diff -r 8b1b8968a794
> linux/drivers/media/video/gspca/stv06xx/stv06xx_pb0100.c
> --- a/linux/drivers/media/video/gspca/stv06xx/stv06xx_pb0100.c  Tue Nov 25
> 22:19:48 2008 +0100
> +++ b/linux/drivers/media/video/gspca/stv06xx/stv06xx_pb0100.c  Thu Nov 27
> 11:30:56 2008 +0100
> @@ -31,7 +31,7 @@
>
>  int pb0100_probe(struct sd *sd)
>  {
> -       u8 sensor;
> +       u16 sensor;
>        int err;
>
>        err = stv06xx_read_sensor_w(sd, PB_IDENT, &sensor);
> @@ -39,7 +39,7 @@
>        if (err < 0)
>                return -ENODEV;
>
> -       if (sensor == 0x64) {
> +       if ((sensor & 0xff) == 0x64) {
>                info("Photobit pb0100 sensor detected");
>
>                sd->gspca_dev.cam.cam_mode = stv06xx_sensor_pb0100.modes;
> diff -r 8b1b8968a794
> linux/drivers/media/video/gspca/stv06xx/stv06xx_vv6410.c
> --- a/linux/drivers/media/video/gspca/stv06xx/stv06xx_vv6410.c  Tue Nov 25
> 22:19:48 2008 +0100
> +++ b/linux/drivers/media/video/gspca/stv06xx/stv06xx_vv6410.c  Thu Nov 27
> 11:30:56 2008 +0100
> @@ -55,8 +55,7 @@
>
>  int vv6410_init(struct sd *sd)
>  {
> -       int err = 0, i, no_cmds = ARRAY_SIZE(vv6410_sensor_init);
> -       u8 buf_p = 0;
> +       int err = 0, i;
>
>        for (i = 0; i < ARRAY_SIZE(stv_bridge_init); i++) {
>                /* if NULL then len contains single value */
> @@ -74,34 +73,11 @@
>        }
>
>        if (err < 0)
> -               goto out;
> +               return err;
>
> -       PDEBUG(D_USBO, "%d commands to send", no_cmds);
> -       while (no_cmds) {
> -               u8 add_buf[I2C_MAX_COMMANDS], dat_buf[I2C_MAX_COMMANDS];
> -               int len = min(no_cmds, I2C_MAX_COMMANDS);
> -               PDEBUG(D_USBO, "Batch contains %d entries", len);
> +       err = stv06xx_write_sensor_bytes(sd, vv6410_sensor_init,
> +                                        ARRAY_SIZE(vv6410_sensor_init));
>
> -               /* Prepare the buffer */
> -               for (i = 0; i < len; i++) {
> -                       add_buf[i] = vv6410_sensor_init[buf_p + i][0];
> -                       dat_buf[i] = vv6410_sensor_init[buf_p + i][1];
> -                       PDEBUG(D_USBO, "I2C: Adding 0x%x to address 0x%x on"
> -                                      "buffer slot %d",
> -                                       dat_buf[i], add_buf[i], i);
> -               }
> -
> -               /* Write out the buffer */
> -               err = stv06xx_write_sensor(sd, add_buf, dat_buf, len);
> -
> -               if (err < 0)
> -                       goto out;
> -
> -               buf_p += len;
> -               no_cmds -= len;
> -       }
> -
> -out:
>        return (err < 0) ? err : 0;
>  }
>
> @@ -174,12 +150,10 @@
>  int vv6410_set_hflip(struct gspca_dev *gspca_dev, __s32 val)
>  {
>        int err;
> -       u8 i2c_data = val & VV6410_HFLIP;
> -       u8 address = VV6410_DATAFORMAT;
>        struct sd *sd = (struct sd *) gspca_dev;
>
>        PDEBUG(D_V4L2, "Set horizontal flip to %d", val);
> -       err = stv06xx_write_sensor(sd, &address, &i2c_data, 1);
> +       err = stv06xx_write_sensor_b(sd, VV6410_DATAFORMAT, val &
> VV6410_HFLIP);
>
>        return (err < 0) ? err : 0;
>  }
> @@ -202,12 +176,10 @@
>  int vv6410_set_vflip(struct gspca_dev *gspca_dev, __s32 val)
>  {
>        int err;
> -       u8 i2c_data = val & VV6410_VFLIP;
> -       u8 address = VV6410_DATAFORMAT;
>        struct sd *sd = (struct sd *) gspca_dev;
>
> -       PDEBUG(D_V4L2, "Set vertical flip to %d", i2c_data);
> -       err = stv06xx_write_sensor(sd, &address, &i2c_data, 1);
> +       PDEBUG(D_V4L2, "Set vertical flip to %d", val);
> +       err = stv06xx_write_sensor_b(sd, VV6410_DATAFORMAT, val &
> VV6410_VFLIP);
>
>        return (err < 0) ? err : 0;
>  }
> @@ -230,12 +202,10 @@
>  int vv6410_set_analog_gain(struct gspca_dev *gspca_dev, __s32 val)
>  {
>        int err;
> -       u8 i2c_data = 0xf0 | (val & 0xf);
> -       u8 address = VV6410_ANALOGGAIN;
>        struct sd *sd = (struct sd *) gspca_dev;
>
> -       PDEBUG(D_V4L2, "Set analog gain to %d", i2c_data);
> -       err = stv06xx_write_sensor(sd, &address, &i2c_data, 1);
> +       PDEBUG(D_V4L2, "Set analog gain to %d", val);
> +       err = stv06xx_write_sensor_b(sd, VV6410_ANALOGGAIN, 0xf0 | (val &
> 0xf));
>
>        return (err < 0) ? err : 0;
>  }
>
>

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: Please test the gspca-stv06xx branch

[Hans de Goede]

Erik Andrén wrote:
> 2008/11/27 Hans de Goede <hdegoede@redhat.com>:

<snip>

>> I've solved this be creating 2 separate stv06xx_write_sensor functions:
>> typedef u8 u8_pair[2];
>> typedef u16 u16_pair[2];
>>
>> int stv06xx_write_sensor_bytes(struct sd *sd, const u8_pair *data, int len);
>> int stv06xx_write_sensor_words(struct sd *sd, const u16_pair *data, int
>> len);
>>
>> These functions get passed one or more u8 / u16  pairs where pair[0] is an
>> register-address and pair[1] is the value to write to that register, note
>> that for stv06xx_write_sensor_words() the addresses are in reality still 8
>> bits, they are gettings stored in an u16 for easier coding.
> 
> This is an alternate way of solving the same problem. I think this
> approach is more convoluted without any real gain.
> First you must multiplex the data and addresses by putting them
> together and then demultiplex them in the function.
> Not ideal but also not a big deal.
> 


The gain here is that it is easy to define a single table with address, value 
pairs to init the sensor. Like you've done in stv06xx_vv6410.h, my patch now 
uses that table directly and unmodifed. Imagine what this table would look like 
if you had 2 separate tables for addresses and values, then the resulting table 
declaration_S_ in stv06xx_vv6410.h would make it very hard to see which value 
gets written to which register, so IMHO there is a real and big gain here.

<snip>

Regards,

Hans

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list