| Message ID | 1385379031-27766-1-git-send-email-ezequiel.garcia@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Ezequiel, On Mon, Nov 25, 2013 at 08:30:31AM -0300, Ezequiel Garcia wrote: > Add a nice "nand:" prefix to all pr_xxx() messages. This allows > to get rid of the "NAND" words in messages, given the context > is already given by the prefix. This ptach looks mostly good to me. A few comments below. If no one objects soon, I'll push this. > Remove the __func__ report from messages where it's not needed and refactor > the device detection messages to show itself in several lines. There are a few more instances of __func__ that might not be needed, but I think we're OK for now. > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > --- ... > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index bd39f7b..4bdc0df 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c ... > @@ -3372,8 +3374,8 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, > id_data[i] = chip->read_byte(mtd); > > if (id_data[0] != *maf_id || id_data[1] != *dev_id) { > - pr_info("%s: second ID read did not match " > - "%02x,%02x against %02x,%02x\n", __func__, > + pr_info("second ID read did not match " > + "%02x,%02x against %02x,%02x\n", scripts/checkpatch.pl and Documentation/CodingStyle don't like this long string (even though it would be over 80 chars). I think we should combine them. If no one objects to the patch, I'll just make the modification myself. > *maf_id, *dev_id, id_data[0], id_data[1]); > return ERR_PTR(-ENODEV); > } Brian
>From: Brian Norris [mailto:computersforpeace@gmail.com] >>On Mon, Nov 25, 2013 at 08:30:31AM -0300, Ezequiel Garcia wrote: [...] >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c >> index bd39f7b..4bdc0df 100644 >> --- a/drivers/mtd/nand/nand_base.c >> +++ b/drivers/mtd/nand/nand_base.c >... >> @@ -3372,8 +3374,8 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, >> id_data[i] = chip->read_byte(mtd); >> >> if (id_data[0] != *maf_id || id_data[1] != *dev_id) { >> - pr_info("%s: second ID read did not match " >> - "%02x,%02x against %02x,%02x\n", __func__, >> + pr_info("second ID read did not match " >> + "%02x,%02x against %02x,%02x\n", > >scripts/checkpatch.pl and Documentation/CodingStyle don't like this long >string (even though it would be over 80 chars). I think we should combine them. > Yes, breaking the string into multiple prints have a problem that you don't know whether all parts of string get displayed continuously in console log. Or they are scattered and mixed with other device probes logs because device probe can sleep. Thus, we need to justify between: - constrain print log to be within 80 character, (even in world where most modern have gone much beyond 80+ character of display). OR - have logs scattered and mixed with other device probe logs, which makes On-field tracing and debugging difficult. (I like the other part of this patch, prefixing all messages with consistent string). with regards, pekon
On Wed, Dec 04, 2013 at 06:01:37PM -0800, Brian Norris wrote: > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > > index bd39f7b..4bdc0df 100644 > > --- a/drivers/mtd/nand/nand_base.c > > +++ b/drivers/mtd/nand/nand_base.c > ... > > @@ -3372,8 +3374,8 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, > > id_data[i] = chip->read_byte(mtd); > > > > if (id_data[0] != *maf_id || id_data[1] != *dev_id) { > > - pr_info("%s: second ID read did not match " > > - "%02x,%02x against %02x,%02x\n", __func__, > > + pr_info("second ID read did not match " > > + "%02x,%02x against %02x,%02x\n", > > scripts/checkpatch.pl and Documentation/CodingStyle don't like this long > string (even though it would be over 80 chars). I think we should combine them. > You're right, user-visible strings shouldn't be splitted. The 80-char limit has an exception about this (qouting Documentation/CodingStyle): "" Chapter 2: Breaking long lines and strings [..] Statements longer than 80 columns will be broken into sensible chunks, unless exceeding 80 columns significantly increases readability and does not hide information. [..] However, never break user-visible strings such as printk messages, because that breaks the ability to grep for them. "" Regards!
On Wed, Dec 04, 2013 at 06:01:37PM -0800, Brian Norris wrote: > Hi Ezequiel, > > On Mon, Nov 25, 2013 at 08:30:31AM -0300, Ezequiel Garcia wrote: > > Add a nice "nand:" prefix to all pr_xxx() messages. This allows > > to get rid of the "NAND" words in messages, given the context > > is already given by the prefix. [...] > > @@ -3372,8 +3374,8 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, > > id_data[i] = chip->read_byte(mtd); > > > > if (id_data[0] != *maf_id || id_data[1] != *dev_id) { > > - pr_info("%s: second ID read did not match " > > - "%02x,%02x against %02x,%02x\n", __func__, > > + pr_info("second ID read did not match " > > + "%02x,%02x against %02x,%02x\n", > > scripts/checkpatch.pl and Documentation/CodingStyle don't like this long > string (even though it would be over 80 chars). I think we should combine them. > > If no one objects to the patch, I'll just make the modification myself. > > > *maf_id, *dev_id, id_data[0], id_data[1]); > > return ERR_PTR(-ENODEV); > > } Pushed to l2-mtd.git, with the above edit. Brian
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index bd39f7b..4bdc0df 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -29,6 +29,8 @@ * */ +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + #include <linux/module.h> #include <linux/delay.h> #include <linux/errno.h> @@ -2979,7 +2981,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, chip->onfi_version = 10; if (!chip->onfi_version) { - pr_info("%s: unsupported ONFI version: %d\n", __func__, val); + pr_info("unsupported ONFI version: %d\n", val); return 0; } @@ -3372,8 +3374,8 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd, id_data[i] = chip->read_byte(mtd); if (id_data[0] != *maf_id || id_data[1] != *dev_id) { - pr_info("%s: second ID read did not match " - "%02x,%02x against %02x,%02x\n", __func__, + pr_info("second ID read did not match " + "%02x,%02x against %02x,%02x\n", *maf_id, *dev_id, id_data[0], id_data[1]); return ERR_PTR(-ENODEV); } @@ -3440,10 +3442,10 @@ ident_done: * Check, if buswidth is correct. Hardware drivers should set * chip correct! */ - pr_info("NAND device: Manufacturer ID:" - " 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id, - *dev_id, nand_manuf_ids[maf_idx].name, mtd->name); - pr_warn("NAND bus width %d instead %d bit\n", + pr_info("device found, Manufacturer ID: 0x%02x, Chip ID: 0x%02x\n", + *maf_id, *dev_id); + pr_info("%s %s\n", nand_manuf_ids[maf_idx].name, mtd->name); + pr_warn("bus width %d instead %d bit\n", (chip->options & NAND_BUSWIDTH_16) ? 16 : 8, busw ? 16 : 8); return ERR_PTR(-EINVAL); @@ -3472,14 +3474,13 @@ ident_done: if (mtd->writesize > 512 && chip->cmdfunc == nand_command) chip->cmdfunc = nand_command_lp; - pr_info("NAND device: Manufacturer ID: 0x%02x, Chip ID: 0x%02x (%s %s)\n", - *maf_id, *dev_id, nand_manuf_ids[maf_idx].name, + pr_info("device found, Manufacturer ID: 0x%02x, Chip ID: 0x%02x\n", + *maf_id, *dev_id); + pr_info("%s %s\n", nand_manuf_ids[maf_idx].name, chip->onfi_version ? chip->onfi_params.model : type->name); - - pr_info("NAND device: %dMiB, %s, page size: %d, OOB size: %d\n", + pr_info("%dMiB, %s, page size: %d, OOB size: %d\n", (int)(chip->chipsize >> 20), nand_is_slc(chip) ? "SLC" : "MLC", mtd->writesize, mtd->oobsize); - return type; } @@ -3535,7 +3536,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips, chip->select_chip(mtd, -1); } if (i > 1) - pr_info("%d NAND chips detected\n", i); + pr_info("%d chips detected\n", i); /* Store the number of chips and calc total size for mtd */ chip->numchips = i;
Add a nice "nand:" prefix to all pr_xxx() messages. This allows to get rid of the "NAND" words in messages, given the context is already given by the prefix. Remove the __func__ report from messages where it's not needed and refactor the device detection messages to show itself in several lines. Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> --- Currently, we report found devices with: NAND device: Manufacturer ID: 0xec, Chip ID: 0xdc (Samsung NAND 512MiB 3,3V 8-bit) NAND device: 512MiB, SLC, page size: 2048, OOB size: 64 And after this change, the above is: nand: device found, Manufacturer ID: 0xec, Chip ID: 0xdc nand: Samsung NAND 512MiB 3,3V 8-bit nand: 512MiB, SLC, page size: 2048, OOB size: 64 drivers/mtd/nand/nand_base.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-)