[OPW,kernel] staging: media: lirc: Replaced printk macro with pr_info(....) and pr_cont(...) in lirc_sasem.c
diff mbox

Message ID 1382909417-6545-1-git-send-email-archanakumari959@gmail.com
State Rejected
Headers show

Commit Message

Archana kumari Oct. 27, 2013, 9:30 p.m. UTC
This patch Replaces printk macro with pr_info(....) and pr_cont(...)
in lirc_sasem.c

Signed-off-by: Archana kumari <archanakumari959@gmail.com>
---
 drivers/staging/media/lirc/lirc_sasem.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Waskiewicz Jr, Peter P Oct. 28, 2013, 12:16 a.m. UTC | #1
On Mon, 2013-10-28 at 03:00 +0530, Archana kumari wrote:
> This patch Replaces printk macro with pr_info(....) and pr_cont(...)
> in lirc_sasem.c

dev_info() is much more preferred here.  Also a few additional comments
below.

> 
> Signed-off-by: Archana kumari <archanakumari959@gmail.com>
> ---
>  drivers/staging/media/lirc/lirc_sasem.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/media/lirc/lirc_sasem.c b/drivers/staging/media/lirc/lirc_sasem.c
> index 68acca7..bee6c7c 100644
> --- a/drivers/staging/media/lirc/lirc_sasem.c
> +++ b/drivers/staging/media/lirc/lirc_sasem.c
> @@ -586,10 +586,10 @@ static void incoming_packet(struct sasem_context *context,
>  	}
>  
>  	if (debug) {
> -		printk(KERN_INFO "Incoming data: ");
> +		pr_info("Incoming data: ");

dev_info() please

>  		for (i = 0; i < 8; ++i)
> -			printk(KERN_CONT "%02x ", buf[i]);
> -		printk(KERN_CONT "\n");
> +			pr_cont("%02x ", buf[i]);
> +		pr_cont("\n");

Looking at the kernel comments for KERN_CONT (which pr_cont() resolves
to using - in linux/include/linux/kern_levels.h):

/*
 * Annotation for a "continued" line of log printout (only done after a
 * line that had no enclosing \n). Only to be used by core/arch code
 * during early bootup (a continued line is not SMP-safe otherwise).
 */

It would be better to rework this code to sprintf() into a temporary
buffer in the loop, then use dev_info() to print the whole thing all at
once.

It's more work than just replacing function calls with different calls,
but it is really more the right way to fix this.  Mind making these
minor adjustments and re-sending?

-PJ

>  	}
>  
>  	/*
> -- 
> 1.8.1.2
>
Greg Kroah-Hartman Oct. 28, 2013, 3:22 a.m. UTC | #2
On Mon, Oct 28, 2013 at 12:16:40AM +0000, Waskiewicz Jr, Peter P wrote:
> On Mon, 2013-10-28 at 03:00 +0530, Archana kumari wrote:
> > This patch Replaces printk macro with pr_info(....) and pr_cont(...)
> > in lirc_sasem.c
> 
> dev_info() is much more preferred here.  Also a few additional comments
> below.
> 
> > 
> > Signed-off-by: Archana kumari <archanakumari959@gmail.com>
> > ---
> >  drivers/staging/media/lirc/lirc_sasem.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/media/lirc/lirc_sasem.c b/drivers/staging/media/lirc/lirc_sasem.c
> > index 68acca7..bee6c7c 100644
> > --- a/drivers/staging/media/lirc/lirc_sasem.c
> > +++ b/drivers/staging/media/lirc/lirc_sasem.c
> > @@ -586,10 +586,10 @@ static void incoming_packet(struct sasem_context *context,
> >  	}
> >  
> >  	if (debug) {
> > -		printk(KERN_INFO "Incoming data: ");
> > +		pr_info("Incoming data: ");
> 
> dev_info() please
> 
> >  		for (i = 0; i < 8; ++i)
> > -			printk(KERN_CONT "%02x ", buf[i]);
> > -		printk(KERN_CONT "\n");
> > +			pr_cont("%02x ", buf[i]);
> > +		pr_cont("\n");
> 
> Looking at the kernel comments for KERN_CONT (which pr_cont() resolves
> to using - in linux/include/linux/kern_levels.h):
> 
> /*
>  * Annotation for a "continued" line of log printout (only done after a
>  * line that had no enclosing \n). Only to be used by core/arch code
>  * during early bootup (a continued line is not SMP-safe otherwise).
>  */
> 
> It would be better to rework this code to sprintf() into a temporary
> buffer in the loop, then use dev_info() to print the whole thing all at
> once.

Or even better yet, use the hexdump function to print it all out
properly with just one function call, that's what it is there for :)

