diff mbox

[4/4] Input-gameport: Replace some printk() calls by pr_info() in joydump_connect()

Message ID f486d64d-0759-9cd6-1e4e-df723acb6fea@users.sourceforge.net (mailing list archive)
State Rejected
Headers show

Commit Message

SF Markus Elfring Sept. 24, 2016, 1:50 p.m. UTC 
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 24 Sep 2016 15:12:21 +0200

* Prefer usage of the macro "pr_info" over the interface "printk"
  in this function.

* Reduce number of output function calls.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/input/joystick/joydump.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

Comments

Joe Perches Sept. 24, 2016, 4:13 p.m. UTC | #1 
On Sat, 2016-09-24 at 15:50 +0200, SF Markus Elfring wrote:

> * Prefer usage of the macro "pr_info" over the interface "printk"
>   in this function.
> * Reduce number of output function calls.

Did you test this?  I doubt it.

> diff --git a/drivers/input/joystick/joydump.c b/drivers/input/joystick/joydump.c
[]
> @@ -57,27 +57,29 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
> >  	unsigned long flags;
> >  	unsigned char u;
>  
> > -	printk(KERN_INFO "joydump: ,------------------ START ----------------.\n");
> > -	printk(KERN_INFO "joydump: | Dumping: %30s |\n", gameport->phys);
> > -	printk(KERN_INFO "joydump: | Speed: %28d kHz |\n", gameport->speed);
> > +	pr_info(",------------------ START ----------------.\n"
> > +		"| Dumping: %30s |\n"
> > +		"| Speed: %28d kHz |\n",
> > +		gameport->phys,
> > +		gameport->speed);

Not the same output.

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
SF Markus Elfring Sept. 24, 2016, 4:32 p.m. UTC | #2 
>> @@ -57,27 +57,29 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
>>>  	unsigned long flags;
>>>  	unsigned char u;
>>  
>>> -	printk(KERN_INFO "joydump: ,------------------ START ----------------.\n");
>>> -	printk(KERN_INFO "joydump: | Dumping: %30s |\n", gameport->phys);
>>> -	printk(KERN_INFO "joydump: | Speed: %28d kHz |\n", gameport->speed);
>>> +	pr_info(",------------------ START ----------------.\n"
>>> +		"| Dumping: %30s |\n"
>>> +		"| Speed: %28d kHz |\n",
>>> +		gameport->phys,
>>> +		gameport->speed);
> 
> Not the same output.

Should the desired output be the same when the relevant data are passed by a single function call
(instead of three as before)?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Torokhov Sept. 24, 2016, 4:39 p.m. UTC | #3 
On Sat, Sep 24, 2016 at 06:32:29PM +0200, SF Markus Elfring wrote:
> >> @@ -57,27 +57,29 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
> >>>  	unsigned long flags;
> >>>  	unsigned char u;
> >>  
> >>> -	printk(KERN_INFO "joydump: ,------------------ START ----------------.\n");
> >>> -	printk(KERN_INFO "joydump: | Dumping: %30s |\n", gameport->phys);
> >>> -	printk(KERN_INFO "joydump: | Speed: %28d kHz |\n", gameport->speed);
> >>> +	pr_info(",------------------ START ----------------.\n"
> >>> +		"| Dumping: %30s |\n"
> >>> +		"| Speed: %28d kHz |\n",
> >>> +		gameport->phys,
> >>> +		gameport->speed);
> > 
> > Not the same output.
> 
> Should the desired output be the same when the relevant data are passed by a single function call
> (instead of three as before)?

The desired output should not be broken in conversion, which you did. Do
you know how syslog works and why the transformation is not correct.

I am also curious as to why you are patching joydump? Are you working on
extending it?

Thanks.
SF Markus Elfring Sept. 24, 2016, 4:41 p.m. UTC | #4 
>> @@ -57,27 +57,29 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
>>>  	unsigned long flags;
>>>  	unsigned char u;
>>  
>>> -	printk(KERN_INFO "joydump: ,------------------ START ----------------.\n");
>>> -	printk(KERN_INFO "joydump: | Dumping: %30s |\n", gameport->phys);
>>> -	printk(KERN_INFO "joydump: | Speed: %28d kHz |\n", gameport->speed);
>>> +	pr_info(",------------------ START ----------------.\n"
>>> +		"| Dumping: %30s |\n"
>>> +		"| Speed: %28d kHz |\n",
>>> +		gameport->phys,
>>> +		gameport->speed);
> 
> Not the same output.

Do you insist that each line from a multi-line text that is passed by such a single logging call
contains the same module prefix?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches Sept. 24, 2016, 4:47 p.m. UTC | #5 
On Sat, 2016-09-24 at 18:32 +0200, SF Markus Elfring wrote:
> > > @@ -57,27 +57,29 @@ static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
> > >  	unsigned long flags;
> > >  	unsigned char u;
> > 
> >  
> > > -	printk(KERN_INFO "joydump: ,------------------ START ----------------.\n");
> > > -	printk(KERN_INFO "joydump: | Dumping: %30s |\n", gameport->phys);
> > > -	printk(KERN_INFO "joydump: | Speed: %28d kHz |\n", gameport->speed);
> > > +	pr_info(",------------------ START ----------------.\n"
> > > +		"| Dumping: %30s |\n"
> > > +		"| Speed: %28d kHz |\n",
> > > +		gameport->phys,
> > > +		gameport->speed);
> 
> 
> > Not the same output.
> 
> 
> Should the desired output be the same when the relevant data are passed by a single function call
> (instead of three as before)?

