All of lore.kernel.org
 help / color / mirror / Atom feed
From: Artem Bityutskiy <dedekind1@gmail.com>
To: Brian Norris <computersforpeace@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>,
	linux-mtd@lists.infradead.org,
	Igor Grinberg <grinberg@compulab.co.il>
Subject: Re: [PATCH 2/4] mtd: nand: convert printk() to pr_*()
Date: Wed, 08 Jun 2011 17:43:47 +0300	[thread overview]
Message-ID: <1307544227.31223.115.camel@localhost> (raw)
In-Reply-To: <1307487717-25402-3-git-send-email-computersforpeace@gmail.com>

Brian, thanks for the patch, it certainly makes the code nicer and a bit
more readable. But I have few requests.

On Tue, 2011-06-07 at 16:01 -0700, Brian Norris wrote:
> -		printk(KERN_NOTICE "%s: Attempt to write not "
> +		pr_notice("%s: Attempt to write not "
>  				"page aligned data\n", __func__);

OK, so with this change the string becomes shorter and you do not have
to split it any more. You should make it look like this:

               pr_notice("%s: attempt to write not page aligned data\n",
                          __func__);
 
This is more readable and preferable - no string split. Please, do this
for other messages too.

Also, while on it, may be you could unify the messages and make them:

1. start with small letter if there is a function name prerix -
   currently some start with small and some start with capital.
2. if there is a dot at the end - remove it.


> @@ -2561,7 +2561,7 @@ int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
>  		/* Heck if we have a bad block, we do not erase bad blocks! */
>  		if (nand_block_checkbad(mtd, ((loff_t) page) <<
>  					chip->page_shift, 0, allowbbt)) {
> -			printk(KERN_WARNING "%s: attempt to erase a bad block "
> +			pr_warning("%s: attempt to erase a bad block "
>  					"at page 0x%08x\n", __func__, page);

Since you now have more space, I think it is a bit better to re-wrap the
message, i.e., make it like this:

                       pr_warning("%s: attempt to erase a bad block at page "
                                  "0x%08x\n", __func__, page);

or, if you prefer, even like this:

                       pr_warning("%s: attempt to erase a bad block at page 0x%08x\n",
                                   __func__, page);
 
because the coding style does allow strings longer than 80 characters if
"exceeding 80 columns significantly increases readability".

Ditto for the rest. With this, your clean-up will make the code even
cleaner :-)

Thanks!

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

  reply	other threads:[~2011-06-08 14:48 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-06-07 23:01 [PATCH 0/4] debug, printk cleanup Brian Norris
2011-06-07 23:01 ` [PATCH 1/4] mtd: remove printk's for [kv][mz]alloc failures Brian Norris
2011-06-08 14:27   ` Artem Bityutskiy
2011-06-07 23:01 ` [PATCH 2/4] mtd: nand: convert printk() to pr_*() Brian Norris
2011-06-08 14:43   ` Artem Bityutskiy [this message]
2011-06-08 18:25     ` [PATCH v2 " Brian Norris
2011-06-09  6:40       ` Artem Bityutskiy
2011-06-09  6:53       ` Artem Bityutskiy
2011-06-09  6:59         ` Artem Bityutskiy
2011-06-09  7:44       ` Artem Bityutskiy
2011-06-09 16:00         ` Brian Norris
2011-06-09 16:03           ` Artem Bityutskiy
2011-06-13 18:24             ` Brian Norris
2011-06-22  4:40               ` Artem Bityutskiy
2011-06-22  9:12                 ` Dmitry Eremin-Solenikov
2011-07-06 18:51                   ` Brian Norris
2011-07-06 19:12                     ` Brian Norris
2011-07-06 19:47                       ` Dmitry Eremin-Solenikov
2011-07-06 19:59                         ` Brian Norris
2011-07-07  6:58                     ` Artem Bityutskiy
2011-07-07 17:00                       ` Brian Norris
2011-07-07 19:56                         ` Artem Bityutskiy
2011-07-08 16:06                           ` Brian Norris
2011-07-20  3:59                             ` Artem Bityutskiy
2011-07-07  7:01               ` Artem Bityutskiy
2011-07-07 17:01                 ` Brian Norris
2011-06-09  8:13       ` Artem Bityutskiy
2011-06-09 16:22         ` Brian Norris
2011-06-10 18:25           ` Brian Norris
2011-06-07 23:01 ` [PATCH 3/4] mtd: nand: replace DEBUG() with dev_dbg() Brian Norris
2011-06-08 14:44   ` Artem Bityutskiy
2011-06-08 18:27     ` [PATCH v2 " Brian Norris
2011-06-09  6:46       ` Artem Bityutskiy
2011-06-07 23:01 ` [PATCH 4/4] mtd: nand: define pr_fmt() to include __func__ in debug output Brian Norris
2011-06-08 14:47   ` Artem Bityutskiy
2011-06-08 18:23     ` Brian Norris
2011-06-08 18:28       ` [PATCH v2 " Brian Norris
2011-06-09  7:10         ` Artem Bityutskiy
2011-06-08 19:03       ` [PATCH " Mike Frysinger

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1307544227.31223.115.camel@localhost \
    --to=dedekind1@gmail.com \
    --cc=computersforpeace@gmail.com \
    --cc=dwmw2@infradead.org \
    --cc=grinberg@compulab.co.il \
    --cc=linux-mtd@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.