thanks,

greg k-h
Archana kumari Oct. 28, 2013, 9:42 a.m. UTC | #3
Thanks Waskiewicz Jr, Peter P
 for your feedback . but i had tried using dev_info but then it broke the 
build 
 and pr_info( ... ) builded well so therefor I had used pr_info(...) so now 
what 
 should I do?

 thanks
 Archana 
On Monday, October 28, 2013 3:00:17 AM UTC+5:30, Archana Kumari wrote:
>
> This patch Replaces printk macro with pr_info(....) and pr_cont(...) 
> in lirc_sasem.c 
>
> Signed-off-by: Archana kumari <archanakumari959@gmail.com> 
> --- 
>  drivers/staging/media/lirc/lirc_sasem.c | 6 +++--- 
>  1 file changed, 3 insertions(+), 3 deletions(-) 
>
> diff --git a/drivers/staging/media/lirc/lirc_sasem.c 
> b/drivers/staging/media/lirc/lirc_sasem.c 
> index 68acca7..bee6c7c 100644 
> --- a/drivers/staging/media/lirc/lirc_sasem.c 
> +++ b/drivers/staging/media/lirc/lirc_sasem.c 
> @@ -586,10 +586,10 @@ static void incoming_packet(struct sasem_context 
> *context, 
>          } 
>   
>          if (debug) { 
> -                printk(KERN_INFO "Incoming data: "); 
> +                pr_info("Incoming data: "); 
>                  for (i = 0; i < 8; ++i) 
> -                        printk(KERN_CONT "%02x ", buf[i]); 
> -                printk(KERN_CONT "\n"); 
> +                        pr_cont("%02x ", buf[i]); 
> +                pr_cont("\n"); 
>          } 
>   
>          /* 
> -- 
> 1.8.1.2 
>
>
Archana kumari Oct. 28, 2013, 10:10 a.m. UTC | #4
hii Greg ,

THANKS a lot for your feedback . 
and a small question too that  if I use hexdump to print all at the same 
time 
 do I need to include some headers.
 
Thanks
Archana

On Monday, October 28, 2013 3:00:17 AM UTC+5:30, Archana Kumari wrote:
>
> This patch Replaces printk macro with pr_info(....) and pr_cont(...) 
> in lirc_sasem.c 
>
> Signed-off-by: Archana kumari <archanakumari959@gmail.com> 
> --- 
>  drivers/staging/media/lirc/lirc_sasem.c | 6 +++--- 
>  1 file changed, 3 insertions(+), 3 deletions(-) 
>
> diff --git a/drivers/staging/media/lirc/lirc_sasem.c 
> b/drivers/staging/media/lirc/lirc_sasem.c 
> index 68acca7..bee6c7c 100644 
> --- a/drivers/staging/media/lirc/lirc_sasem.c 
> +++ b/drivers/staging/media/lirc/lirc_sasem.c 
> @@ -586,10 +586,10 @@ static void incoming_packet(struct sasem_context 
> *context, 
>          } 
>   
>          if (debug) { 
> -                printk(KERN_INFO "Incoming data: "); 
> +                pr_info("Incoming data: "); 
>                  for (i = 0; i < 8; ++i) 
> -                        printk(KERN_CONT "%02x ", buf[i]); 
> -                printk(KERN_CONT "\n"); 
> +                        pr_cont("%02x ", buf[i]); 
> +                pr_cont("\n"); 
>          } 
>   
>          /* 
> -- 
> 1.8.1.2 
>
>
Waskiewicz Jr, Peter P Oct. 28, 2013, 10:47 a.m. UTC | #5
On Mon, 2013-10-28 at 02:42 -0700, Archana Kumari wrote:
>  Thanks Waskiewicz Jr, Peter P
>  for your feedback . but i had tried using dev_info but then it broke
> the build 
>  and pr_info( ... ) builded well so therefor I had used pr_info(...)
> so now what 
>  should I do?

You should always reply to emails where the feedback originated, not
your original email.  Remember that maintainers deal with hundreds of
emails a day, and losing context of a thread is something that really is
a big time drain.  So please make sure to reply to the feedback email in
the future.

As for getting dev_info() to work, there are lots of other drivers using
it.  There have also been other patches to this list that added
dev_info() calls, so it's good practice to see how other people solve
these problems.  What you're probably missing is the definition of a
pr_fmt() macro, so dev_info() won't compile.  I'd recommend digging
around some other drivers in the kernel, including staging, that use
dev_info(), and see how they make it work.

>  thanks
>  Archana 
> On Monday, October 28, 2013 3:00:17 AM UTC+5:30, Archana Kumari wrote:
>         This patch Replaces printk macro with pr_info(....) and
>         pr_cont(...) 
>         in lirc_sasem.c 
>         
>         Signed-off-by: Archana kumari <archanakumari959@gmail.com> 
>         --- 
>          drivers/staging/media/lirc/lirc_sasem.c | 6 +++--- 
>          1 file changed, 3 insertions(+), 3 deletions(-) 
>         
>         diff --git a/drivers/staging/media/lirc/lirc_sasem.c
>         b/drivers/staging/media/lirc/lirc_sasem.c 
>         index 68acca7..bee6c7c 100644 
>         --- a/drivers/staging/media/lirc/lirc_sasem.c 
>         +++ b/drivers/staging/media/lirc/lirc_sasem.c 
>         @@ -586,10 +586,10 @@ static void incoming_packet(struct
>         sasem_context *context, 
>                  } 
>           
>                  if (debug) { 
>         -                printk(KERN_INFO "Incoming data: "); 
>         +                pr_info("Incoming data: "); 
>                          for (i = 0; i < 8; ++i) 
>         -                        printk(KERN_CONT "%02x ", buf[i]); 
>         -                printk(KERN_CONT "\n"); 
>         +                        pr_cont("%02x ", buf[i]); 
>         +                pr_cont("\n"); 
>                  } 
>           
>                  /* 
>         -- 
>         1.8.1.2 
>
Greg Kroah-Hartman Oct. 28, 2013, 2:27 p.m. UTC | #6
On Mon, Oct 28, 2013 at 10:47:14AM +0000, Waskiewicz Jr, Peter P wrote:
> On Mon, 2013-10-28 at 02:42 -0700, Archana Kumari wrote:
> >  Thanks Waskiewicz Jr, Peter P
> >  for your feedback . but i had tried using dev_info but then it broke
> > the build 
> >  and pr_info( ... ) builded well so therefor I had used pr_info(...)
> > so now what 
> >  should I do?
> 
> You should always reply to emails where the feedback originated, not
> your original email.  Remember that maintainers deal with hundreds of
> emails a day, and losing context of a thread is something that really is
> a big time drain.  So please make sure to reply to the feedback email in
> the future.
> 
> As for getting dev_info() to work, there are lots of other drivers using
> it.  There have also been other patches to this list that added
> dev_info() calls, so it's good practice to see how other people solve
> these problems.  What you're probably missing is the definition of a
> pr_fmt() macro, so dev_info() won't compile.

dev_info() does not use pr_fmt(), it uses a 'struct device' instead.

greg k-h
Waskiewicz Jr, Peter P Oct. 28, 2013, 9:05 p.m. UTC | #7
On Mon, 2013-10-28 at 07:27 -0700, Greg KH wrote:
> On Mon, Oct 28, 2013 at 10:47:14AM +0000, Waskiewicz Jr, Peter P wrote:
> > On Mon, 2013-10-28 at 02:42 -0700, Archana Kumari wrote:
> > >  Thanks Waskiewicz Jr, Peter P
> > >  for your feedback . but i had tried using dev_info but then it broke
> > > the build 
> > >  and pr_info( ... ) builded well so therefor I had used pr_info(...)
> > > so now what 
> > >  should I do?
> > 
> > You should always reply to emails where the feedback originated, not
> > your original email.  Remember that maintainers deal with hundreds of
> > emails a day, and losing context of a thread is something that really is
> > a big time drain.  So please make sure to reply to the feedback email in
> > the future.
> > 
> > As for getting dev_info() to work, there are lots of other drivers using
> > it.  There have also been other patches to this list that added
> > dev_info() calls, so it's good practice to see how other people solve
> > these problems.  What you're probably missing is the definition of a
> > pr_fmt() macro, so dev_info() won't compile.
> 
> dev_info() does not use pr_fmt(), it uses a 'struct device' instead.

And that's why I shouldn't reply to emails at 3:45am...

> greg k-h
>

Patch
diff mbox

diff --git a/drivers/staging/media/lirc/lirc_sasem.c b/drivers/staging/media/lirc/lirc_sasem.c
index 68acca7..bee6c7c 100644
--- a/drivers/staging/media/lirc/lirc_sasem.c
+++ b/drivers/staging/media/lirc/lirc_sasem.c
@@ -586,10 +586,10 @@  static void incoming_packet(struct sasem_context *context,
 	}
 
 	if (debug) {
-		printk(KERN_INFO "Incoming data: ");
+		pr_info("Incoming data: ");
 		for (i = 0; i < 8; ++i)
-			printk(KERN_CONT "%02x ", buf[i]);
-		printk(KERN_CONT "\n");
+			pr_cont("%02x ", buf[i]);
+		pr_cont("\n");
 	}
 
 	/*