[PATCH] Replaced hard coded function names in debug messages with __func__ macro.

[PATCH] Replaced hard coded function names in debug messages with __func__ macro.

[Tabot Kevin]

This patch fixes the following:
- Uses __func__ macro to print function names.
- Got rid of unnecessary braces around single line if statements.
- End of block comments on a seperate line.
- A spelling mistake of the word "on".

Signed-off-by: Tabot Kevin <tabot.kevin@gmail.com>
---
 drivers/staging/media/atomisp/i2c/atomisp-ov2680.c | 25 +++++++++++-----------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index c907305..1396a33 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -146,7 +146,7 @@ static int ov2680_g_bin_factor_x(struct v4l2_subdev *sd, s32 *val)
 	struct ov2680_device *dev = to_ov2680_sensor(sd);
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 
-	dev_dbg(&client->dev,  "++++ov2680_g_bin_factor_x\n");
+	dev_dbg(&client->dev,  "++++%s\n", __func__);
 	*val = ov2680_res[dev->fmt_idx].bin_factor_x;
 
 	return 0;
@@ -158,7 +158,7 @@ static int ov2680_g_bin_factor_y(struct v4l2_subdev *sd, s32 *val)
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 
 	*val = ov2680_res[dev->fmt_idx].bin_factor_y;
-	dev_dbg(&client->dev,  "++++ov2680_g_bin_factor_y\n");
+	dev_dbg(&client->dev,  "++++%s\n", __func__);
 	return 0;
 }
 
