Re: [PATCH] media: atomisp: ov2722: replace hardcoded function name

2021-01-07 Thread Dan Carpenter 
On Wed, Jan 06, 2021 at 01:17:47PM -0800, Joe Perches wrote:
> On Wed, 2021-01-06 at 22:36 +0300, Dan Carpenter wrote:
> > On Wed, Jan 06, 2021 at 10:25:26AM -0800, Joe Perches wrote:
> > > On Wed, 2021-01-06 at 18:52 +0100, Greg Kroah-Hartman wrote:
> > > > On Wed, Jan 06, 2021 at 07:43:42PM +0200, Filip Kolev wrote: 
> > > > > On 06-Jan-21 09:51, Greg Kroah-Hartman wrote:
> > > > > > On Tue, Jan 05, 2021 at 10:29:18PM +0200, Filip Kolev wrote:
> > > > > > > There is a debug message using hardcoded function name instead of 
> > > > > > > the
> > > > > > > __func__ macro. Replace it.
> > > > > > > 
> > > > > > > Report from checkpatch.pl on the file:
> > > > > > > 
> > > > > > > WARNING: Prefer using '"%s...", __func__' to using 
> > > > > > > 'ov2722_remove', this function's name, in a string
> > > > > > > + dev_dbg(&client->dev, "ov2722_remove...\n");
> []
> > > There are quite a lot of these relatively useless function tracing like
> > > uses in the kernel:
> > > 
> > > $ git grep -P '"%s[\.\!]*\\n"\s*,\s*__func__\s*\)' | wc -l
> > > 1065
> > 
> > These are printing other stuff besides just the function name.
> 
> No, these are printing _only_ the function name.
> 

Oh...  Duh...  I was looking at the complete wrong output.  My bad.

Yeah.  I like this warning.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] media: atomisp: ov2722: replace hardcoded function name

2021-01-06 Thread Joe Perches 
On Wed, 2021-01-06 at 22:36 +0300, Dan Carpenter wrote:
> On Wed, Jan 06, 2021 at 10:25:26AM -0800, Joe Perches wrote:
> > On Wed, 2021-01-06 at 18:52 +0100, Greg Kroah-Hartman wrote:
> > > On Wed, Jan 06, 2021 at 07:43:42PM +0200, Filip Kolev wrote: 
> > > > On 06-Jan-21 09:51, Greg Kroah-Hartman wrote:
> > > > > On Tue, Jan 05, 2021 at 10:29:18PM +0200, Filip Kolev wrote:
> > > > > > There is a debug message using hardcoded function name instead of 
> > > > > > the
> > > > > > __func__ macro. Replace it.
> > > > > > 
> > > > > > Report from checkpatch.pl on the file:
> > > > > > 
> > > > > > WARNING: Prefer using '"%s...", __func__' to using 'ov2722_remove', 
> > > > > > this function's name, in a string
> > > > > > +   dev_dbg(&client->dev, "ov2722_remove...\n");
[]
> > There are quite a lot of these relatively useless function tracing like
> > uses in the kernel:
> > 
> > $ git grep -P '"%s[\.\!]*\\n"\s*,\s*__func__\s*\)' | wc -l
> > 1065
> 
> These are printing other stuff besides just the function name.

No, these are printing _only_ the function name.

> Maybe grep for '", __func__\)'?

No, that wouldn't work as there are many many uses like:

printk('%s: some string\n", __func__);


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] media: atomisp: ov2722: replace hardcoded function name

2021-01-06 Thread Dan Carpenter 
On Wed, Jan 06, 2021 at 10:25:26AM -0800, Joe Perches wrote:
> On Wed, 2021-01-06 at 18:52 +0100, Greg Kroah-Hartman wrote:
> > On Wed, Jan 06, 2021 at 07:43:42PM +0200, Filip Kolev wrote: 
> > > On 06-Jan-21 09:51, Greg Kroah-Hartman wrote:
> > > > On Tue, Jan 05, 2021 at 10:29:18PM +0200, Filip Kolev wrote:
> > > > > There is a debug message using hardcoded function name instead of the
> > > > > __func__ macro. Replace it.
> > > > > 
> > > > > Report from checkpatch.pl on the file:
> > > > > 
> > > > > WARNING: Prefer using '"%s...", __func__' to using 'ov2722_remove', 
> > > > > this function's name, in a string
> > > > > + dev_dbg(&client->dev, "ov2722_remove...\n");
> []
> > > > > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c 
> > > > > b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> []
> > > > > @@ -1175,7 +1175,7 @@ static int ov2722_remove(struct i2c_client 
> > > > > *client)
> > > > >   struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > > >   struct ov2722_device *dev = to_ov2722_sensor(sd);
> > > > > - dev_dbg(&client->dev, "ov2722_remove...\n");
> > > > > + dev_dbg(&client->dev, "%s...\n", __func__);
> > > > 
> > > > dev_dbg() provides the function name already, and this is just a "trace"
> > > > call, and ftrace should be used instead, so the whole line should be
> > > > removed entirely.
> > > 
> > > Thank you for the review!
> > > 
> > > How do I go about this? Do I amend the patch and re-send as v2 or create a
> > > new patch entirely?
> > 
> > New patch entirely please.
> 
> There are quite a lot of these relatively useless function tracing like
> uses in the kernel:
> 
> $ git grep -P '"%s[\.\!]*\\n"\s*,\s*__func__\s*\)' | wc -l
> 1065

