-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi, and thanks for the patch!
I've added some more comments below. ~ Lukáš Karas wrote: | Ok, thanks for comments. | New patch is in attachment (patch comparet to branche m5602, revision 372). | | ______________________________________________________________ |> Od: [EMAIL PROTECTED] |> Komu: Lukáš Karas <[EMAIL PROTECTED]> |> CC: m560x-driver-devel <[email protected]> |> Datum: 06.09.2008 17:42 |> Předmět: Re: [M560x-driver-devel] m5602/s5k83a brightness,whiteness and gain control |> | | | ~ Luká? Karas wrote: | | Hi Eric. | | | | There is my today edit for s5k83a sensor: | http://www.karry.wz.cz/download/m5602.20080906.zip | | I was edited m5602 branch. Now is possible controll brightness, | whiteness and gain. | | | | I identify sensor register (0x18), that drasticaly extend expose time. | With maximal value is fps about 3! | | In this case getting good images in dark. You can see at: | http://www.karry.wz.cz/download/m5602-night_mode.png | | I named this properties as GAIN. It is right? I dont find apposites | properties on http://v4l2spec.bytesex.org/spec/x542.htm ... | | | | At end, I have small question. What you use as camera viewer? I find | neither viewer, that support change all parameters | | and work good with m5602... | | | | Regards Karry | | | Hi again. | This is a diff against the current driver. See my comments inline. | Please address them and send me the updated driver as a patch. | | | --- | /home/erik/Utveckling/m560x-driver/m560x/branches/m5602/m5602_s5k83a.c | 2008-09-06 16:59:02.000000000 +0200 | +++ m5602_s5k83a.c 2008-09-06 13:07:53.000000000 +0200 | @@ -18,6 +18,11 @@ | | ~ #include "m5602_s5k83a.h" | | +static int brightness = S5K83A_DEFAULT_BRIGHTNESS; | +static int whiteness = S5K83A_DEFAULT_WHITENESS; | +static int gain = S5K83A_DEFAULT_GAIN; | + | | Do not use static integers in the code. It's not pretty and can cause | race conditions. Also why shadow the registers in the sensor with local | variables? | | |> Sorry, I dont know how make it better. I try read register values instead |> shadows his, but read function return me different values compare with |> written in some cases! |> Especiali with whitness, what is write with sequence three registers. |> It is hard when we have any datasheet. I hardly belive they've spread out the different bits in an illogical way. There must be some systems. We need to figure out how to read the correct registers and interpret the results in a deterministic and consistent way, otherwise having these controls is pointless. | |> Maybe for pretty code create new structure with actual controls... | | + | ~ static int s5k83a_probe(struct m5602_camera *cam) | ~ { | ~ u8 prod_id = 0, ver_id = 0; | @@ -159,6 +164,10 @@ | ~ return -EINVAL; | ~ } | ~ } | + | + s5k83a_set_brightness(cam,brightness); | + s5k83a_set_whiteness(cam, whiteness); | + s5k83a_set_gain(cam, gain); | | This is ok I guess, although I would prefer if you altered the init code | in the m5602_s5k83a.h instead. | |> Good idea. I do it. Thanks | | ~ if (dump_sensor) | ~ s5k83a_dump_registers(cam); | @@ -173,13 +182,102 @@ | | ~ static int s5k83a_get_ctrl(struct m5602_camera *cam, struct | v4l2_control *ctrl) | ~ { | - return 0; | + switch (ctrl->id) { | + case V4L2_CID_BRIGHTNESS: | + ctrl->value = brightness; | + PDEBUG(DBG_V4L2, "get brightness (0x%x)", ctrl->value ); | + return 0; | | Probe the actual sensor registers instead of copying the shadowed variable. | | |> I explayin top, why I do this way. | | + case V4L2_CID_WHITENESS: | + ctrl->value = whiteness; | + PDEBUG(DBG_V4L2, "get whiteness (0x%x)", ctrl->value ); | | Same here. | | + return 0; | + case V4L2_CID_GAIN: | + ctrl->value = gain; | + PDEBUG(DBG_V4L2, "get gain (0x%x)", ctrl->value ); | + return 0; | | And here | + default: | + PDEBUG(DBG_V4L2, "s5k83a_get_ctrl 0x%x", ctrl->id); | | This should warn that the selected ioctl isn't supported. | | |> OK | | + } | + return EINVAL; | ~ } | | | | ~ static int s5k83a_set_ctrl(struct m5602_camera *cam, | ~ const struct v4l2_control *ctrl) | ~ { | - return 0; | + switch (ctrl->id) { | + case V4L2_CID_BRIGHTNESS: | + brightness = ctrl->value; | + s5k83a_set_brightness( cam, ctrl->value ); | + PDEBUG(DBG_V4L2, "set brightness to 0x%x", ctrl->value ); | + return 0; | | Please remove the spaces around the brackets and replace the return with | a break. | | |> ok | | + case V4L2_CID_WHITENESS: | + whiteness = ctrl->value; | + s5k83a_set_whiteness(cam, ctrl->value); | + PDEBUG(DBG_V4L2, "set whiteness to 0x%x", ctrl->value ); | + return 0; | | Same here. | | + case V4L2_CID_GAIN: | + gain = ctrl->value; | + s5k83a_set_gain(cam, ctrl->value); | + PDEBUG(DBG_V4L2, "set whiteness to 0x%x", ctrl->value ); | + return 0; | | Same here. | + default: | + PDEBUG(DBG_V4L2, "s5k83a_set_ctrl 0x%x to value 0x%x", | ctrl->id, ctrl->value); | + } | + return EINVAL; | | Check for errors and set the return value properly. | |> ok | | +} | + | + | +static void s5k83a_set_brightness(struct m5602_camera *cam, u16 value) | +{ | + int err = 0; | + u8 data[2]; | + | + data[0] = 0x00; | + data[1] = 0x20; | + err = s5k83a_write_sensor(cam, 0x14, data, 2); | Is this necessary? What does it do? | | |> Yes. It is necessary. I extract this sequence from snoop with |> windows driver. Without write 0x14 and 0x0d registers |> before 0x1b has strange results. This is interesting, I wonder why? Could there be some kind of sync-signal? | | + | + data[0] = 0x01; | + data[1] = (value & 0xff); // part of expose time (i dont know | what is this...) | | If you don't know what this is why do you write to it? | |> Value of second byte change fractionally brightness. |> This change is little with compare 0x1b register. |> I set this value to 0x00 Expose all possible resolution, do not limit it in the driver. | | + err = s5k83a_write_sensor(cam, 0x0d, data, 2); | + | + data[0] = (value ) >> 3; // brightness, high 5 bits | + data[1] = (value ) >> 2; // brightness, high 6 bits (range is | 7bits, but with the highest bit is unusable) | This doesn't make any sense. Do you really need to write almost the same | value to the two bytes? What happens if you only write to one byte? | | |> So. This values are partly experimented. Both bytes change brightness. |> If use both, range is greater. What is the dependency between the registers? Could one be some kind of multiplier? | |> Can you see video with this? Don't understand what you mean here. | | + err = s5k83a_write_sensor(cam, 0x1b, data, 2); | + | +} | + | +static void s5k83a_set_whiteness(struct m5602_camera *cam, u8 value) | +{ | + int tmp, err = 0; | + u8 data[2]; | + tmp = value; | + | + | + data[0] = value; | + data[1] = 0x00; | + err = s5k83a_write_sensor(cam, 0x0a, data, 1); | + | If you only write the first byte, don't set the next at all. | | |> ok | | + | + /* | + // this code is for playing :-D | + info("set to value 0x%x", tmp); | + | + data[0] = tmp; | + data[1] = tmp; | + err = s5k83a_write_sensor(cam, 0xe8, data, 2); | + */ | Remove this comment. | | |> ok | | +} | + | +static void s5k83a_set_gain(struct m5602_camera *cam, u8 value) | Don't return void but the error value generated by the | s5k83a_write_sensor() command. | | +{ | + // values near 0x3c is greath for nigh mode | Remove this comment. | | |> ok | | + int err = 0; | + u8 data[2]; | + | + data[0] = 0; | + data[1] = value; | + err = s5k83a_write_sensor(cam, 0x18, data, 2); | | Is it only the high byte that alters the gain? | | |> Yes exactly. First byte change nothing. Second byte change "gain" only from 0x00 to 0x3c... | | + | ~ } | | ~ static void s5k83a_dump_registers(struct m5602_camera *cam) | | Thanks, | Erik | | | |> Please write me how solve problem with shadows registers/ controls | |> Karry Again, nice improvements. Thanks, Erik | | | | | | | | | | - ------------------------------------------------------------------------- | | This SF.Net email is sponsored by the Moblin Your Move Developer's | challenge | | Build the coolest Linux based applications with Moblin SDK & win great | prizes | | Grand prize is a trip for two to an Open Source event anywhere in the | world | | http://moblin-contest.org/redirect.php?banner_id=100&url=/ | | _______________________________________________ | | M560x-driver-devel mailing list | | [email protected] | | https://lists.sourceforge.net/lists/listinfo/m560x-driver-devel | | |> -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFIw9xYN7qBt+4UG0ERApQRAJ9aKa9tieSCwvZfP+yYnLEfmVY0wwCfYkUJ zGL+HhZ6/E6kPQjnXZ31Tt0= =eOvz -----END PGP SIGNATURE----- ------------------------------------------------------------------------- This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100&url=/ _______________________________________________ M560x-driver-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/m560x-driver-devel
