[Meego-kernel] [patch] mrstov5640: use linux debug print mechanism
He, Yong M
yong.m.he at intel.com
Sat Jun 18 08:36:26 PDT 2011
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:arjan at linux.intel.com]
> Sent: Saturday, June 18, 2011 12:12 AM
> To: He, Yong M
> Cc: meego-kernel at 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 <arjan at 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 <arjan at 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.he at 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;
>
>
More information about the MeeGo-kernel
mailing list