These are printing other stuff besides just the function name.  Maybe
grep for '", __func__\)'?

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] media: atomisp: ov2722: replace hardcoded function name

2021-01-06 Thread Joe Perches 
On Wed, 2021-01-06 at 18:52 +0100, Greg Kroah-Hartman wrote:
> On Wed, Jan 06, 2021 at 07:43:42PM +0200, Filip Kolev wrote: 
> > On 06-Jan-21 09:51, Greg Kroah-Hartman wrote:
> > > On Tue, Jan 05, 2021 at 10:29:18PM +0200, Filip Kolev wrote:
> > > > There is a debug message using hardcoded function name instead of the
> > > > __func__ macro. Replace it.
> > > > 
> > > > Report from checkpatch.pl on the file:
> > > > 
> > > > WARNING: Prefer using '"%s...", __func__' to using 'ov2722_remove', 
> > > > this function's name, in a string
> > > > +   dev_dbg(&client->dev, "ov2722_remove...\n");
[]
> > > > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c 
> > > > b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
[]
> > > > @@ -1175,7 +1175,7 @@ static int ov2722_remove(struct i2c_client 
> > > > *client)
> > > >     struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > >     struct ov2722_device *dev = to_ov2722_sensor(sd);
> > > > -   dev_dbg(&client->dev, "ov2722_remove...\n");
> > > > +   dev_dbg(&client->dev, "%s...\n", __func__);
> > > 
> > > dev_dbg() provides the function name already, and this is just a "trace"
> > > call, and ftrace should be used instead, so the whole line should be
> > > removed entirely.
> > 
> > Thank you for the review!
> > 
> > How do I go about this? Do I amend the patch and re-send as v2 or create a
> > new patch entirely?
> 
> New patch entirely please.

There are quite a lot of these relatively useless function tracing like
uses in the kernel:

$ git grep -P '"%s[\.\!]*\\n"\s*,\s*__func__\s*\)' | wc -l
1065

Perhaps yet another checkpatch warning would be useful:
---
 scripts/checkpatch.pl | 8 
 1 file changed, 8 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e6857bdfcb2d..46b8ec8fe9e1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5981,6 +5981,14 @@ sub process {
 "Prefer using '\"%s...\", __func__' to using 
'$context_function', this function's name, in a string\n" . $herecurr);
}
 
+# check for unnecessary function tracing like uses
+   if ($rawline =~ 
/^\+\s*$logFunctions\s*\([^"]*"%s[\.\!]*\\n"\s*,\s*__func__\s*\)\s*;\s*$/) {
+   if (WARN("TRACING_LOGGING",
+"Unnecessary ftrace-like logging - prefer 
using ftrace\n" . $herecurr) &&
+   $fix) {
+fix_delete_line($fixlinenr, $rawline);
+   }
+   }
 # check for spaces before a quoted newline
if ($rawline =~ /^.*\".*\s\\n/) {
if (WARN("QUOTED_WHITESPACE_BEFORE_NEWLINE",

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] media: atomisp: ov2722: replace hardcoded function name

2021-01-06 Thread Greg Kroah-Hartman 
On Wed, Jan 06, 2021 at 07:43:42PM +0200, Filip Kolev wrote:
> 
> 
> On 06-Jan-21 09:51, Greg Kroah-Hartman wrote:
> > On Tue, Jan 05, 2021 at 10:29:18PM +0200, Filip Kolev wrote:
> > > There is a debug message using hardcoded function name instead of the
> > > __func__ macro. Replace it.
> > > 
> > > Report from checkpatch.pl on the file:
> > > 
> > > WARNING: Prefer using '"%s...", __func__' to using 'ov2722_remove', this 
> > > function's name, in a string
> > > + dev_dbg(&client->dev, "ov2722_remove...\n");
> > > 
> > > Signed-off-by: Filip Kolev 
> > > ---
> > >   drivers/staging/media/atomisp/i2c/atomisp-ov2722.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c 
> > > b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> > > index eecefcd734d0e..21d6bc62d452a 100644
> > > --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> > > +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> > > @@ -1175,7 +1175,7 @@ static int ov2722_remove(struct i2c_client *client)
> > >   struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > >   struct ov2722_device *dev = to_ov2722_sensor(sd);
> > > - dev_dbg(&client->dev, "ov2722_remove...\n");
> > > + dev_dbg(&client->dev, "%s...\n", __func__);
> > 
> > dev_dbg() provides the function name already, and this is just a "trace"
> > call, and ftrace should be used instead, so the whole line should be
> > removed entirely.
> 
> Thank you for the review!
> 
> How do I go about this? Do I amend the patch and re-send as v2 or create a
> new patch entirely?

New patch entirely please.

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] media: atomisp: ov2722: replace hardcoded function name

2021-01-06 Thread Filip Kolev 




On 06-Jan-21 09:51, Greg Kroah-Hartman wrote:

On Tue, Jan 05, 2021 at 10:29:18PM +0200, Filip Kolev wrote:

There is a debug message using hardcoded function name instead of the
__func__ macro. Replace it.

Report from checkpatch.pl on the file:

WARNING: Prefer using '"%s...", __func__' to using 'ov2722_remove', this 
function's name, in a string
+   dev_dbg(&client->dev, "ov2722_remove...\n");

Signed-off-by: Filip Kolev 
---
  drivers/staging/media/atomisp/i2c/atomisp-ov2722.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c 
b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
index eecefcd734d0e..21d6bc62d452a 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
@@ -1175,7 +1175,7 @@ static int ov2722_remove(struct i2c_client *client)
struct v4l2_subdev *sd = i2c_get_clientdata(client);
struct ov2722_device *dev = to_ov2722_sensor(sd);
  
-	dev_dbg(&client->dev, "ov2722_remove...\n");

+   dev_dbg(&client->dev, "%s...\n", __func__);


dev_dbg() provides the function name already, and this is just a "trace"
call, and ftrace should be used instead, so the whole line should be
removed entirely.


Thank you for the review!

How do I go about this? Do I amend the patch and re-send as v2 or create 
a new patch entirely?
Newbie here, doing this as part of the Eudyptula challenge, so I very 
much appreciate everyone's patience.




thanks,

greg k-h


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] media: atomisp: ov2722: replace hardcoded function name

2021-01-05 Thread Greg Kroah-Hartman 
On Tue, Jan 05, 2021 at 10:29:18PM +0200, Filip Kolev wrote:
> There is a debug message using hardcoded function name instead of the
> __func__ macro. Replace it.
> 
> Report from checkpatch.pl on the file:
> 
> WARNING: Prefer using '"%s...", __func__' to using 'ov2722_remove', this 
> function's name, in a string
> + dev_dbg(&client->dev, "ov2722_remove...\n");
> 
> Signed-off-by: Filip Kolev 
> ---
>  drivers/staging/media/atomisp/i2c/atomisp-ov2722.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c 
> b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> index eecefcd734d0e..21d6bc62d452a 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> @@ -1175,7 +1175,7 @@ static int ov2722_remove(struct i2c_client *client)
>   struct v4l2_subdev *sd = i2c_get_clientdata(client);
>   struct ov2722_device *dev = to_ov2722_sensor(sd);
>  
> - dev_dbg(&client->dev, "ov2722_remove...\n");
> + dev_dbg(&client->dev, "%s...\n", __func__);

dev_dbg() provides the function name already, and this is just a "trace"
call, and ftrace should be used instead, so the whole line should be
removed entirely.

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] media: atomisp: ov2722: replace hardcoded function name

2021-01-05 Thread Filip Kolev 
There is a debug message using hardcoded function name instead of the
__func__ macro. Replace it.

Report from checkpatch.pl on the file:

WARNING: Prefer using '"%s...", __func__' to using 'ov2722_remove', this 
function's name, in a string
+   dev_dbg(&client->dev, "ov2722_remove...\n");

Signed-off-by: Filip Kolev 
---
 drivers/staging/media/atomisp/i2c/atomisp-ov2722.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c 
b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
index eecefcd734d0e..21d6bc62d452a 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
@@ -1175,7 +1175,7 @@ static int ov2722_remove(struct i2c_client *client)
struct v4l2_subdev *sd = i2c_get_clientdata(client);
struct ov2722_device *dev = to_ov2722_sensor(sd);
 
-   dev_dbg(&client->dev, "ov2722_remove...\n");
+   dev_dbg(&client->dev, "%s...\n", __func__);
 
dev->platform_data->csi_cfg(sd, 0);
v4l2_ctrl_handler_free(&dev->ctrl_handler);
-- 
2.30.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel