Messages in this thread | | | Date | Thu, 22 Feb 2018 15:45:09 +0100 | From | Boris Brezillon <> | Subject | Re: [PATCH NAND v3] mtd: nand: Replace printk() with appropriate pr_*() macro |
| |
Hi Shreeya,
On Thu, 22 Feb 2018 18:40:14 +0530 Shreeya Patel <shreeya.patel23498@gmail.com> wrote:
> diff --git a/drivers/mtd/nand/diskonchip.c b/drivers/mtd/nand/diskonchip.c > index c3aa53c..8643512 100644 > --- a/drivers/mtd/nand/diskonchip.c > +++ b/drivers/mtd/nand/diskonchip.c
[...]
> @@ -438,7 +438,7 @@ static void __init doc2000_count_chips(struct mtd_info *mtd) > break; > } > doc->chips_per_floor = i; > - printk(KERN_DEBUG "Detected %d chips per floor.\n", i); > + pr_debug("Detected %d chips per floor.\n", i);
Just want to add some more context on pr_debug(), nothing that requires changes on your side, but as Richard said, it's important to explain it.
pr_debug() and printk(KERN_DEBUG) are not exactly the same. pr_debug() calls are NOPs by default unless you have enabled the CONFIG_DYNAMIC_DEBUG option or defined DEBUG (either directly in the sources or by passing -DDEBUG to gcc). This is not the case with printk(KERN_DEBUG).
If you want to know more about dynamic debug, you can read [1].
> diff --git a/drivers/mtd/nand/r852.c b/drivers/mtd/nand/r852.c > index fc9287a..cd3cd1c 100644 > --- a/drivers/mtd/nand/r852.c > +++ b/drivers/mtd/nand/r852.c > @@ -7,6 +7,9 @@ > * published by the Free Software Foundation. > */ > > +#define DRV_NAME "r852" > +#define pr_fmt(fmt) DRV_NAME fmt
It would be nicer with:
#define pr_fmt(fmt) DRV_NAME ": " fmt
...
> + > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/jiffies.h> > @@ -935,7 +938,7 @@ static int r852_probe(struct pci_dev *pci_dev, const struct pci_device_id *id) > &dev->card_detect_work, 0); > > > - printk(KERN_NOTICE DRV_NAME ": driver loaded successfully\n"); > + pr_notice(": driver loaded successfully\n");
and then
pr_notice("driver loaded successfully\n");
> return 0; > > error10: > diff --git a/drivers/mtd/nand/r852.h b/drivers/mtd/nand/r852.h > index 8713c57..709eced 100644 > --- a/drivers/mtd/nand/r852.h > +++ b/drivers/mtd/nand/r852.h > @@ -145,16 +145,16 @@ struct r852_device { > }; > > #define DRV_NAME "r852"
You don't need this definition anymore, it's been moved to the .c file. Actually, if you keep it you'll hit a 'redefine' error.
> - > +#define pr_fmt(fmt) DRV_NAME fmt
Why do you redefine it here? The only file including r852.h is r852.c, and pr_fmt() has been defined before all include directives, so when the preprocessor reaches this point pr_fmt() already has a valid definition.
> > #define dbg(format, ...) \ > if (debug) \ > - printk(KERN_DEBUG DRV_NAME ": " format "\n", ## __VA_ARGS__) > + pr_debug(": " format "\n", ## __VA_ARGS__) > > #define dbg_verbose(format, ...) \ > if (debug > 1) \ > - printk(KERN_DEBUG DRV_NAME ": " format "\n", ## __VA_ARGS__) > + pr_debug(": " format "\n", ## __VA_ARGS__)
With the pr_fmt() definition modified as suggested it should be:
pr_debug(format "\n", ## __VA_ARGS__)
Regards,
Boris
[1]https://01.org/linuxgraphics/gfx-docs/drm/admin-guide/dynamic-debug-howto.html
-- Boris Brezillon, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
| |