2009/1/13 "Dr. Bernhard Neubüser" <[email protected]>: > Dear Erik Andrén, > > I own a Fujitsu Siemens Amilo Xi 2550 running Ubuntu 8.10 (64bit) and I > would like to help improve this subdriver for v4l2 for this laptop. In > particular I identified some problems, see below. > I read the complete README.patches file which accompanies the v4l2 source > code. I installed mercurial related packages from the Ubuntu repositories > but do not really know in what way to use the installed software. I was also > not sure how to follow the instructions in item 7 of the README: shoud I > really send this e-mail instead to [email protected]? Mailing me is fine as I'm the maintainer of the driver. I'm adding the specific devel list as a cc to this mail in order to preserve this conversation.
> I hope that you > accept this kind of input and forward it the correct way. Please also let me > know if I should have acted differently. This way is perfectly fine. > I would also be grateful if you > could give advise regarding a development environment which is convenient > for further developments of the driver. I would like to investigate the > relevance of other registers and add a high resolution mode (1280*960, or > whatever the maximum is) to the driver capabilities. There was some work made by Grégory Lardière in acheiving this. http://osdir.com/ml/linux.drivers.m560x.devel/2008-08/msg00007.html > Is there any further documentation on the sensor or the bridge chip? I have written some rudimentary notes on how I belive the m5602 bridge behaves at http://m560x-driver.svn.sourceforge.net/viewvc/m560x-driver/m560x/Documentation/m5602.txt?revision=231&view=markup There are no public datasheet available for the Samsung s5k4aa sensor. I've tried to request it from without success. > > I had some trouble with the WEB cam support in that I did not get a usable > image: it was almost completely black. During investigation I identified the > following little mistakes in the files m5602_s5k4aa.h and m5602_s5k4aa.c: > > m5602_s5k4aa.h: > 1. The last time rowstart/columnstart are set rowstart shoud be set to an > odd value otherwise the Bayer-pattern is read with a shift (the image is > mostly grey with slight tints of color instead of normally colored; using > pixfmt-test I verified that indeed interpreting the Bayerpattern in a > different order yields a correct color image). > > {SENSOR, S5K4AA_ROWSTART_HI, 0x00, 0x00}, > {SENSOR, S5K4AA_ROWSTART_LO, 0x29, 0x00}, /* BN was 2a */ > > > I was not able to understand the comment > /* ROWSTART_HI, ROWSTART_LO : 10 + (1024-960)/2 = 42 = 0x002a */ > Where does the "10" come from? Beats me, I probably just tried to come up with a sane interpretation of the value. > > Trying out other odd values also give acceptable results, however slightly > shifted images. I am not sure whether my suggested patch might create > problems for users with other laptop with the same chip, especially those > where the chip is mounted 180 degrees turned ( cf. end of function > s5k4aa_init in m5602_s5k4aa.c). > > > 2. I also changed the values with which gain and exposure are initialized > because the original values yield images that are so dark that under normal > artificial illumination NOTHING can be seen. I would propose the following > values: > I played around with GLOBAL_GAIN and could not detect any effect. My > proposal would therefore be that it is not set at all. > > {SENSOR, S5K4AA_EXPOSURE_HI, 0x06, 0x00}, /* BN was 01,00 */ > {SENSOR, S5K4AA_EXPOSURE_LO, 0x3f, 0x00}, /*BN was 00,00 */ > {SENSOR, 0x11, 0x04, 0x00}, > {SENSOR, 0x12, 0xc3, 0x00}, > {SENSOR, S5K4AA_PAGE_MAP, 0x02, 0x00}, > {SENSOR, 0x02, 0x0e, 0x00}, > /* {SENSOR_LONG, S5K4AA_GLOBAL_GAIN__, 0xff, 0xff}, */ /*BN was 0f,00 */ > {SENSOR, S5K4AA_GAIN_1, 0x10, 0x00}, /* BN was 0b,00*/ > {SENSOR, S5K4AA_GAIN_2, 0x5f, 0x00} /* BN was a0,00*/ > Ok, lets remove global gain and adjust the default values. > Regarding m5602_s5k4aa.c: > 3. The functions to set/get vflip/hflip each contain a very small error > resulting however in messing up the driver to such an extend that it has to > be deactivated and reactivated again (using FN-F7 key combination (in case > of the amilo Xi 2550). The errors namely are that in > > a) s5k4aa_get_vflip > > it reads just before the jump label "out": > err = s5k4aa_read_sensor(sd, S5K4AA_PAGE_MAP, &data, 1); > *val = (data & S5K4AA_RM_V_FLIP) >> 7; > PDEBUG(D_V4L2, "Read vertical flip %d", *val); > > > Correct is: > > err = s5k4aa_read_sensor(sd, S5K4AA_READ_MODE, &data, 1); /*BN read from > READ_MODE, not PAGE_MAP */ > *val = (data & S5K4AA_RM_V_FLIP) >> 7; > PDEBUG(D_V4L2, "Read vertical flip %d", *val); > > b) The identical same error is present in s5k4aa_get_hflip > > c) In s5k4aa_set_vflip it reads: > int s5k4aa_set_vflip(struct gspca_dev *gspca_dev, __s32 val) > { > struct sd *sd = (struct sd *) gspca_dev; > u8 data = S5K4AA_PAGE_MAP_2; > int err; > > PDEBUG(D_V4L2, "Set vertical flip to %d", val); > err = s5k4aa_write_sensor(sd, S5K4AA_PAGE_MAP, &data, 1); > if (err < 0) > goto out; > err = s5k4aa_write_sensor(sd, S5K4AA_READ_MODE, &data, 1); > if (err < 0) > goto out; > data = ((data & ~S5K4AA_RM_V_FLIP) > | ((val & 0x01) << 7)); > err = s5k4aa_write_sensor(sd, S5K4AA_READ_MODE, &data, 1); > if (err < 0) > goto out; > > if (val) { > err = s5k4aa_read_sensor(sd, S5K4AA_ROWSTART_LO, &data, 1); > if (err < 0) > goto out; > > data++; > err = s5k4aa_write_sensor(sd, S5K4AA_ROWSTART_LO, &data, 1); > } else { > err = s5k4aa_read_sensor(sd, S5K4AA_ROWSTART_LO, &data, 1); > if (err < 0) > goto out; > > data--; > err = s5k4aa_write_sensor(sd, S5K4AA_ROWSTART_LO, &data, 1); > } > out: > return (err < 0) ? err : 0; > } > > Correct is: > int s5k4aa_set_vflip(struct gspca_dev *gspca_dev, __s32 val) > { > struct sd *sd = (struct sd *) gspca_dev; > u8 data = S5K4AA_PAGE_MAP_2; > int err; > > PDEBUG(D_V4L2, "Set vertical flip to %d", val); > err = s5k4aa_write_sensor(sd, S5K4AA_PAGE_MAP, &data, 1); > if (err < 0) > goto out; > err = s5k4aa_read_sensor(sd, S5K4AA_READ_MODE, &data, 1); /*BN read, not > write */ > if (err < 0) > goto out; > data = ((data & ~S5K4AA_RM_V_FLIP) > | ((val & 0x01) << 7)); > err = s5k4aa_write_sensor(sd, S5K4AA_READ_MODE, &data, 1); > if (err < 0) > goto out; > > if (val) { > err = s5k4aa_read_sensor(sd, S5K4AA_ROWSTART_LO, &data, 1); > if (err < 0) > goto out; > > data++; > err = s5k4aa_write_sensor(sd, S5K4AA_ROWSTART_LO, &data, 1); > } else { > err = s5k4aa_read_sensor(sd, S5K4AA_ROWSTART_LO, &data, 1); > if (err < 0) > goto out; > > data--; > err = s5k4aa_write_sensor(sd, S5K4AA_ROWSTART_LO, &data, 1); > } > out: > return (err < 0) ? err : 0; > } > > d) The same error exists in ..._hflip. Fixed. > > 4. To me it appears that register 0x37 of map 02 turn noise reduction > (temporal or spatial averaging) on/off. I would suggest to include it as a > further (private control of the driver, e.g by > > int s5k4aa_get_noise(struct gspca_dev *gspca_dev, __s32 *val) > { > struct sd *sd = (struct sd *) gspca_dev; > u8 data = S5K4AA_PAGE_MAP_2; > int err; > > err = s5k4aa_write_sensor(sd, S5K4AA_PAGE_MAP, &data, 1); > if (err < 0) > goto out; > > err = s5k4aa_read_sensor(sd, 0x37, &data, 1); > *val = data & 0x01; > PDEBUG(D_V4L2, "Read noise suppression %d", *val); > > out: > return (err < 0) ? err : 0; > } > > int s5k4aa_set_noise(struct gspca_dev *gspca_dev, __s32 val) > { > struct sd *sd = (struct sd *) gspca_dev; > u8 data = S5K4AA_PAGE_MAP_2; > int err; > > PDEBUG(D_V4L2, "Set gain to %d", val); > err = s5k4aa_write_sensor(sd, S5K4AA_PAGE_MAP, &data, 1); > if (err < 0) > goto out; > > data = val & 0x01; > err = s5k4aa_write_sensor(sd, 0x37, &data, 1); > > out: > return (err < 0) ? err : 0; > } > > > > > > and by adding to > > static struct m5602_sensor s5k4aa = { .... > > the entry > > > { > .id = V4L2_CID_PRIVATE_BASE, > .type = V4L2_CTRL_TYPE_BOOLEAN, > .name = "Noise suppression (smoothing)", > .minimum = 0, > .maximum = 1, > .step = 1, > .default_value = 1, > }, > .set = s5k4aa_set_noise, > .get = s5k4aa_get_noise > }, > > > 5. I would like to propose the following replacement of the function > dumpregisters. Compared to the original version it differs in that > - it prints several register values together in one line; above each line > it prints whether the register is read/write or read-only. > A typical output look like the following: > > Dumping the s5k4aa register state for page 0x0 > rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw| > 0|10| 0|4b|33|75| 1|ff| 3|ff| 3|ff|68|28| 0| > 6|10|88|40|30|30|30|30|30|30|30|30|20|10|10|10|20| > Address offset 20 > rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw|rw| > 30| 7|22| 0|c2| 0| 4| 0| 1| 4| 0| d| 0|6a| 0| 0| 0| 0| 1|70| c| 5| 7| 5| 4| > 6| 7| 9|18|10|30| 0| > > > > This more compact representation allows it more easily to store dumps and > compare them. > > The code to replace the original version is: > > void s5k4aa_dump_registers(struct sd *sd) > { > #define LINE_SIZE 0x20 //Number of register infos printed in one line > char values[3*LINE_SIZE+1]=""; > char writable[3*LINE_SIZE+1]=""; > char output[4]; > int line_counter=0; > int address; > u8 page, old_page; > s5k4aa_read_sensor(sd, S5K4AA_PAGE_MAP, &old_page, 1); > > for (page = 0; page < 16; page++) { > s5k4aa_write_sensor(sd, S5K4AA_PAGE_MAP, &page, 1); > info("Dumping the s5k4aa register state for page 0x%2.2x", page); > for (address = 0; address <= 0xff; address++) { > u8 old_value, ctrl_value, test_value = 0xff; > > s5k4aa_read_sensor(sd, address, &old_value, 1); > > sprintf(output,"%2x|",old_value); > strcat(values,output); > > test_value = ~old_value; > s5k4aa_write_sensor(sd, address, &test_value, 1); > s5k4aa_read_sensor(sd, address, &ctrl_value, 1); > /* Restore original value */ > s5k4aa_write_sensor(sd, address, &old_value, 1); > > if (ctrl_value == test_value) > strcat(writable,"rw|"); > else > strcat(writable,"ro|"); > > if ( (++line_counter) == LINE_SIZE) { > line_counter = 0; > info("%s",writable); > info("%s",values); > if (address < 0xff) info("Address offset %x",address+1); > writable[0] = 0; > values[0] = 0; > } > > } > } > info("s5k4aa register state dump complete"); > s5k4aa_write_sensor(sd, S5K4AA_PAGE_MAP, &old_page, 1); > } > Great idea. I'll try to add it asap. There is no datasheet for the s5k4aa all values have been reversed engineered. I also don't have this hardware making all commits made by me error prone as I have no way of verifying the functionality. Thanks for your input. Best regards, Erik > > > > Regards, > > Bernhard Neubüser > ------------------------------------------------------------------------------ This SF.net email is sponsored by: SourcForge Community SourceForge wants to tell your story. http://p.sf.net/sfu/sf-spreadtheword _______________________________________________ M560x-driver-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/m560x-driver-devel
