Please test the gspca-stv06xx branch
-----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:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
Hi Erik,
Quoted 4 lines · Erik Andrén
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.
Signature · 2 lines
Regards, olv
linux/drivers/media/video/gspca/stv06xx/stv06xx.c +16 -10
@@ -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 );
linux/drivers/media/video/gspca/stv06xx/stv06xx_hdcs.c +127 -3
152 lines not on this page · raw message
linux/drivers/media/video/gspca/stv06xx/stv06xx_hdcs.h +2 -0
@@ -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,
--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
Quoted 4 lines · Erik Andrén
Quoted 2 lines
*
stv06xx_write_sensorsends 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?
Quoted 2 lines
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 :)
Signature · 7 lines
Regards, olv -- video4linux-list mailing list Unsubscribe mailto:[email protected]?subject=unsubscribe https://www.redhat.com/mailman/listinfo/video4linux-list
2008/11/25 Chia-I Wu <[email protected]>:
Quoted 12 lines · Erik Andrén
Quoted 4 lines
Quoted 2 lines
*
stv06xx_write_sensorsends 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 quickcaYes. qc-usb sends an entra packet when GET_PRODUCTID(qc)==0x0850. Does your quickcam have product id
0x0850?Quoted 2 lines
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
Quoted 4 lines
--
Regards,
olv
--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
Hi,
2008/11/25 Chia-I Wu <[email protected]>:
Quoted 12 lines
Hi Erik,
Quoted 4 lines · Erik Andrén
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.
Quoted 4 lines
While working on it, I encounter two minor issues:
*
stv06xx_write_sensorsends 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
Quoted 5 lines
* 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:
Quoted 9 lines
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
Quoted 4 lines
--
Regards,
olv
--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
Quoted 5 lines · Hans de Goede
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.
Signature · 7 lines
Regards, olv -- video4linux-list mailing list Unsubscribe mailto:[email protected]?subject=unsubscribe https://www.redhat.com/mailman/listinfo/video4linux-list
2008/11/27 Hans de Goede <[email protected]>:
Quoted 5 lines
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.
Quoted 7 lines
The issues are:
stv06xx_write_sensor_w()was callingstv06xx_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.
Quoted 4 lines
I've solved this be creating 2 separate
stv06xx_write_sensorfunctions:
typedef u8 u8_pair[2];
typedef u16 u16_pair[2];
Quoted 8 lines
int
stv06xx_write_sensor_bytes(struct sd *sd, const u8_pair *data, int len); intstv06xx_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.
Quoted 5 lines
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.
Quoted 5 lines
The other fix is that
stv06xx_read_sensor_wwas being passed an u8 pointer and then stored 2 bytes there, where as it was being called frompb0100.cwith 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.
Quoted 6 lines
While modifying
stv06xx_vv6410.cto use the new methods I think I found a bug, both set flip functions are modifyingVV6410_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.
Quoted 11 lines
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
Quoted 391 lines
Quoted 30 lines · Chia-I Wu
Hi Erik,
Quoted 5 lines · Erik Andrén
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_sensorsends 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?
Quoted 7 lines
I will keep working on it. But you can find a primitive patch and a sample image in the attachments.
------------------------------------------------------------------------
diff -r
8b1b8968a794linux/drivers/media/video/gspca/stv06xx/stv06xx.c---a/linux/drivers/media/video/gspca/stv06xx/stv06xx.cTue Nov 25 22:19:48 2008 +0100
+++b/linux/drivers/media/video/gspca/stv06xx/stv06xx.cThu Nov 27 11:30:56 2008 +0100(quoted diff) +70
-43@@ -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
(quoted diff) +8
-2@@ -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,
linux/drivers/media/video/gspca/stv06xx/stv06xx_hdcs.c +2
-2@@ -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
8b1b8968a794linux/drivers/media/video/gspca/stv06xx/stv06xx.h---a/linux/drivers/media/video/gspca/stv06xx/stv06xx.hTue Nov 25 22:19:48 2008 +0100
+++b/linux/drivers/media/video/gspca/stv06xx/stv06xx.hThu Nov 27 11:30:56 2008 +0100(quoted diff) +10
-6@@ -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
8b1b8968a794linux/drivers/media/video/gspca/stv06xx/stv06xx_pb0100.c
---a/linux/drivers/media/video/gspca/stv06xx/stv06xx_pb0100.cTue Nov 25 22:19:48 2008 +0100
+++b/linux/drivers/media/video/gspca/stv06xx/stv06xx_pb0100.cThu Nov 27 11:30:56 2008 +0100(quoted diff) +2
-2@@ -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
8b1b8968a794linux/drivers/media/video/gspca/stv06xx/stv06xx_vv6410.c
---a/linux/drivers/media/video/gspca/stv06xx/stv06xx_vv6410.cTue Nov 25 22:19:48 2008 +0100
+++b/linux/drivers/media/video/gspca/stv06xx/stv06xx_vv6410.cThu Nov 27 11:30:56 2008 +0100(quoted diff) +5
-31@@ -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;}
(quoted diff) +2
-4@@ -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;}
(quoted diff) +2
-4@@ -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;
Quoted 3 lines
}
--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
2008/11/27 Chia-I Wu <[email protected]>:
Quoted 6 lines · Hans de Goede
Quoted 5 lines
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 ?
Quoted 6 lines
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
Quoted 4 lines
--
Regards,
olv
--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
Quoted 12 lines · Erik Andrén
2008/11/27 Chia-I Wu <[email protected]>:
Quoted 11 lines · Hans de Goede
Quoted 5 lines
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.
Quoted 7 lines
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.
Signature · 7 lines
Regards, olv -- video4linux-list mailing list Unsubscribe mailto:[email protected]?subject=unsubscribe https://www.redhat.com/mailman/listinfo/video4linux-list
Quoted 1 line · Erik Andrén
2008/11/27 Hans de Goede <[email protected]>:
<snip>
Quoted 13 lines
Quoted 4 lines
I've solved this be creating 2 separate
stv06xx_write_sensorfunctions:
typedef u8 u8_pair[2];
typedef u16 u16_pair[2];Quoted 8 lines
int
stv06xx_write_sensor_bytes(struct sd *sd, const u8_pair *data, int len); intstv06xx_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.
Quoted 6 lines
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:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
Quoted 6 lines · Hans de Goede
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.
Quoted 2 lines
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.
Signature · 7 lines
Regards, olv -- video4linux-list mailing list Unsubscribe mailto:[email protected]?subject=unsubscribe https://www.redhat.com/mailman/listinfo/video4linux-list