From: josh.wu@atmel•com (Josh Wu)
To: linux-arm-kernel@lists•infradead.org
Subject: [RFC PATCH v2] MTD: nand: add return value for write_page()/write_page_raw() functions in structure of nand_ecc_ctrl.
Date: Mon, 25 Jun 2012 18:16:06 +0800 [thread overview]
Message-ID: <4FE83A66.9090100@atmel.com> (raw)
In-Reply-To: <CAN8TOE9ebFAfJHaG2-po0LeLi=wTq9YZ9pNFXhQE0PLt5TQxeg@mail.gmail.com>
Hi, Brian
On 6/23/2012 3:32 AM, Brian Norris wrote:
> Hi Josh,
>
> On Wed, Jun 20, 2012 at 9:02 PM, Josh Wu <josh.wu@atmel•com> wrote:
>> On 6/21/2012 2:43 AM, Brian Norris wrote:
>>> On Tue, Jun 19, 2012 at 8:00 PM, Josh Wu<josh.wu@atmel•com> wrote:
>>>> On 6/13/2012 2:06 PM, Josh Wu wrote:
>>>>> There is an implemention of hardware ECC write page function which may
>>>>> return an error indication. But now the definition of 'write_page'
>>>>> function
>>>>> in struct nand_ecc_ctrl is 'void'.
>>> I think it would help reviewers (and changelog readers) to note which
>>> implementations are the real issue, if there are a small number of
>>> implementations targeted.
>> Currently, I am introducing the atmel pragrammable multibit ECC(PMECC)
>> hardware code to nand flash. I meet following situation that I need change
>> the write_page()'s return value to 'int':
>>
>> when writing one page into a nand flash with PMECC enabled, the hardware
>> engine will compute the BCH ecc code for this page. so we need read a the
>> status register to theck whether the ecc code is generated.
>> But we cannot assume the status register always can be ready, (for instance,
>> incorrect hardware configuration or hardware issue), in such case I need
>> write_page() to return a error code.
>>
>> So this is the reason that I push this patch to change the return value to
>> int.
> OK, thanks for the clarification here. I think that this is valuable
> information that should be included in the change log when you send
> v3. Also, this is the kind of fix that should probably be sent in a
> series with the patch for your Atmel PMECC driver. That way, they can
> be reviewed/accepted together. As you mention, it is pointless to
> merge this patch without your driver patch.
>
>>>>> --- a/drivers/mtd/nand/nand_base.c
>>>>> +++ b/drivers/mtd/nand/nand_base.c
>>> ...
>>>>> @@ -2096,9 +2104,14 @@ static int nand_write_page(struct mtd_info *mtd,
>>>>> struct nand_chip *chip,
>>>>> chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);
>>>>>
>>>>> if (unlikely(raw))
>>>>> - chip->ecc.write_page_raw(mtd, chip, buf, oob_required);
>>>>> + status = chip->ecc.write_page_raw(mtd, chip, buf, oob_required);
>>>>> else
>>>>> - chip->ecc.write_page(mtd, chip, buf, oob_required);
>>>>> + status = chip->ecc.write_page(mtd, chip, buf, oob_required);
>>>>> +
>>>>> + if (status< 0) {
>>>>> + pr_warn("Error happened when calling nand_write_page()\n");
>>>>> + return status;
>>>>> + }
>>> I'm not sure this is the most informative error message. (Similar
>>> comment applies in cafe_nand.c, which imitates a lot of nand_base.c
>>> code)
>> Maybe in this case I need print the error code as well.
> After a little closer look, I don't think you need to print anything
> (here, or in cafe_nand.c). This is a kind of intermediate-layer
> function that doesn't print anything on other errors (e.g., when it
> checks the status, it just fails with 'return -EIO;'). The error will
> be caught by upper layers and an appropriate error code may or may not
> be displayed. So just make sure that your new driver asserts an
> appropriate error code, and no print should be necessary.
>
> Regards,
> Brian
According to your advice, I resent a new patch series, which merge this
patch with PMECC.
Thank you.
Best Regards,
Josh Wu
next prev parent reply other threads:[~2012-06-25 10:16 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-06-13 6:06 [RFC PATCH v2] MTD: nand: add return value for write_page()/write_page_raw() functions in structure of nand_ecc_ctrl Josh Wu
2012-06-20 3:00 ` Josh Wu
2012-06-20 18:43 ` Brian Norris
2012-06-21 4:02 ` Josh Wu
2012-06-22 19:32 ` Brian Norris
2012-06-25 10:16 ` Josh Wu [this message]
2012-07-02 16:39 ` Brian Norris
2012-06-21 17:58 ` Mike Dunn
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=4FE83A66.9090100@atmel.com \
--to=josh.wu@atmel$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox