Hi, Please review the updates. 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 - 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 revert to use
software power down.
Also, this patch added hw status flag and save/restore ops between standby and
wakeup.
Signed-off-by: He, Yong <yong.m...@intel.com>
---
Index: a/drivers/staging/mrstci/mrstisp/mrstisp_main.c
===================================================================
--- a/drivers/staging/mrstci/mrstisp/mrstisp_main.c (revision 96)
+++ a/drivers/staging/mrstci/mrstisp/mrstisp_main.c (working copy)
@@ -1369,8 +1369,10 @@
DBG_entering;
for (i = 0 ; i <= isp->sensor_max; i++) {
- // power the sensor
- v4l2_subdev_call(isp->cameras[i].camera, core, s_gpio, 1);
+ /* power off all sensor if exist*/
+ if (isp->cameras[i].camera) {
+ v4l2_subdev_call(isp->cameras[i].camera, core, s_gpio, 1);
+ }
};
DBG_leaving;
@@ -2245,6 +2247,16 @@
return -EBUSY;
}
+ /* switch to a different sensor? */
+ if (isp->sensor_curr != i) {
+ /* suspend current sensor */
+ if ((isp->sensor_curr >=0 ) && (isp->sensor_curr <= isp->sensor_max)) {
+ if (isp->cameras[isp->sensor_curr].camera) {
+ v4l2_subdev_call(isp->cameras[isp->sensor_curr].camera, core,
s_gpio, 1);
+ }
+ }
+ }
+
isp->sensor_curr = i;
dprintk(0, "set sensor %s as input", camera->name);
Index: a/drivers/staging/mrstci/mrstov5640/ov5640.h
===================================================================
--- a/drivers/staging/mrstci/mrstov5640/ov5640.h (revision 96)
+++ a/drivers/staging/mrstci/mrstov5640/ov5640.h (working copy)
@@ -42,8 +42,6 @@
static struct regval_list ov5640_reg_reset[] = {
- {0x3103, 0x11},
- {0x3008, 0x82},
{0x3008, 0x42},
{0x3103, 0x03},
{0x3017, 0x00},
Index: a/drivers/staging/mrstci/mrstov5640/mrstov5640.c
===================================================================
--- a/drivers/staging/mrstci/mrstov5640/mrstov5640.c (revision 96)
+++ a/drivers/staging/mrstci/mrstov5640/mrstov5640.c (working copy)
@@ -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,33 @@
#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
+/*
+ * 6 -- VGA index in the array ov5640_res[], which is the default resolution
setting
+ */
+
+#define RESOLUTION_INDEX_VGA 6
+
static struct __ov5640_status_ {
+ /* lock */
+ spinlock_t state_lock;
+
+ /* sw status*/
int current_res_i;
int power;
int reg_inited;
+ int hflip,vflip;
+
} ov5640_status = {
- .current_res_i = 6,//SENSOR_RES_VGA;
- .power = OV5640_POWER_OFF,
- .reg_inited = OV5640_REG_INITED,
+ .state_lock = SPIN_LOCK_UNLOCKED,
+ .current_res_i = RESOLUTION_INDEX_VGA,
+ .power = OV5640_POWER_OFF,
+ .reg_inited = OV5640_REG_BEFORE_PROBE,
+ .hflip = 0,
+ .vflip = 1,
};
/*
@@ -312,62 +328,77 @@
static int ov5640_hw_init (struct i2c_client *c)
{
int ret;
+ unsigned long flags;
DBG_entering;
- if (ov5640_status.reg_inited == OV5640_REG_INITED) {
- return 0;
+ spin_lock_irqsave(&ov5640_status.state_lock, flags);
+
+ if (ov5640_status.reg_inited != OV5640_REG_RESET) {
+ spin_unlock_irqrestore(&ov5640_status.state_lock,flags);
+ ret = 0;
+ goto __ov5640_hw_init_exit;
}
- ret = ov5640_write(c, 0x3008, 0x82);
+ ov5640_status.reg_inited = OV5640_REG_INITED;
+ ov5640_status.current_res_i = RESOLUTION_INDEX_VGA ;
+ spin_unlock_irqrestore(&ov5640_status.state_lock,flags);
+
/* 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);
+ /* turn on AGC/AEC */
+ ret += ov5640_write(c, 0x3503, 0x0);
ov5640_set_data_pin_in(c);
- msleep(300);
- ov5640_status.reg_inited = OV5640_REG_INITED;
+__ov5640_hw_init_exit:
DBG_leaving;
return ret;
}
-
+static int ov5640_save_hw_status(struct i2c_client *c);
+static int ov5640_restore_hw_status(struct i2c_client *c);
static int ov5640_s_stream(struct v4l2_subdev *sd, int enable);
-
/*
* Sensor specific helper function
*/
-static int ov5640_standby(struct v4l2_subdev *sd)
+static void 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);
+ unsigned long flags;
+ struct i2c_client *c = v4l2_get_subdevdata(sd);
+
+ ov5640_save_hw_status(c);
+ ov5640_s_stream(sd , 0);
+ spin_lock_irqsave(&ov5640_status.state_lock, flags);
+
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;
+
+ spin_unlock_irqrestore(&ov5640_status.state_lock,flags);
dprintk(1, "PM: ov5640 standby called\n");
- return 0;
}
-static int ov5640_wakeup(struct v4l2_subdev *sd, int hw_init)
+static void ov5640_wakeup(struct v4l2_subdev *sd, int hw_reinit)
{
- struct i2c_client *c = v4l2_get_subdevdata(sd);
-
+ unsigned long flags;
dprintk(1, "PM: ov5640 wakeup called\n");
+ spin_lock_irqsave(&ov5640_status.state_lock, flags);
if (ov5640_status.power == OV5640_POWER_OFF) {
+ ov5640_status.power = OV5640_POWER_ON;
+ spin_unlock_irqrestore(&ov5640_status.state_lock,flags);
dprintk(1, "PM: ov5640 switch OFF to ON.\n");
gpio_set_value(GPIO_STDBY_PIN, 0);
- //if (hw_init) { ov5640_hw_init(c); }
- ov5640_status.power = OV5640_POWER_ON;
+ if (hw_reinit) {
+ struct i2c_client *c = v4l2_get_subdevdata(sd);
+ ov5640_hw_init(c);
+ ov5640_restore_hw_status(c);
+ }
+ } else {
+ spin_unlock_irqrestore(&ov5640_status.state_lock,flags);
}
- return 0;
}
static int ov5640_s_power(struct v4l2_subdev *sd, u32 val)
@@ -418,8 +449,11 @@
info->mipi_lanes = MIPI_LANES; /*MIPI lanes, 1 or 2*/
memcpy(info->name, "ov5640", 7);
- ov5640_status.reg_inited = OV5640_REG_RESET; // need do init reg here
+ 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;
@@ -584,6 +618,7 @@
static int ov5640_set_fmt(struct v4l2_subdev *sd,
struct v4l2_mbus_framefmt *fmt)
{
+ unsigned long flags;
struct i2c_client *client = v4l2_get_subdevdata(sd);
struct ci_sensor_config *info = to_sensor_config(sd);
int ret = 0;
@@ -606,25 +641,19 @@
uint8_t target_res_index;
target_res_index = find_OV5640_TIMING_index(res_index->res);
+ spin_lock_irqsave(&ov5640_status.state_lock, flags);
if (ov5640_status.current_res_i != target_res_index) {
- u8 REG20,REG21,REG20_new,REG21_new;
+ ov5640_status.current_res_i = target_res_index;
+ spin_unlock_irqrestore(&ov5640_status.state_lock,flags);
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);
+ ov5640_save_hw_status(client);
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_status.current_res_i = target_res_index;
+ ov5640_restore_hw_status(client);
}
else {
+ spin_unlock_irqrestore(&ov5640_status.state_lock,flags);
dprintk(3, "[ov5640-wr-reg] <----------- same res
index-%d (%dx%d)", target_res_index,width, height);
}
@@ -690,6 +719,7 @@
//info->bpat = SENSOR_BPAT_BGBGGRGR;
}
err += ov5640_write(client, 0x3821, v);
+ ov5640_status.hflip = value;
msleep(10); /* FIXME */
return err;
@@ -720,6 +750,7 @@
else
v &= ~0x06;
err += ov5640_write(client, 0x3820, v);
+ ov5640_status.vflip = value;
msleep(10); /* FIXME */
return err;
@@ -980,6 +1011,26 @@
return ret;
}
+static int ov5640_save_hw_status(struct i2c_client *c)
+{
+ int ret = 0;
+ /* flip status has already been saved in the ioctl, no need to save here*/
+ /* TODO: save original hw status*/
+
+ return ret;
+}
+
+static int ov5640_restore_hw_status(struct i2c_client *c)
+{
+ struct v4l2_subdev *sd = i2c_get_clientdata(c);
+ int ret = 0;
+
+ /* restore original flip status*/
+ ov5640_t_hflip(sd, ov5640_status.hflip);
+ ov5640_t_vflip(sd, ov5640_status.vflip);
+ return ret;
+}
+
static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
@@ -995,10 +1046,8 @@
ov5640_write(client, 0x3008, 0x02);
msleep(300);
}
- //ov5640_set_data_pin_out(client);
} else {
ov5640_write(client, 0x3008, 0x42);
- //ov5640_set_data_pin_in(client);
}
DBG_leaving;
@@ -1076,7 +1125,7 @@
if (ov5640_res[ov5640_status.current_res_i].used) {
for (fps_i = 0; fps_i < OV5640_N_FPS; fps_i++) {
if (target_fps ==
ov5640_res[ov5640_status.current_res_i].fps[fps_i]) {
- // found the target fps
+ /* found the target fps*/
u8 val;
dprintk( 1, "set sensor FPS to %d\n",
target_fps);
ov5640_read(client, 0x3008, &val);
Thanks,
-- Yong
Phone (+86) 21-61166334
Lab (+86) 21-61167881
-----Original Message-----
From: Kristen Carlson Accardi [mailto:kris...@linux.intel.com]
Sent: Tuesday, June 14, 2011 2:18 AM
To: He, Yong M
Cc: MeeGo-kernel@lists.meego.com; Van De Ven, Arjan; Accardi, Kristen C
Subject: Re: [Meego-kernel] [PATCH] MRST Tablet camera driver ver-0.942
(re-send), fix 9411
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 - 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
>
linux-2.6.37-camera-ov5640-ov9740-version-0.941_to_0.942.patch
Description: linux-2.6.37-camera-ov5640-ov9740-version-0.941_to_0.942.patch
_______________________________________________ MeeGo-kernel mailing list MeeGo-kernel@lists.meego.com http://lists.meego.com/listinfo/meego-kernel
