Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 3 Nov 2020 10:04:40 +0000
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     Tabot Kevin <tabot.kevin@gmail.com>
Cc:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Replaced hard coded function names in debug messages
 with __func__ macro.
Message-ID: <20201103100440.wo2wkkyr5rs4qhhl@holly.lan>
References: <20201031164059.GA5534@tabot>
 <20201102093324.dz5es32yhmga34hs@holly.lan>
 <20201102121556.GA37670@crucifix.local>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20201102121556.GA37670@crucifix.local>
Precedence: bulk

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.