Hi Eric. After fix read_sensor function as in S5K4AA read always same value that was before writen. So, shadow values is no more needed... New patch included in attachment (over 375 rev.).
> > > > >Grégory Lardière wrote: >| Hi, >| >| We also had problems when reading S5K4AA registers, maybe the same fix >will work for S5K83A, take a look at revision 322. >| Concerning exposure time, it seems controlled by a 16bits value >(registers 0x17-0x18) on sensor S5K4AA, it might also be the case for >S5K83A... >| I also found (with help from Arnaud) that both sensors share the same >page map register 0xec, so there is not only 256, but virtually 16*256 >registers to try ;-) >| >| Greg >| >| > >Direct link to the changeset for that revision: > >http://m560x-driver.svn.sourceforge.net/viewvc/m560x-driver/m560x/branches/m5602/m5602_s5k4aa.c?r1=315&r2=322&pathrev=322 > > > >| >| ----- Message d'origine ---- >| De : Erik Andrén <[EMAIL PROTECTED]> >| À : Lukáš Karas <[EMAIL PROTECTED]> >| Cc : m560x-driver-devel <[email protected]> >| Envoyé le : Dimanche, 7 Septembre 2008, 15h51mn 20s >| Objet : Re: [M560x-driver-devel] m5602/s5k83a brightness, whiteness >and gain control >| >| Hi, >| and thanks for the patch! >| >| I've added some more comments below. >| >| ~ Lukáa Karas wrote: >| | Ok, thanks for comments. >| | New patch is in attachment (patch comparet to branche m5602, revision >| 372). >| | >| | ______________________________________________________________ >| |> Od: [EMAIL PROTECTED] >| |> Komu: Lukáa Karas <[EMAIL PROTECTED]> >| |> CC: m560x-driver-devel <[email protected]> >| |> Datum: 06.09.2008 17:42 >| |> PYedmt: 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. >| fix with new sensor read function >| | >| |> 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? >| If this bytes use separately, efect is approximately same. If use both, brightness range is greater... I dont know more. As I say, I write these function experimentally. But its work and this is general, or not? :) >| | >| |> 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 >| | | >| |> >| > Regards Karry
m5602.patch
Description: Binary data
------------------------------------------------------------------------- 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
