Hi Arjan, Thanks for your suggestion, I'll work out a patch to replace all dprintk(). Before that, I need to work on the 3 left (of total 5) critical camera patches, the middle-ware team are waiting for them to be integrated. Is that OK?
BTW, How can I set different debug levels using pr_debug and dev_dbg()? Thanks, -- Yong Phone (+86) 21-61166334 Lab (+86) 21-61167881 > > -----Original Message----- > From: Arjan van de Ven [mailto:ar...@linux.intel.com] > Sent: Saturday, June 18, 2011 12:12 AM > To: He, Yong M > Cc: meego-ker...@meego.com; Kristen Carlson Accardi > Subject: [patch] mrstov5640: use linux debug print mechanism > > Hi Yong, > > one of the things that has been bugging me a bit lately is how much printk > spew the camera drivers give in dmesg; it's not a huge deal, just a minor > irritation, but it made me look into the source code. > > the patch below tries to improve the camera debug functionality by using the > linux kernel standard debug mechanism (pr_debug and dev_dbg() ), which allows > for runtime tuning of the print/debug levels and also has the benefit of > working automatically well with diagnostic tools (because of a standard > leading format header). > > Please review the patch and consider integrating it into your source code; I > think it makes the driver better and more of a linux driver. > > > > From: Arjan van de Ven <ar...@linux.intel.com> > Subject: mrstov5640: use linux debug print mechanism > > The ov5640 driver invented its own dprintk() abstraction rather than > using the standard linux pr_debug/dev_dbg > mechanism. This patch switches the driver to the standard mechanism, > which allows the debugging to be used > by automatic analysis and diagnostic tools. > > Signed-off-by: Arjan van de Ven <ar...@linux.intel.com> > > > diff --git a/drivers/staging/mrstci/mrstov5640/mrstov5640.c > b/drivers/staging/mrstci/mrstov5640/mrstov5640.c > index 375f609..b27f94e 100644 > --- a/drivers/staging/mrstci/mrstov5640/mrstov5640.c > +++ b/drivers/staging/mrstci/mrstov5640/mrstov5640.c > @@ -24,6 +24,8 @@ > * Yong He <yong.m...@intel.com> > */ > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > #include <linux/module.h> > #include <linux/types.h> > #include <linux/kernel.h> > @@ -50,10 +52,6 @@ > > #define DRIVER_VERSION "0.942" > > -static int mrstov5640_debug=5; > -module_param(mrstov5640_debug, int, 0644); > -MODULE_PARM_DESC(mrstov5640_debug, "Debug level (0-1)"); > - > #define GPIO_FLASH 45 > static int ov5640_flash=0; > > @@ -61,19 +59,8 @@ static int ov5640_flash=0; > #define V4L2_CID_FLASH_STROBE > (V4L2_CID_CAMERA_CLASS_BASE+17) > #endif > > -#define dprintk(level, fmt, arg...) do { \ > - if (mrstov5640_debug >= level) \ > - printk(KERN_DEBUG "ov5640@%s: " fmt "\n", \ > - __func__, ## arg); } \ > - while (0) > - > -#define eprintk(fmt, arg...) \ > - printk(KERN_ERR "ov5640@%s: line %d: " fmt "\n", \ > - __func__, __LINE__, ## arg); > - > -#define DBG_entering dprintk(2, "entering"); > -#define DBG_leaving dprintk(2, "leaving"); > -#define DBG_line dprintk(2, " line: %d", __LINE__); > +#define DBG_entering pr_debug("entering %s\n", __func__); > +#define DBG_leaving pr_debug("leaving %s\n", __func__); > > static inline struct ci_sensor_config *to_sensor_config(struct > v4l2_subdev *sd) > { > @@ -244,7 +231,7 @@ static int ov5640_read(struct i2c_client *c, u16 > reg, u8 *value) > *value = ret_val; > > ret = (ret == 2) ? 0 : -1; > - dprintk( 4, " && ov5640_read \t\t\t(0x%04x) ------> > 0x%02x",reg,*value); > + dev_dbg(&c->adapter->dev, "ov5640_read \t\t\t(0x%04x) ------> > 0x%02x\n", reg, *value); > return ret; > } > > @@ -272,7 +259,7 @@ static int ov5640_write(struct i2c_client *c, u16 > reg, u8 value) > if (reg == 0x3008 && (value & 0x80)) > msleep(3); > ret = (ret == 1) ? 0 : -1; > - dprintk( 4, "&& ov5640_write \t\t\t(0x%04x) <------ 0x%02x",reg,value); > + dev_dbg(&c->adapter->dev, "ov5640_write \t\t\t(0x%04x) <------ > 0x%02x\n", reg, value); > return ret; > } > > @@ -346,7 +333,7 @@ static int ov5640_hw_init (struct i2c_client *c) > > /* Set registers into default config value */ > ret = ov5640_write_array(c, ov5640_def_reg); > - dprintk( 1, "[ov5640-wr-reg] <----------- load ov5640 default > settings\n"); > + dev_dbg(&c->adapter->dev, "[ov5640-wr-reg] <----------- load ov5640 > default settings\n"); > /* turn on AGC/AEC */ > ret += ov5640_write(c, 0x3503, 0x0); > ov5640_set_data_pin_in(c); > @@ -377,22 +364,23 @@ static void ov5640_standby(struct v4l2_subdev *sd) > ov5640_status.reg_inited = OV5640_REG_RESET; > > spin_unlock_irqrestore(&ov5640_status.state_lock, flags); > - dprintk(1, "PM: ov5640 standby called\n"); > + dev_dbg(&c->adapter->dev, "PM: ov5640 standby called\n"); > } > > static void ov5640_wakeup(struct v4l2_subdev *sd, int hw_reinit) > { > unsigned long flags; > - dprintk(1, "PM: ov5640 wakeup called\n"); > + struct i2c_client *c = v4l2_get_subdevdata(sd); > + > + dev_dbg(&c->adapter->dev, "PM: ov5640 wakeup called\n"); We cannot use 'struct i2c_client *c' here, because when wakeup are firstly called, this c is not inited yet. I think here we should use printk() directly to print out this important message. > 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"); > + dev_dbg(&c->adapter->dev, "PM: ov5640 switch OFF to ON.\n"); > gpio_set_value(GPIO_STDBY_PIN, 0); > if (hw_reinit) { > - struct i2c_client *c = v4l2_get_subdevdata(sd); > ov5640_hw_init(c); > ov5640_restore_hw_status(c); > } > @@ -476,7 +464,7 @@ static int ov5640_try_res(u32 *w, u32 *h) > struct ov5640_res_struct *res_index, *p = NULL; > int dis, last_dis = ov5640_res->width + ov5640_res->height; > > - dprintk(1, "&&&&& before %dx%d", *w, *h); > + pr_debug("&&&&& before %dx%d", *w, *h); > for (res_index = ov5640_res; > res_index < ov5640_res + N_RES; > res_index++) { > @@ -509,7 +497,7 @@ static int ov5640_try_res(u32 *w, u32 *h) > *w = res_index->width; > *h = res_index->height; > > - dprintk(1, "&&&&& after %dx%d", *w, *h); > + pr_debug("&&&&& after %dx%d", *w, *h); > return 0; > } > > @@ -533,11 +521,13 @@ static int ov5640_try_fmt(struct v4l2_subdev *sd, > struct v4l2_mbus_framefmt *fmt) > { > int ret = 0; > + struct i2c_client *c = v4l2_get_subdevdata(sd); > + > DBG_entering; > - dprintk(1, "&&&&& before %dx%d", fmt->width, > + dev_dbg(&c->adapter->dev, "&&&&& before %dx%d", fmt->width, > fmt->height); > ret = ov5640_try_res(&fmt->width, &fmt->height); > - dprintk(1, "&&&&& after %dx%d", fmt->width, > + dev_dbg(&c->adapter->dev, "&&&&& after %dx%d", fmt->width, > fmt->height); > DBG_leaving; > return ret; > @@ -648,8 +638,8 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd, > 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"); > + dev_dbg(&client->adapter->dev, "[ov5640-wr-reg] > <----------- changing res from index-%d to index-%d (%dx%d)", > ov5640_status.current_res_i, target_res_index, width, height); > + dev_dbg(&client->adapter->dev, "[ov5640-wr-reg] > <----------- Set resolution Configs"); > ov5640_save_hw_status(client); > ret += ov5640_write_array(client, res_index->regs); > ov5640_restore_hw_status(client); > @@ -657,7 +647,7 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd, > 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); > + dev_dbg(&client->adapter->dev, "[ov5640-wr-reg] > <----------- same res index-%d (%dx%d)", target_res_index, width, height); > } > > /* Marked current sensor res as being "used" */ > @@ -671,7 +661,7 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd, > } > > for (index = 0; index < N_RES; index++) > - dprintk(2, "index = %d, used = %d\n", index, > + dev_dbg(&client->adapter->dev, "index = %d, used = %d\n", > index, > ov5640_res[index].used); > > } > @@ -1042,13 +1032,13 @@ static int ov5640_s_stream(struct v4l2_subdev > *sd, int enable) > struct i2c_client *client = v4l2_get_subdevdata(sd); > DBG_entering; > > - dprintk( 1, "[ov5640-wr-reg] <----------- ov5640: s_stream %d > \n",enable); > + dev_dbg(&client->adapter->dev, "[ov5640-wr-reg] <----------- > ov5640: s_stream %d\n", enable); > > if (enable) { > unsigned char v; > ov5640_read(client, 0x3008, &v); > if ((v & 0xff) != 0x02) { > - dprintk( 1, "[ov5640-wr-reg] <----------- set stream on, > sleep 300 ms for sensor adjusting...\n"); > + dev_dbg(&client->adapter->dev, "[ov5640-wr-reg] > <----------- set stream on, sleep 300 ms for sensor adjusting...\n"); > ov5640_write(client, 0x3008, 0x02); > msleep(300); > } > @@ -1085,11 +1075,14 @@ static int ov5640_enum_frameintervals(struct > v4l2_subdev *sd, > { > unsigned int index = fival->index; > unsigned int res_index; > + struct i2c_client *client = v4l2_get_subdevdata(sd); > + > + > > DBG_entering; > > res_index = find_OV5640_res_index(fival->width, fival->height); > - dprintk( 1, "find res index %d (%dx%d)\n",res_index,fival->width, > fival->height); > + dev_dbg(&client->adapter->dev, "find res index %d (%dx%d)\n", > res_index, fival->width, fival->height); > > if (res_index >= N_RES) > return -EINVAL; > @@ -1112,7 +1105,7 @@ static int ov5640_enum_frameintervals(struct > v4l2_subdev *sd, > fival->type = V4L2_FRMIVAL_TYPE_DISCRETE; > fival->discrete.numerator = 1; > fival->discrete.denominator = ov5640_res[res_index].fps[index]; > - dprintk( 1, "find FPS index-%d > (%d)\n",index,fival->discrete.denominator); > + dev_dbg(&client->adapter->dev, "find FPS index-%d (%d)\n", index, > fival->discrete.denominator); > > DBG_leaving; > > @@ -1133,7 +1126,7 @@ static int ov5640_s_parm(struct v4l2_subdev *sd, > struct v4l2_streamparm *param){ > if (target_fps == > ov5640_res[ov5640_status.current_res_i].fps[fps_i]) { > /* found the target fps*/ > u8 val; > - dprintk( 1, "set sensor FPS to %d\n", target_fps); > + dev_dbg(&client->adapter->dev, "set sensor FPS to > %d\n", target_fps); > ov5640_read(client, 0x3008, &val); > if (val == 0x02) ov5640_s_stream(sd, 0); > > @@ -1146,12 +1139,12 @@ static int ov5640_s_parm(struct v4l2_subdev *sd, > struct v4l2_streamparm *param){ > } > } > if (fps_i >= OV5640_N_FPS) { > - dprintk( 1, "Warning!! target FPS %d/%d is not found in > sensor supported table.\n", > + dev_dbg(&client->adapter->dev, "Warning!! target FPS %d/%d > is not found in sensor supported table.\n", > param->parm.capture.timeperframe.denominator, > param->parm.capture.timeperframe.numerator); > return -EINVAL; > } > } else { > - dprintk( 1, "Warning!! current_res (%d) is not used??\n", > + dev_dbg(&client->adapter->dev, "Warning!! current_res (%d) is > not used??\n", > ov5640_status.current_res_i); > } > > @@ -1189,7 +1182,7 @@ static int ov5640_s_register(struct v4l2_subdev *sd, > { > struct i2c_client *client = v4l2_get_subdevdata(sd); > > - dprintk( 1, "[ov5640-wr-reg] <----------- ov5640_s_register 0x%04x > (0x%02x)\n", > + dev_dbg(&client->adapter->dev, "[ov5640-wr-reg] <----------- > ov5640_s_register 0x%04x (0x%02x)\n", > (unsigned int)(reg->reg & 0xffff),(unsigned int)(reg->val > & 0xff)); > ov5640_write(client, reg->reg & 0xffff, reg->val & 0xff); > return 0; > > _______________________________________________ MeeGo-kernel mailing list MeeGo-kernel@lists.meego.com http://lists.meego.com/listinfo/meego-kernel