@@ -173,7 +173,7 @@ static int ov2680_get_intg_factor(struct i2c_client *client,
 	u16 reg_val;
 	int ret;
 
-	dev_dbg(&client->dev,  "++++ov2680_get_intg_factor\n");
+	dev_dbg(&client->dev,  "++++%s\n", __func__);
 	if (!info)
 		return -EINVAL;
 
@@ -251,8 +251,8 @@ static long __ov2680_set_exposure(struct v4l2_subdev *sd, int coarse_itg,
 	int ret, exp_val;
 
 	dev_dbg(&client->dev,
-		"+++++++__ov2680_set_exposure coarse_itg %d, gain %d, digitgain %d++\n",
-		coarse_itg, gain, digitgain);
+		"+++++++%s coarse_itg %d, gain %d, digitgain %d++\n",
+		__func__, coarse_itg, gain, digitgain);
 
 	vts = ov2680_res[dev->fmt_idx].lines_per_frame;
 
@@ -461,11 +461,11 @@ static int ov2680_v_flip(struct v4l2_subdev *sd, s32 value)
 	ret = ov2680_read_reg(client, 1, OV2680_FLIP_REG, &val);
 	if (ret)
 		return ret;
-	if (value) {
+	if (value)
 		val |= OV2680_FLIP_MIRROR_BIT_ENABLE;
-	} else {
+	else
 		val &= ~OV2680_FLIP_MIRROR_BIT_ENABLE;
-	}
+
 	ret = ov2680_write_reg(client, 1,
 			       OV2680_FLIP_REG, val);
 	if (ret)
@@ -731,7 +731,8 @@ static int gpio_ctrl(struct v4l2_subdev *sd, bool flag)
 	 * existing integrations often wire two (reset/power_down)
 	 * because that is the way other sensors work.  There is no
 	 * way to tell how it is wired internally, so existing
-	 * firmwares expose both and we drive them symmetrically. */
+	 * firmwares expose both and we drive them symmetrically.
+	 */
 	if (flag) {
 		ret = dev->platform_data->gpio0_ctrl(sd, 1);
 		usleep_range(10000, 15000);
@@ -1060,9 +1061,9 @@ static int ov2680_s_stream(struct v4l2_subdev *sd, int enable)
 
 	mutex_lock(&dev->input_lock);
 	if (enable)
-		dev_dbg(&client->dev, "ov2680_s_stream one\n");
+		dev_dbg(&client->dev, "%s on\n", __func__);
 	else
-		dev_dbg(&client->dev, "ov2680_s_stream off\n");
+		dev_dbg(&client->dev, "%s off\n", __func__);
 
 	ret = ov2680_write_reg(client, 1, OV2680_SW_STREAM,
 			       enable ? OV2680_START_STREAMING :
@@ -1226,7 +1227,7 @@ static int ov2680_remove(struct i2c_client *client)
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
 	struct ov2680_device *dev = to_ov2680_sensor(sd);
 
-	dev_dbg(&client->dev, "ov2680_remove...\n");
+	dev_dbg(&client->dev, "%s...\n", __func__);
 
 	dev->platform_data->csi_cfg(sd, 0);
 
-- 
2.7.4

Re: [PATCH] Replaced hard coded function names in debug messages with __func__ macro.

[Daniel Thompson]

On Sat, Oct 31, 2020 at 05:41:03PM +0100, Tabot Kevin wrote:
> This patch fixes the following:
> - Uses __func__ macro to print function names.
> - Got rid of unnecessary braces around single line if statements.
> - End of block comments on a seperate line.
> - A spelling mistake of the word "on".
> 
> Signed-off-by: Tabot Kevin <tabot.kevin@gmail.com>
> ---
>  drivers/staging/media/atomisp/i2c/atomisp-ov2680.c | 25 +++++++++++-----------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> index c907305..1396a33 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> @@ -146,7 +146,7 @@ static int ov2680_g_bin_factor_x(struct v4l2_subdev *sd, s32 *val)
>  	struct ov2680_device *dev = to_ov2680_sensor(sd);
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  
> -	dev_dbg(&client->dev,  "++++ov2680_g_bin_factor_x\n");
> +	dev_dbg(&client->dev,  "++++%s\n", __func__);

It might be better just to remove this sort of message.

They are not "wrong wrong" but are they actually useful one a
driver's basic functions work? Even where they are useful
dynamic techniques (ftrace, tracepoints, etc) arguably provide a
better way to support "did my function actually run" debug
approaches anyway.


Daniel.


>  	*val = ov2680_res[dev->fmt_idx].bin_factor_x;
>  
>  	return 0;
> @@ -158,7 +158,7 @@ static int ov2680_g_bin_factor_y(struct v4l2_subdev *sd, s32 *val)
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  
>  	*val = ov2680_res[dev->fmt_idx].bin_factor_y;
> -	dev_dbg(&client->dev,  "++++ov2680_g_bin_factor_y\n");
> +	dev_dbg(&client->dev,  "++++%s\n", __func__);
>  	return 0;
>  }
>  
> @@ -173,7 +173,7 @@ static int ov2680_get_intg_factor(struct i2c_client *client,
>  	u16 reg_val;
>  	int ret;
>  
> -	dev_dbg(&client->dev,  "++++ov2680_get_intg_factor\n");
> +	dev_dbg(&client->dev,  "++++%s\n", __func__);
>  	if (!info)
>  		return -EINVAL;
>  
> @@ -251,8 +251,8 @@ static long __ov2680_set_exposure(struct v4l2_subdev *sd, int coarse_itg,
>  	int ret, exp_val;
>  
>  	dev_dbg(&client->dev,
> -		"+++++++__ov2680_set_exposure coarse_itg %d, gain %d, digitgain %d++\n",
> -		coarse_itg, gain, digitgain);
> +		"+++++++%s coarse_itg %d, gain %d, digitgain %d++\n",
> +		__func__, coarse_itg, gain, digitgain);
>  
>  	vts = ov2680_res[dev->fmt_idx].lines_per_frame;
>  
> @@ -461,11 +461,11 @@ static int ov2680_v_flip(struct v4l2_subdev *sd, s32 value)
>  	ret = ov2680_read_reg(client, 1, OV2680_FLIP_REG, &val);
>  	if (ret)
>  		return ret;
> -	if (value) {
> +	if (value)
>  		val |= OV2680_FLIP_MIRROR_BIT_ENABLE;
> -	} else {
> +	else
>  		val &= ~OV2680_FLIP_MIRROR_BIT_ENABLE;
> -	}
> +
>  	ret = ov2680_write_reg(client, 1,
>  			       OV2680_FLIP_REG, val);
>  	if (ret)
> @@ -731,7 +731,8 @@ static int gpio_ctrl(struct v4l2_subdev *sd, bool flag)
>  	 * existing integrations often wire two (reset/power_down)
>  	 * because that is the way other sensors work.  There is no
>  	 * way to tell how it is wired internally, so existing
> -	 * firmwares expose both and we drive them symmetrically. */
> +	 * firmwares expose both and we drive them symmetrically.
> +	 */
>  	if (flag) {
>  		ret = dev->platform_data->gpio0_ctrl(sd, 1);
>  		usleep_range(10000, 15000);
> @@ -1060,9 +1061,9 @@ static int ov2680_s_stream(struct v4l2_subdev *sd, int enable)
>  
>  	mutex_lock(&dev->input_lock);
>  	if (enable)
> -		dev_dbg(&client->dev, "ov2680_s_stream one\n");
> +		dev_dbg(&client->dev, "%s on\n", __func__);
>  	else
> -		dev_dbg(&client->dev, "ov2680_s_stream off\n");
> +		dev_dbg(&client->dev, "%s off\n", __func__);
>  
>  	ret = ov2680_write_reg(client, 1, OV2680_SW_STREAM,
>  			       enable ? OV2680_START_STREAMING :
> @@ -1226,7 +1227,7 @@ static int ov2680_remove(struct i2c_client *client)
>  	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>  	struct ov2680_device *dev = to_ov2680_sensor(sd);
>  
> -	dev_dbg(&client->dev, "ov2680_remove...\n");
> +	dev_dbg(&client->dev, "%s...\n", __func__);
>  
>  	dev->platform_data->csi_cfg(sd, 0);
>  
> -- 
> 2.7.4
> 

Re: [PATCH] Replaced hard coded function names in debug messages with __func__ macro.

[Tabot Kevin]

Greetings Daniel,
Thank you very much for the response. So, should I just revert back to the original 
all the changes in places where I replace hard coded functions names with  __func__?

Kind regards,

Tabot Kevin.

On Mon, Nov 02, 2020 at 09:33:24AM +0000, Daniel Thompson wrote:
> On Sat, Oct 31, 2020 at 05:41:03PM +0100, Tabot Kevin wrote:
> > This patch fixes the following:
> > - Uses __func__ macro to print function names.
> > - Got rid of unnecessary braces around single line if statements.
> > - End of block comments on a seperate line.
> > - A spelling mistake of the word "on".
> > 
> > Signed-off-by: Tabot Kevin <tabot.kevin@gmail.com>
> > ---
> >  drivers/staging/media/atomisp/i2c/atomisp-ov2680.c | 25 +++++++++++-----------
> >  1 file changed, 13 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> > index c907305..1396a33 100644
> > --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> > +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> > @@ -146,7 +146,7 @@ static int ov2680_g_bin_factor_x(struct v4l2_subdev *sd, s32 *val)
> >  	struct ov2680_device *dev = to_ov2680_sensor(sd);
> >  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> >  
> > -	dev_dbg(&client->dev,  "++++ov2680_g_bin_factor_x\n");
> > +	dev_dbg(&client->dev,  "++++%s\n", __func__);
> 
> It might be better just to remove this sort of message.
> 
> They are not "wrong wrong" but are they actually useful one a
> driver's basic functions work? Even where they are useful
> dynamic techniques (ftrace, tracepoints, etc) arguably provide a
> better way to support "did my function actually run" debug
> approaches anyway.
> 
> 
> Daniel.
> 
> 
> >  	*val = ov2680_res[dev->fmt_idx].bin_factor_x;
> >  
> >  	return 0;
> > @@ -158,7 +158,7 @@ static int ov2680_g_bin_factor_y(struct v4l2_subdev *sd, s32 *val)
> >  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> >  
> >  	*val = ov2680_res[dev->fmt_idx].bin_factor_y;
> > -	dev_dbg(&client->dev,  "++++ov2680_g_bin_factor_y\n");
> > +	dev_dbg(&client->dev,  "++++%s\n", __func__);
> >  	return 0;
> >  }
> >  
> > @@ -173,7 +173,7 @@ static int ov2680_get_intg_factor(struct i2c_client *client,
> >  	u16 reg_val;
> >  	int ret;
> >  
> > -	dev_dbg(&client->dev,  "++++ov2680_get_intg_factor\n");
> > +	dev_dbg(&client->dev,  "++++%s\n", __func__);
> >  	if (!info)
> >  		return -EINVAL;
> >  
> > @@ -251,8 +251,8 @@ static long __ov2680_set_exposure(struct v4l2_subdev *sd, int coarse_itg,
> >  	int ret, exp_val;
> >  
> >  	dev_dbg(&client->dev,
> > -		"+++++++__ov2680_set_exposure coarse_itg %d, gain %d, digitgain %d++\n",
> > -		coarse_itg, gain, digitgain);
> > +		"+++++++%s coarse_itg %d, gain %d, digitgain %d++\n",
> > +		__func__, coarse_itg, gain, digitgain);
> >  
> >  	vts = ov2680_res[dev->fmt_idx].lines_per_frame;
> >  
> > @@ -461,11 +461,11 @@ static int ov2680_v_flip(struct v4l2_subdev *sd, s32 value)
> >  	ret = ov2680_read_reg(client, 1, OV2680_FLIP_REG, &val);
> >  	if (ret)
> >  		return ret;
> > -	if (value) {
> > +	if (value)
> >  		val |= OV2680_FLIP_MIRROR_BIT_ENABLE;
> > -	} else {
> > +	else
> >  		val &= ~OV2680_FLIP_MIRROR_BIT_ENABLE;
> > -	}
> > +
> >  	ret = ov2680_write_reg(client, 1,
> >  			       OV2680_FLIP_REG, val);
> >  	if (ret)
> > @@ -731,7 +731,8 @@ static int gpio_ctrl(struct v4l2_subdev *sd, bool flag)
> >  	 * existing integrations often wire two (reset/power_down)
> >  	 * because that is the way other sensors work.  There is no
> >  	 * way to tell how it is wired internally, so existing
> > -	 * firmwares expose both and we drive them symmetrically. */
> > +	 * firmwares expose both and we drive them symmetrically.
> > +	 */
> >  	if (flag) {
> >  		ret = dev->platform_data->gpio0_ctrl(sd, 1);
> >  		usleep_range(10000, 15000);
> > @@ -1060,9 +1061,9 @@ static int ov2680_s_stream(struct v4l2_subdev *sd, int enable)
> >  
> >  	mutex_lock(&dev->input_lock);
> >  	if (enable)
> > -		dev_dbg(&client->dev, "ov2680_s_stream one\n");
> > +		dev_dbg(&client->dev, "%s on\n", __func__);
> >  	else
> > -		dev_dbg(&client->dev, "ov2680_s_stream off\n");
> > +		dev_dbg(&client->dev, "%s off\n", __func__);
> >  
> >  	ret = ov2680_write_reg(client, 1, OV2680_SW_STREAM,
> >  			       enable ? OV2680_START_STREAMING :
> > @@ -1226,7 +1227,7 @@ static int ov2680_remove(struct i2c_client *client)
> >  	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> >  	struct ov2680_device *dev = to_ov2680_sensor(sd);
> >  
> > -	dev_dbg(&client->dev, "ov2680_remove...\n");
> > +	dev_dbg(&client->dev, "%s...\n", __func__);
> >  
> >  	dev->platform_data->csi_cfg(sd, 0);
> >  
> > -- 
> > 2.7.4
> > 

Re: [PATCH] Replaced hard coded function names in debug messages with __func__ macro.

[Daniel Thompson]

On Mon, Nov 02, 2020 at 01:15:56PM +0100, Tabot Kevin wrote:
> On Mon, Nov 02, 2020 at 09:33:24AM +0000, Daniel Thompson wrote:
> > On Sat, Oct 31, 2020 at 05:41:03PM +0100, Tabot Kevin wrote:
> > > This patch fixes the following:
> > > - Uses __func__ macro to print function names.
> > > - Got rid of unnecessary braces around single line if statements.
> > > - End of block comments on a seperate line.
> > > - A spelling mistake of the word "on".
> > > 
> > > Signed-off-by: Tabot Kevin <tabot.kevin@gmail.com>
> > > ---
> > >  drivers/staging/media/atomisp/i2c/atomisp-ov2680.c | 25 +++++++++++-----------
> > >  1 file changed, 13 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> > > index c907305..1396a33 100644
> > > --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> > > +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> > > @@ -146,7 +146,7 @@ static int ov2680_g_bin_factor_x(struct v4l2_subdev *sd, s32 *val)
> > >  	struct ov2680_device *dev = to_ov2680_sensor(sd);
> > >  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> > >  
> > > -	dev_dbg(&client->dev,  "++++ov2680_g_bin_factor_x\n");
> > > +	dev_dbg(&client->dev,  "++++%s\n", __func__);
> > 
> > It might be better just to remove this sort of message.
> > 
> > They are not "wrong wrong" but are they actually useful one a
> > driver's basic functions work? Even where they are useful
> > dynamic techniques (ftrace, tracepoints, etc) arguably provide a
> > better way to support "did my function actually run" debug
> > approaches anyway.
>
> Thank you very much for the response. So, should I just revert back to
> the original all the changes in places where I replace hard coded
> functions names with  __func__?

[Responses on LKML should be quoted like this rather than top-posted]

Personally I think it is better to remove them completely from the
driver rather than revert to the original form. Naturally if Mauro or
Sakari have strong views on this kind of printed message then you
need to take that into account but, in general, messages like this
add little or no value to the driver and can be removed.


> > > @@ -251,8 +251,8 @@ static long __ov2680_set_exposure(struct v4l2_subdev *sd, int coarse_itg,
> > >  	int ret, exp_val;
> > >  
> > >  	dev_dbg(&client->dev,
> > > -		"+++++++__ov2680_set_exposure coarse_itg %d, gain %d, digitgain %d++\n",
> > > -		coarse_itg, gain, digitgain);
> > > +		"+++++++%s coarse_itg %d, gain %d, digitgain %d++\n",
> > > +		__func__, coarse_itg, gain, digitgain);

This case is a little less clear cut since the printed message does show
some elements of internal state. However AFAICT this function just writes
some state to the hardware so I still take the view that dynamic
tools (I2C tracepoints for example) provide a better way to debug the
driver.


Daniel.

Re: [PATCH] Replaced hard coded function names in debug messages with __func__ macro.

[Tabot Kevin]

On Tue, Nov 03, 2020 at 10:04:40AM +0000, Daniel Thompson wrote:
> On Mon, Nov 02, 2020 at 01:15:56PM +0100, Tabot Kevin wrote:
> > On Mon, Nov 02, 2020 at 09:33:24AM +0000, Daniel Thompson wrote:
> > > On Sat, Oct 31, 2020 at 05:41:03PM +0100, Tabot Kevin wrote:
> > > > This patch fixes the following:
> > > > - Uses __func__ macro to print function names.
> > > > - Got rid of unnecessary braces around single line if statements.
> > > > - End of block comments on a seperate line.
> > > > - A spelling mistake of the word "on".
> > > > 
> > > > Signed-off-by: Tabot Kevin <tabot.kevin@gmail.com>
> > > > ---
> > > >  drivers/staging/media/atomisp/i2c/atomisp-ov2680.c | 25 +++++++++++-----------
> > > >  1 file changed, 13 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> > > > index c907305..1396a33 100644
> > > > --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> > > > +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> > > > @@ -146,7 +146,7 @@ static int ov2680_g_bin_factor_x(struct v4l2_subdev *sd, s32 *val)
> > > >  	struct ov2680_device *dev = to_ov2680_sensor(sd);
> > > >  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> > > >  
> > > > -	dev_dbg(&client->dev,  "++++ov2680_g_bin_factor_x\n");
> > > > +	dev_dbg(&client->dev,  "++++%s\n", __func__);
> > > 
> > > It might be better just to remove this sort of message.
> > > 
> > > They are not "wrong wrong" but are they actually useful one a
> > > driver's basic functions work? Even where they are useful
> > > dynamic techniques (ftrace, tracepoints, etc) arguably provide a
> > > better way to support "did my function actually run" debug
> > > approaches anyway.
> >
> > Thank you very much for the response. So, should I just revert back to
> > the original all the changes in places where I replace hard coded
> > functions names with  __func__?
> 
> [Responses on LKML should be quoted like this rather than top-posted]
> 
> Personally I think it is better to remove them completely from the
> driver rather than revert to the original form. Naturally if Mauro or
> Sakari have strong views on this kind of printed message then you
> need to take that into account but, in general, messages like this
> add little or no value to the driver and can be removed.
> 
I went through the code in an attempt to completely remove all "dev_dbg"
messages, but I noticed not only are there many "dev_dbg" messages, there
are also many such messages like (dev_info, dev_err, etc). Should I
remove them all too?
> 
> > > > @@ -251,8 +251,8 @@ static long __ov2680_set_exposure(struct v4l2_subdev *sd, int coarse_itg,
> > > >  	int ret, exp_val;
> > > >  
> > > >  	dev_dbg(&client->dev,
> > > > -		"+++++++__ov2680_set_exposure coarse_itg %d, gain %d, digitgain %d++\n",
> > > > -		coarse_itg, gain, digitgain);
> > > > +		"+++++++%s coarse_itg %d, gain %d, digitgain %d++\n",
> > > > +		__func__, coarse_itg, gain, digitgain);
> 
> This case is a little less clear cut since the printed message does show
> some elements of internal state. However AFAICT this function just writes
> some state to the hardware so I still take the view that dynamic
> tools (I2C tracepoints for example) provide a better way to debug the
> driver.
> 
> 
> Daniel.

Re: [PATCH] Replaced hard coded function names in debug messages with __func__ macro.

[Daniel Thompson]

On Tue, Nov 03, 2020 at 09:36:54PM +0100, Tabot Kevin wrote:
> On Tue, Nov 03, 2020 at 10:04:40AM +0000, Daniel Thompson wrote:
> > On Mon, Nov 02, 2020 at 01:15:56PM +0100, Tabot Kevin wrote:
> > > On Mon, Nov 02, 2020 at 09:33:24AM +0000, Daniel Thompson wrote:
> > > > > @@ -146,7 +146,7 @@ static int ov2680_g_bin_factor_x(struct v4l2_subdev *sd, s32 *val)
> > > > >  	struct ov2680_device *dev = to_ov2680_sensor(sd);
> > > > >  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> > > > >  
> > > > > -	dev_dbg(&client->dev,  "++++ov2680_g_bin_factor_x\n");
> > > > > +	dev_dbg(&client->dev,  "++++%s\n", __func__);
> > > > 
> > > > It might be better just to remove this sort of message.
> > > > 
> > > > They are not "wrong wrong" but are they actually useful one a
> > > > driver's basic functions work? Even where they are useful
> > > > dynamic techniques (ftrace, tracepoints, etc) arguably provide a
> > > > better way to support "did my function actually run" debug
> > > > approaches anyway.
> > >
> > > Thank you very much for the response. So, should I just revert back to
> > > the original all the changes in places where I replace hard coded
> > > functions names with  __func__?
> > 
> > Personally I think it is better to remove them completely from the
> > driver rather than revert to the original form. Naturally if Mauro or
> > Sakari have strong views on this kind of printed message then you
> > need to take that into account but, in general, messages like this
> > add little or no value to the driver and can be removed.
> > 
> I went through the code in an attempt to completely remove all "dev_dbg"
> messages,

The goal should not be to remove all dev_dbg() messages. I have only
suggested removing dev_dbg() that print things that are not useful or
redundantly duplicate what can be achieved with ftrace or tracepoints.

Maybe that will remove the dev_dbg() messages and maybe it won't. That
depends entirely what the function actually prints when executed.


> but I noticed not only are there many "dev_dbg" messages, there
> are also many such messages like (dev_info, dev_err, etc). Should I
> remove them all too?

The resulting patch will have your name on it rather than mine. That
means it is you that must make the decisions here.

Firstly you can review each message output to decide if it is useful.
Only remove message whose output is not useful (same as for dev_dbg() ).

If it is useful then you should think about whether the log level
matches the importance of the message. For example, are the dev_err()
message really covering error conditions? are there warnings for normal"
conditions? is the dev_info() useful to someone who is not the driver
author?).


Daniel.




> > 
> > > > > @@ -251,8 +251,8 @@ static long __ov2680_set_exposure(struct v4l2_subdev *sd, int coarse_itg,
> > > > >  	int ret, exp_val;
> > > > >  
> > > > >  	dev_dbg(&client->dev,
> > > > > -		"+++++++__ov2680_set_exposure coarse_itg %d, gain %d, digitgain %d++\n",
> > > > > -		coarse_itg, gain, digitgain);
> > > > > +		"+++++++%s coarse_itg %d, gain %d, digitgain %d++\n",
> > > > > +		__func__, coarse_itg, gain, digitgain);
> > 
> > This case is a little less clear cut since the printed message does show
> > some elements of internal state. However AFAICT this function just writes
> > some state to the hardware so I still take the view that dynamic
> > tools (I2C tracepoints for example) provide a better way to debug the
> > driver.
> > 
> > 
> > Daniel.