Hi Bean,
I see that you have sent this several times in a relatively short
period, and I see that there are several comments you haven't addressed
[1] [2].
It is very difficult for me to apply your patch. It seems to be an
invalid patch (I think the line counts are wrong), so it doesn't apply
with git-am. Christian already had this problem in [1]. Can you try
sending mail with git-send-email? It will probably do better than your
current mailer. If you're having any doubt, try sending the patch to
yourself and then applying it with git-am.
Documentation/email-clients.txt also has some tips for email.
Your patch also has several coding style issues, one of which Christian
also noticed, in [2]. (Thanks for reviewing, BTW, Chrisitan!)
Can you fix the subject to follow other patches' style? Like:
mtd: cfi_cmdset_0002: ...
(I can fix one or two patches for these kind of things, but you said you
plan to send several more patches.)
On Mon, Jun 30, 2014 at 07:56:37AM +0000, Bean Huo 霍斌斌 (beanhuo) wrote:
> The size of the buffer program has been increased from 256 to 512 , 2ms
> maximum timeout for do_write_buffer can not adapt to all the different
> vendor's norflash.There maximum timeout information in the CFI area,so the
> best way is to choose the result calculated according to timeout field of
> struct cfi_ident that probed from norflash's CFI aera.This is also a standard
> defined by CFI.
Please line-wrap your commit descriptions. This line is >300 characters
long.
>
> Without this change, if the size of buffer program is 512 or bigger than 256,
> due to timeout is the shorter than that the chip required,do_write_buffer
> sometimes fails.
>
> Tested with Micron JS28F512M29EWx and Micron MT28EW512ABA flash devices.
>
> Signed-off-by: bean huo <bean...@micron.com>
> ---
> changes
> v1->v2:Deleted unused parameters in this patch (word_write_time_max and
> erase_time_max).
> Using usecs_to_jiffies instead of msecs_to_jiffies for convert timeout value
> into jiffies.
> v2->v3:Removed unnecessary messages form comments and deleted trailing
> whitespace.
>
> drivers/mtd/chips/cfi_cmdset_0002.c | 27 ++++++++++++++++++++++++---
> 1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c
> b/drivers/mtd/chips/cfi_cmdset_0002.c
> index e21fde9..f7098d6 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -628,10 +628,24 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map,
> int primary)
> cfi->chips[i].word_write_time =
> 1<<cfi->cfiq->WordWriteTimeoutTyp;
> cfi->chips[i].buffer_write_time =
> 1<<cfi->cfiq->BufWriteTimeoutTyp;
> cfi->chips[i].erase_time = 1<<cfi->cfiq->BlockEraseTimeoutTyp;
> + /*
> + * We first calculate the timeout max according to timeout
> + * field of struct cfi_ident that probed from chip's CFI
> + * aera,If haven't probed this information,we will specify
> + * a default value,and the time unit is us.
> + */
> + if (cfi->cfiq->BufWriteTimeoutTyp &&
> + cfi->cfiq->BufWriteTimeoutMax){
Can you put a space before the brace?
> + cfi->chips[i].buffer_write_time_max =
> + 1<<(cfi->cfiq->BufWriteTimeoutTyp +
Please put spaces around the shift operator.
> + cfi->cfiq->BufWriteTimeoutMax);
> + } else {
> + /* specify maximum timeout for buffer program 2000us */
> + cfi->chips[i].buffer_write_time_max = 2000;
> + }
The indentation of this entire 'else' block is wrong.
> cfi->chips[i].ref_point_counter = 0;
> init_waitqueue_head(&(cfi->chips[i].wq));
> }
> -
Please don't remove this line.
> map->fldrv = &cfi_amdstd_chipdrv;
>
> return cfi_amdstd_setup(mtd);
> @@ -1462,8 +1476,15 @@ static int __xipram do_write_buffer(struct map_info
> *map, struct flchip *chip, {
> struct cfi_private *cfi = map->fldrv_priv;
> unsigned long timeo = jiffies + HZ;
> - /* see comments in do_write_oneword() regarding uWriteTimeo. */
> - unsigned long uWriteTimeout = ( HZ / 1000 ) + 1;
> + /* There maximum timeout information in the CFI area,so the
> + * best way is to choose the result calculated according to
> + * timeout field of struct cfi_ident that probed from
> + * norflash's CFI aera,see comments in cfi_cmdset_0002().
> + * uWriteTimeout is used for timeout step,it must be concerted
> + * into jiffies.
Some of the language here needs improvement, but I can spruce it up once
your patch is ready. Just fix the style up a bit, and make sure the
patch can apply, then I'll take care of the rest. Thanks!
> + */
> + unsigned long uWriteTimeout =
> + usecs_to_jiffies(chip->buffer_write_time_max);
> int ret = -EIO;
> unsigned long cmd_adr;
> int z, words;
> --
> 1.7.9.5
Brian
[1] http://lists.infradead.org/pipermail/linux-mtd/2014-June/054362.html
[2] http://lists.infradead.org/pipermail/linux-mtd/2014-June/054367.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