Adding a singleton for a pr_fmt #define constant string and
updating the printk subsystem to prepend that constant string
to each use of a pr_<level> at runtime would be an improvement
as it could reduce constant data used by the format strings.

That would be a _real_ improvement.

Please try to implement something like that before submitting
more of these incorrect patches.

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
SF Markus Elfring Sept. 24, 2016, 5:20 p.m. UTC | #6 
> The desired output should not be broken in conversion, which you did.

I dared to change another source code place a bit too much perhaps.


> Do you know how syslog works and why the transformation is not correct.

I imagine that there are chances to improve the software situation a bit more,
aren't there?


> I am also curious as to why you are patching joydump?

The discussed function implementation contains update candidates.

One of them was detected by the execution of a script for the semantic patch
language (Coccinelle software).
I identified further update possibilities after the opportunity for using
the function "kmalloc_array" also in this software module.


> Are you working on extending it?

I attempted just another software refactoring.

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
SF Markus Elfring Sept. 24, 2016, 7:50 p.m. UTC | #7 
>> Should the desired output be the same when the relevant data are passed by a single function call
>> (instead of three as before)?
> 
> Adding a singleton for a pr_fmt #define constant string and
> updating the printk subsystem to prepend that constant string
> to each use of a pr_<level> at runtime would be an improvement
> as it could reduce constant data used by the format strings.

Thanks for for this constructive feedback.


> That would be a _real_ improvement.

This sounds interesting for me too.


> Please try to implement something like that before submitting
> more of these incorrect patches.

Nice wish!

Did any other software developer try to implement such enhanced
logging functionality already?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/input/joystick/joydump.c b/drivers/input/joystick/joydump.c
index a38f10e..02ea60c 100644
--- a/drivers/input/joystick/joydump.c
+++ b/drivers/input/joystick/joydump.c
@@ -57,27 +57,29 @@  static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
 	unsigned long flags;
 	unsigned char u;
 
-	printk(KERN_INFO "joydump: ,------------------ START ----------------.\n");
-	printk(KERN_INFO "joydump: | Dumping: %30s |\n", gameport->phys);
-	printk(KERN_INFO "joydump: | Speed: %28d kHz |\n", gameport->speed);
+	pr_info(",------------------ START ----------------.\n"
+		"| Dumping: %30s |\n"
+		"| Speed: %28d kHz |\n",
+		gameport->phys,
+		gameport->speed);
 
 	if (gameport_open(gameport, drv, GAMEPORT_MODE_RAW)) {
-
-		printk(KERN_INFO "joydump: | Raw mode not available - trying cooked.    |\n");
-
+		pr_info("| Raw mode not available - trying cooked.    |\n");
 		if (gameport_open(gameport, drv, GAMEPORT_MODE_COOKED)) {
-
-			printk(KERN_INFO "joydump: | Cooked not available either. Failing.   |\n");
-			printk(KERN_INFO "joydump: `------------------- END -----------------'\n");
+			pr_info("| Cooked not available either. Failing.   |\n"
+				"`------------------- END -----------------'\n");
 			return -ENODEV;
 		}
 
 		gameport_cooked_read(gameport, axes, &buttons);
 
 		for (i = 0; i < 4; i++)
-			printk(KERN_INFO "joydump: | Axis %d: %4d.                           |\n", i, axes[i]);
-		printk(KERN_INFO "joydump: | Buttons %02x.                             |\n", buttons);
-		printk(KERN_INFO "joydump: `------------------- END -----------------'\n");
+			pr_info("| Axis %d: %4d.                           |\n",
+				i,
+				axes[i]);
+		pr_info("| Buttons %02x.                             |\n"
+			"`------------------- END -----------------'\n",
+			buttons);
 	}
 
 	timeout = gameport_time(gameport, 10000); /* 10 ms */
@@ -121,16 +123,15 @@  static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
 	t = i;
 	dump = buf;
 	prev = dump;
-
-	printk(KERN_INFO "joydump: >------------------ DATA -----------------<\n");
-	printk(KERN_INFO "joydump: | index: %3d delta: %3d us data: ", 0, 0);
+	pr_info(">------------------ DATA -----------------<\n"
+		"| index: %3d delta: %3d us data: ", 0, 0);
 	for (j = 7; j >= 0; j--)
 		printk("%d", (dump->data >> j) & 1);
 	printk(" |\n");
 	dump++;
 
 	for (i = 1; i < t; i++, dump++, prev++) {
-		printk(KERN_INFO "joydump: | index: %3d delta: %3d us data: ",
+		pr_info("| index: %3d delta: %3d us data: ",
 			i, dump->time - prev->time);
 		for (j = 7; j >= 0; j--)
 			printk("%d", (dump->data >> j) & 1);
@@ -139,8 +140,7 @@  static int joydump_connect(struct gameport *gameport, struct gameport_driver *dr
 	kfree(buf);
 
 jd_end:
-	printk(KERN_INFO "joydump: `------------------- END -----------------'\n");
-
+	pr_info("`------------------- END -----------------'\n");
 	return 0;
 }