On Mon, 13 Jun 2011 11:20:46 +0800 "He, Yong M" <yong.m...@intel.com> wrote:
> > From: Yong He <yong.m...@intel.com<mailto:yong.m...@intel.com>> > Subject: [PATCH] MRST Tablet camera driver ver-0.942 (re-send), fix 9411 > > Bug 9411<https://bugzilla.otcshare.org/show_bug.cgi?id=9411> - power > regression test failed for camera driver version 0.941 > this is caused by the back sensor HW power down GPIO-49, when perform a HW > power down using this GPIO, somewhere else is leaking, > So I use software power down instead for now. will investigate this later. > Also, this patch added hw status flag and save/restore ops between standby > and wakeup. > > Signed-off-by: He, Yong <yong.m...@intel.com<mailto:yong.m...@intel.com>> > --- > Index: a/drivers/staging/mrstci/mrstov5640/ov5640.h > =================================================================== > --- a/drivers/staging/mrstci/mrstov5640/ov5640.h (revision 89) > +++ a/drivers/staging/mrstci/mrstov5640/ov5640.h (revision 90) > @@ -42,8 +42,8 @@ > > static struct regval_list ov5640_reg_reset[] = { > - {0x3103, 0x11}, > - {0x3008, 0x82}, > +// {0x3103, 0x11}, > +// {0x3008, 0x82}, I would prefer if you would just delete rather than comment out. Also, please read Documentation/CodingStyle: "Linux style for comments is the C89 "/* ... */" style. Don't use C99-style "// ..." comments. " > {0x3008, 0x42}, > {0x3103, 0x03}, > {0x3017, 0x00}, > Index: a/drivers/staging/mrstci/mrstov5640/mrstov5640.c > =================================================================== > --- a/drivers/staging/mrstci/mrstov5640/mrstov5640.c (revision 89) > +++ a/drivers/staging/mrstci/mrstov5640/mrstov5640.c (revision 90) > @@ -48,7 +48,7 @@ > #include "ci_sensor_common.h" > #include "ov5640.h" > -#define DRIVER_VERSION "0.941" > +#define DRIVER_VERSION "0.942" > static int mrstov5640_debug=5; > module_param(mrstov5640_debug, int, 0644); > @@ -183,17 +183,26 @@ > #define OV5640_POWER_OFF 0 > #define OV5640_POWER_ON 1 > -#define OV5640_REG_RESET 0 > +#define OV5640_REG_RESET 2 > #define OV5640_REG_INITED 1 > +#define OV5640_REG_BEFORE_PROBE 0 > static struct __ov5640_status_ { > + // sw status > int current_res_i; > int power; > int reg_inited; > + > + // hw status > + u8 REG20; > + u8 REG21; > + > } ov5640_status = { > .current_res_i = 6,//SENSOR_RES_VGA; > .power = OV5640_POWER_OFF, > - .reg_inited = OV5640_REG_INITED, > + .reg_inited = OV5640_REG_BEFORE_PROBE, > + .REG20 = 0, > + .REG21 = 0, > }; Does this global struct need locking? > /* > @@ -314,56 +323,76 @@ > int ret; > DBG_entering; > - if (ov5640_status.reg_inited == OV5640_REG_INITED) { > + if (ov5640_status.reg_inited != OV5640_REG_RESET) { > return 0; > } > - ret = ov5640_write(c, 0x3008, 0x82); > /* Set registers into default config value */ > - ret += ov5640_write_array(c, ov5640_def_reg); > + ret = ov5640_write_array(c, ov5640_def_reg); > dprintk( 1, "[ov5640-wr-reg] <----------- load ov5640 default > settings\n"); > // turn on AGC/AEC > - ov5640_write(c, 0x3503, 0x0); > - ov5640_status.current_res_i = 6 ; > - > - /* added by wen to stop sensor from streaming */ > - ov5640_write(c, 0x3008, 0x42); > + ret += ov5640_write(c, 0x3503, 0x0); > ov5640_set_data_pin_in(c); > - msleep(300); > ov5640_status.reg_inited = OV5640_REG_INITED; > + ov5640_status.current_res_i = 6 ; If you are going to use magic values everywhere, can you at least put comments saying what they are doing? Also, please don't add unnecessary whitespace. > DBG_leaving; > return ret; > } > +static int ov5640_save_hw_status(struct i2c_client *c) > +{ > + // save original flip status > + ov5640_read(c,0x3820,&ov5640_status.REG20); > + ov5640_read(c,0x3821,&ov5640_status.REG21); > + return 0; > +} Why define this as returning int when the only possible return value is zero, and you never even check it? > +static int ov5640_restore_hw_status(struct i2c_client *c) > +{ > + u8 REG20_new,REG21_new; > + // restore original flip status > + ov5640_read(c,0x3820,®20_new); > + ov5640_read(c,0x3821,®21_new); > + ov5640_write(c,0x3820,((REG20_new & 0xf9) | (ov5640_status.REG20& 0x6))); > + ov5640_write(c,0x3821,((REG21_new & 0xf9) | (ov5640_status.REG21& 0x6))); > + return 0; > +} > + ditto. > static int ov5640_s_stream(struct v4l2_subdev *sd, int enable); > - > /* > * Sensor specific helper function > */ > static int ov5640_standby(struct v4l2_subdev *sd) > { > - // set software power down instead. > - ov5640_s_stream(sd, 0); > - // set hw power down > //gpio_set_value(GPIO_STDBY_PIN, 1); > + ov5640_s_stream(sd , 0); > ov5640_status.power = OV5640_POWER_OFF; > - //ov5640_status.reg_inited = OV5640_REG_RESET; > + > + if (ov5640_status.reg_inited == OV5640_REG_INITED) > + { > + ov5640_status.reg_inited = OV5640_REG_RESET; > + } > + Please read Documentation/CodingStyle: " Do not unnecessarily use braces where a single statement will do. if (condition) action(); " > dprintk(1, "PM: ov5640 standby called\n"); > return 0; > } > -static int ov5640_wakeup(struct v4l2_subdev *sd, int hw_init) > +static int ov5640_wakeup(struct v4l2_subdev *sd, int hw_reinit) > { > - struct i2c_client *c = v4l2_get_subdevdata(sd); > + dprintk(1, "PM: ov5640 wakeup called (%d)\n",hw_reinit); > - dprintk(1, "PM: ov5640 wakeup called\n"); > - > - if (ov5640_status.power == OV5640_POWER_OFF) { > + if (ov5640_status.power == OV5640_POWER_OFF) > + { And here you've done nothing except add changes where they weren't needed. Again, refer to Documentation/CodingStyle: "The other issue that always comes up in C styling is the placement of braces. Unlike the indent size, there are few technical reasons to choose one placement strategy over the other, but the preferred way, as shown to us by the prophets Kernighan and Ritchie, is to put the opening brace last on the line, and put the closing brace first, thusly: if (x is true) { we do y } " > dprintk(1, "PM: ov5640 switch OFF to ON.\n"); > gpio_set_value(GPIO_STDBY_PIN, 0); > - //if (hw_init) { ov5640_hw_init(c); } > + if (hw_reinit) > + { > + struct i2c_client *c = v4l2_get_subdevdata(sd); > + ov5640_save_hw_status(c); > + ov5640_hw_init(c); > + ov5640_restore_hw_status(c); > + } > ov5640_status.power = OV5640_POWER_ON; > } > @@ -419,7 +448,10 @@ > memcpy(info->name, "ov5640", 7); > ov5640_status.reg_inited = OV5640_REG_RESET; // need do init reg here > + ret = ov5640_write(c, 0x3013, 0x11); > + ret = ov5640_write(c, 0x3008, 0x82); > ret = ov5640_hw_init(c); > + msleep(300); > DBG_leaving; > @@ -608,19 +640,12 @@ > target_res_index = find_OV5640_TIMING_index(res_index->res); > if (ov5640_status.current_res_i != target_res_index) { > - u8 REG20,REG21,REG20_new,REG21_new; > dprintk( 3, "[ov5640-wr-reg] <----------- changing > res from index-%d to index-%d (%dx%d)", ov5640_status.current_res_i, > target_res_index,width, height); > dprintk( 3, "[ov5640-wr-reg] <----------- Set > resolutiojn Configs"); > - // save original flip status > - ov5640_read(client,0x3820,®20); > - ov5640_read(client,0x3821,®21); > - ret += ov5640_write_array(client, > res_index->regs); > - // restore original flip status > - ov5640_read(client,0x3820,®20_new); > - ov5640_read(client,0x3821,®21_new); > - ov5640_write(client,0x3820,((REG20_new & 0xf9) | > (REG20& 0x6))); > - ov5640_write(client,0x3821,((REG21_new & 0xf9) | > (REG21& 0x6))); > + ov5640_save_hw_status(client); > + ret += ov5640_write_array(client, res_index->regs); > + ov5640_restore_hw_status(client); > ov5640_status.current_res_i = target_res_index; > } > > Best Regards, > > Yong He > Software Engineer, PCSD SW Solutions, > Intel Asia-Pacific R&D Ltd. > Phone (+86) 21-61166334 > Lab (+86) 21-61167881 > _______________________________________________ MeeGo-kernel mailing list MeeGo-kernel@lists.meego.com http://lists.meego.com/listinfo/meego-kernel
