| Message ID | 1339567570-4816-1-git-send-email-josh.wu@atmel.com |
|---|---|
| State | RFC |
| Headers | show |
Hi, Artem Ping? Do you have any comments for this patch? Best Regards, Josh Wu 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'. > This patch change the definition of 'write_page' fuction to return an 'int' value. It adds code to test the return value, and if negative, indicate an error happend when write page with ECC. > > To be consistent, it also changes the 'write_page_raw' function's return type from 'void' to 'int'. > > This patch also adds 'return 0' for all implementation for the ecc 'write_page' and 'write_page_raw' functions. > > Note: I couldn't compile-test all of these easily, as some had ARCH dependencies. > > Signed-off-by: Josh Wu<josh.wu@atmel.com> > --- > drivers/mtd/nand/bcm_umi_bch.c | 6 ++++-- > drivers/mtd/nand/bf5xx_nand.c | 6 ++++-- > drivers/mtd/nand/cafe_nand.c | 13 ++++++++++--- > drivers/mtd/nand/denali.c | 12 +++++++----- > drivers/mtd/nand/docg4.c | 8 +++++--- > drivers/mtd/nand/fsl_elbc_nand.c | 4 +++- > drivers/mtd/nand/fsl_ifc_nand.c | 4 +++- > drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 6 ++++-- > drivers/mtd/nand/nand_base.c | 29 +++++++++++++++++++++-------- > drivers/mtd/nand/pxa3xx_nand.c | 4 +++- > drivers/mtd/nand/sh_flctl.c | 4 +++- > include/linux/mtd/nand.h | 4 ++-- > 12 files changed, 69 insertions(+), 31 deletions(-) > > diff --git a/drivers/mtd/nand/bcm_umi_bch.c b/drivers/mtd/nand/bcm_umi_bch.c > index 5914bb3..c8799a0 100644 > --- a/drivers/mtd/nand/bcm_umi_bch.c > +++ b/drivers/mtd/nand/bcm_umi_bch.c > @@ -23,7 +23,7 @@ > /* ---- Private Function Prototypes -------------------------------------- */ > static int bcm_umi_bch_read_page_hwecc(struct mtd_info *mtd, > struct nand_chip *chip, uint8_t *buf, int oob_required, int page); > -static void bcm_umi_bch_write_page_hwecc(struct mtd_info *mtd, > +static int bcm_umi_bch_write_page_hwecc(struct mtd_info *mtd, > struct nand_chip *chip, const uint8_t *buf, int oob_required); > > /* ---- Private Variables ------------------------------------------------ */ > @@ -194,7 +194,7 @@ static int bcm_umi_bch_read_page_hwecc(struct mtd_info *mtd, > * @oob_required: must write chip->oob_poi to OOB > * > ***************************************************************************/ > -static void bcm_umi_bch_write_page_hwecc(struct mtd_info *mtd, > +static int bcm_umi_bch_write_page_hwecc(struct mtd_info *mtd, > struct nand_chip *chip, const uint8_t *buf, int oob_required) > { > int sectorIdx = 0; > @@ -214,4 +214,6 @@ static void bcm_umi_bch_write_page_hwecc(struct mtd_info *mtd, > } > > bcm_umi_nand_write_buf(mtd, chip->oob_poi, mtd->oobsize); > + > + return 0; > } > diff --git a/drivers/mtd/nand/bf5xx_nand.c b/drivers/mtd/nand/bf5xx_nand.c > index 3f1c185..ab0caa7 100644 > --- a/drivers/mtd/nand/bf5xx_nand.c > +++ b/drivers/mtd/nand/bf5xx_nand.c > @@ -566,11 +566,13 @@ static int bf5xx_nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip > return 0; > } > > -static void bf5xx_nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, > - const uint8_t *buf, int oob_required) > +static int bf5xx_nand_write_page_raw(struct mtd_info *mtd, > + struct nand_chip *chip, const uint8_t *buf, int oob_required) > { > bf5xx_nand_write_buf(mtd, buf, mtd->writesize); > bf5xx_nand_write_buf(mtd, chip->oob_poi, mtd->oobsize); > + > + return 0; > } > > /* > diff --git a/drivers/mtd/nand/cafe_nand.c b/drivers/mtd/nand/cafe_nand.c > index 41371ba..3c92a98 100644 > --- a/drivers/mtd/nand/cafe_nand.c > +++ b/drivers/mtd/nand/cafe_nand.c > @@ -520,7 +520,7 @@ static struct nand_bbt_descr cafe_bbt_mirror_descr_512 = { > }; > > > -static void cafe_nand_write_page_lowlevel(struct mtd_info *mtd, > +static int cafe_nand_write_page_lowlevel(struct mtd_info *mtd, > struct nand_chip *chip, > const uint8_t *buf, int oob_required) > { > @@ -531,6 +531,8 @@ static void cafe_nand_write_page_lowlevel(struct mtd_info *mtd, > > /* Set up ECC autogeneration */ > cafe->ctl2 |= (1<<30); > + > + return 0; > } > > static int cafe_nand_write_page(struct mtd_info *mtd, struct nand_chip *chip, > @@ -542,9 +544,14 @@ static int cafe_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 cafe_nand_write_page()\n"); > + return status; > + } > > /* > * Cached progamming disabled for now, Not sure if its worth the > diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c > index 0650aaf..e706a23 100644 > --- a/drivers/mtd/nand/denali.c > +++ b/drivers/mtd/nand/denali.c > @@ -1028,7 +1028,7 @@ static void denali_setup_dma(struct denali_nand_info *denali, int op) > > /* writes a page. user specifies type, and this function handles the > * configuration details. */ > -static void write_page(struct mtd_info *mtd, struct nand_chip *chip, > +static int write_page(struct mtd_info *mtd, struct nand_chip *chip, > const uint8_t *buf, bool raw_xfer) > { > struct denali_nand_info *denali = mtd_to_denali(mtd); > @@ -1078,6 +1078,8 @@ static void write_page(struct mtd_info *mtd, struct nand_chip *chip, > > denali_enable_dma(denali, false); > dma_sync_single_for_cpu(denali->dev, addr, size, DMA_TO_DEVICE); > + > + return 0; > } > > /* NAND core entry points */ > @@ -1086,24 +1088,24 @@ static void write_page(struct mtd_info *mtd, struct nand_chip *chip, > * writing a page with ECC or without is similar, all the work is done > * by write_page above. > * */ > -static void denali_write_page(struct mtd_info *mtd, struct nand_chip *chip, > +static int denali_write_page(struct mtd_info *mtd, struct nand_chip *chip, > const uint8_t *buf, int oob_required) > { > /* for regular page writes, we let HW handle all the ECC > * data written to the device. */ > - write_page(mtd, chip, buf, false); > + return write_page(mtd, chip, buf, false); > } > > /* This is the callback that the NAND core calls to write a page without ECC. > * raw access is similar to ECC page writes, so all the work is done in the > * write_page() function above. > */ > -static void denali_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, > +static int denali_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, > const uint8_t *buf, int oob_required) > { > /* for raw page writes, we want to disable ECC and simply write > whatever data is in the buffer. */ > - write_page(mtd, chip, buf, true); > + return write_page(mtd, chip, buf, true); > } > > static int denali_write_oob(struct mtd_info *mtd, struct nand_chip *chip, > diff --git a/drivers/mtd/nand/docg4.c b/drivers/mtd/nand/docg4.c > index a225e49..0f2ffd7 100644 > --- a/drivers/mtd/nand/docg4.c > +++ b/drivers/mtd/nand/docg4.c > @@ -898,7 +898,7 @@ static void docg4_erase_block(struct mtd_info *mtd, int page) > write_nop(docptr); > } > > -static void write_page(struct mtd_info *mtd, struct nand_chip *nand, > +static int write_page(struct mtd_info *mtd, struct nand_chip *nand, > const uint8_t *buf, bool use_ecc) > { > struct docg4_priv *doc = nand->priv; > @@ -950,15 +950,17 @@ static void write_page(struct mtd_info *mtd, struct nand_chip *nand, > write_nop(docptr); > writew(0, docptr + DOC_DATAEND); > write_nop(docptr); > + > + return 0; > } > > -static void docg4_write_page_raw(struct mtd_info *mtd, struct nand_chip *nand, > +static int docg4_write_page_raw(struct mtd_info *mtd, struct nand_chip *nand, > const uint8_t *buf, int oob_required) > { > return write_page(mtd, nand, buf, false); > } > > -static void docg4_write_page(struct mtd_info *mtd, struct nand_chip *nand, > +static int docg4_write_page(struct mtd_info *mtd, struct nand_chip *nand, > const uint8_t *buf, int oob_required) > { > return write_page(mtd, nand, buf, true); > diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c > index 7842938..35d731d 100644 > --- a/drivers/mtd/nand/fsl_elbc_nand.c > +++ b/drivers/mtd/nand/fsl_elbc_nand.c > @@ -766,11 +766,13 @@ static int fsl_elbc_read_page(struct mtd_info *mtd, struct nand_chip *chip, > /* ECC will be calculated automatically, and errors will be detected in > * waitfunc. > */ > -static void fsl_elbc_write_page(struct mtd_info *mtd, struct nand_chip *chip, > +static int fsl_elbc_write_page(struct mtd_info *mtd, struct nand_chip *chip, > const uint8_t *buf, int oob_required) > { > fsl_elbc_write_buf(mtd, buf, mtd->writesize); > fsl_elbc_write_buf(mtd, chip->oob_poi, mtd->oobsize); > + > + return 0; > } > > static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv) > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c > index 9602c1b..47ba534 100644 > --- a/drivers/mtd/nand/fsl_ifc_nand.c > +++ b/drivers/mtd/nand/fsl_ifc_nand.c > @@ -721,11 +721,13 @@ static int fsl_ifc_read_page(struct mtd_info *mtd, struct nand_chip *chip, > /* ECC will be calculated automatically, and errors will be detected in > * waitfunc. > */ > -static void fsl_ifc_write_page(struct mtd_info *mtd, struct nand_chip *chip, > +static int fsl_ifc_write_page(struct mtd_info *mtd, struct nand_chip *chip, > const uint8_t *buf, int oob_required) > { > fsl_ifc_write_buf(mtd, buf, mtd->writesize); > fsl_ifc_write_buf(mtd, chip->oob_poi, mtd->oobsize); > + > + return 0; > } > > static int fsl_ifc_chip_init_tail(struct mtd_info *mtd) > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > index a05b7b4..3bda330 100644 > --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > @@ -930,7 +930,7 @@ exit_nfc: > return ret; > } > > -static void gpmi_ecc_write_page(struct mtd_info *mtd, struct nand_chip *chip, > +static int gpmi_ecc_write_page(struct mtd_info *mtd, struct nand_chip *chip, > const uint8_t *buf, int oob_required) > { > struct gpmi_nand_data *this = chip->priv; > @@ -972,7 +972,7 @@ static void gpmi_ecc_write_page(struct mtd_info *mtd, struct nand_chip *chip, > &payload_virt,&payload_phys); > if (ret) { > pr_err("Inadequate payload DMA buffer\n"); > - return; > + return 0; > } > > ret = send_page_prepare(this, > @@ -1002,6 +1002,8 @@ exit_auxiliary: > nfc_geo->payload_size, > payload_virt, payload_phys); > } > + > + return 0; > } > > /* > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index d47586c..ad9e9a9 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -1927,12 +1927,14 @@ out: > * > * Not for syndrome calculating ECC controllers, which use a special oob layout. > */ > -static void nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, > +static int nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, > const uint8_t *buf, int oob_required) > { > chip->write_buf(mtd, buf, mtd->writesize); > if (oob_required) > chip->write_buf(mtd, chip->oob_poi, mtd->oobsize); > + > + return 0; > } > > /** > @@ -1944,7 +1946,7 @@ static void nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, > * > * We need a special oob layout and handling even when ECC isn't checked. > */ > -static void nand_write_page_raw_syndrome(struct mtd_info *mtd, > +static int nand_write_page_raw_syndrome(struct mtd_info *mtd, > struct nand_chip *chip, > const uint8_t *buf, int oob_required) > { > @@ -1974,6 +1976,8 @@ static void nand_write_page_raw_syndrome(struct mtd_info *mtd, > size = mtd->oobsize - (oob - chip->oob_poi); > if (size) > chip->write_buf(mtd, oob, size); > + > + return 0; > } > /** > * nand_write_page_swecc - [REPLACEABLE] software ECC based page write function > @@ -1982,7 +1986,7 @@ static void nand_write_page_raw_syndrome(struct mtd_info *mtd, > * @buf: data buffer > * @oob_required: must write chip->oob_poi to OOB > */ > -static void nand_write_page_swecc(struct mtd_info *mtd, struct nand_chip *chip, > +static int nand_write_page_swecc(struct mtd_info *mtd, struct nand_chip *chip, > const uint8_t *buf, int oob_required) > { > int i, eccsize = chip->ecc.size; > @@ -1999,7 +2003,7 @@ static void nand_write_page_swecc(struct mtd_info *mtd, struct nand_chip *chip, > for (i = 0; i< chip->ecc.total; i++) > chip->oob_poi[eccpos[i]] = ecc_calc[i]; > > - chip->ecc.write_page_raw(mtd, chip, buf, 1); > + return chip->ecc.write_page_raw(mtd, chip, buf, 1); > } > > /** > @@ -2009,7 +2013,7 @@ static void nand_write_page_swecc(struct mtd_info *mtd, struct nand_chip *chip, > * @buf: data buffer > * @oob_required: must write chip->oob_poi to OOB > */ > -static void nand_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, > +static int nand_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, > const uint8_t *buf, int oob_required) > { > int i, eccsize = chip->ecc.size; > @@ -2029,6 +2033,8 @@ static void nand_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, > chip->oob_poi[eccpos[i]] = ecc_calc[i]; > > chip->write_buf(mtd, chip->oob_poi, mtd->oobsize); > + > + return 0; > } > > /** > @@ -2041,7 +2047,7 @@ static void nand_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, > * The hw generator calculates the error syndrome automatically. Therefore we > * need a special oob layout and handling. > */ > -static void nand_write_page_syndrome(struct mtd_info *mtd, > +static int nand_write_page_syndrome(struct mtd_info *mtd, > struct nand_chip *chip, > const uint8_t *buf, int oob_required) > { > @@ -2075,6 +2081,8 @@ static void nand_write_page_syndrome(struct mtd_info *mtd, > i = mtd->oobsize - (oob - chip->oob_poi); > if (i) > chip->write_buf(mtd, oob, i); > + > + return 0; > } > > /** > @@ -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; > + } > > /* > * Cached progamming disabled for now. Not sure if it's worth the > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c > index 252aaef..86640f7 100644 > --- a/drivers/mtd/nand/pxa3xx_nand.c > +++ b/drivers/mtd/nand/pxa3xx_nand.c > @@ -681,11 +681,13 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command, > info->state = STATE_IDLE; > } > > -static void pxa3xx_nand_write_page_hwecc(struct mtd_info *mtd, > +static int pxa3xx_nand_write_page_hwecc(struct mtd_info *mtd, > struct nand_chip *chip, const uint8_t *buf, int oob_required) > { > chip->write_buf(mtd, buf, mtd->writesize); > chip->write_buf(mtd, chip->oob_poi, mtd->oobsize); > + > + return 0; > } > > static int pxa3xx_nand_read_page_hwecc(struct mtd_info *mtd, > diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c > index aa9b8a5..0700ad9 100644 > --- a/drivers/mtd/nand/sh_flctl.c > +++ b/drivers/mtd/nand/sh_flctl.c > @@ -365,7 +365,7 @@ static int flctl_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, > return 0; > } > > -static void flctl_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, > +static int flctl_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, > const uint8_t *buf, int oob_required) > { > int i, eccsize = chip->ecc.size; > @@ -375,6 +375,8 @@ static void flctl_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, > > for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) > chip->write_buf(mtd, p, eccsize); > + > + return 0; > } > > static void execmd_read_page_sector(struct mtd_info *mtd, int page_addr) > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > index 57977c6..4cb6c7fe 100644 > --- a/include/linux/mtd/nand.h > +++ b/include/linux/mtd/nand.h > @@ -361,13 +361,13 @@ struct nand_ecc_ctrl { > uint8_t *calc_ecc); > int (*read_page_raw)(struct mtd_info *mtd, struct nand_chip *chip, > uint8_t *buf, int oob_required, int page); > - void (*write_page_raw)(struct mtd_info *mtd, struct nand_chip *chip, > + int (*write_page_raw)(struct mtd_info *mtd, struct nand_chip *chip, > const uint8_t *buf, int oob_required); > int (*read_page)(struct mtd_info *mtd, struct nand_chip *chip, > uint8_t *buf, int oob_required, int page); > int (*read_subpage)(struct mtd_info *mtd, struct nand_chip *chip, > uint32_t offs, uint32_t len, uint8_t *buf); > - void (*write_page)(struct mtd_info *mtd, struct nand_chip *chip, > + int (*write_page)(struct mtd_info *mtd, struct nand_chip *chip, > const uint8_t *buf, int oob_required); > int (*write_oob_raw)(struct mtd_info *mtd, struct nand_chip *chip, > int page);
Hi Josh, On Tue, Jun 19, 2012 at 8:00 PM, Josh Wu <josh.wu@atmel.com> wrote: > Hi, Artem > > Ping? Do you have any comments for this patch? I'm not Artem, but I have some comments :) > 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. I noticed, for instance, that docg4.c has some strange code involving a return in a void function (comment below). If that is the *only* existing return statement within an 'ecc_ctrl.write_page' implmentation, then this whole patch is unneeded; you can just remove the 'return' in docg4.c. >> diff --git a/drivers/mtd/nand/docg4.c b/drivers/mtd/nand/docg4.c >> index a225e49..0f2ffd7 100644 >> --- a/drivers/mtd/nand/docg4.c >> +++ b/drivers/mtd/nand/docg4.c ... >> -static void docg4_write_page_raw(struct mtd_info *mtd, struct nand_chip >> *nand, >> +static int docg4_write_page_raw(struct mtd_info *mtd, struct nand_chip >> *nand, >> const uint8_t *buf, int oob_required) >> { >> return write_page(mtd, nand, buf, false); >> } Hmm, this used to be a void function, returning the result of another void function? I would think the compiler would have warned about these issues before. Anyway, this is a useless 'return' statement and can be killed with no real effect. >> -static void docg4_write_page(struct mtd_info *mtd, struct nand_chip >> *nand, >> +static int docg4_write_page(struct mtd_info *mtd, struct nand_chip *nand, >> const uint8_t *buf, int oob_required) >> { >> return write_page(mtd, nand, buf, true); Ditto. Are these the only "issues" being addressed in this 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) Brian
Hi, Brian On 6/21/2012 2:43 AM, Brian Norris wrote: > Hi Josh, > > On Tue, Jun 19, 2012 at 8:00 PM, Josh Wu<josh.wu@atmel.com> wrote: >> Hi, Artem >> >> Ping? Do you have any comments for this patch? > I'm not Artem, but I have some comments :) sure, thanks for the comments. :) > >> 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. > I noticed, for instance, that docg4.c has some strange code involving > a return in a void function (comment below). If that is the *only* > existing return statement within an 'ecc_ctrl.write_page' > implmentation, then this whole patch is unneeded; you can just remove > the 'return' in docg4.c. > >>> diff --git a/drivers/mtd/nand/docg4.c b/drivers/mtd/nand/docg4.c >>> index a225e49..0f2ffd7 100644 >>> --- a/drivers/mtd/nand/docg4.c >>> +++ b/drivers/mtd/nand/docg4.c > ... >>> -static void docg4_write_page_raw(struct mtd_info *mtd, struct nand_chip >>> *nand, >>> +static int docg4_write_page_raw(struct mtd_info *mtd, struct nand_chip >>> *nand, >>> const uint8_t *buf, int oob_required) >>> { >>> return write_page(mtd, nand, buf, false); >>> } > Hmm, this used to be a void function, returning the result of another > void function? I would think the compiler would have warned about > these issues before. Anyway, this is a useless 'return' statement and > can be killed with no real effect. In original version seems the 'return' is useless. I change this only because the write_page() function's return type is changed. > >>> -static void docg4_write_page(struct mtd_info *mtd, struct nand_chip >>> *nand, >>> +static int docg4_write_page(struct mtd_info *mtd, struct nand_chip *nand, >>> const uint8_t *buf, int oob_required) >>> { >>> return write_page(mtd, nand, buf, true); > Ditto. Are these the only "issues" being addressed in this patch? No. This is not the reason for this patch as I said above. > >>> --- 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. > > Brian Best Regards, Josh Wu
On 06/20/2012 11:43 AM, Brian Norris wrote: > > I noticed, for instance, that docg4.c has some strange code involving > a return in a void function (comment below). If that is the *only* > existing return statement within an 'ecc_ctrl.write_page' > implmentation, then this whole patch is unneeded; you can just remove > the 'return' in docg4.c. > >>> diff --git a/drivers/mtd/nand/docg4.c b/drivers/mtd/nand/docg4.c >>> index a225e49..0f2ffd7 100644 >>> --- a/drivers/mtd/nand/docg4.c >>> +++ b/drivers/mtd/nand/docg4.c > ... >>> -static void docg4_write_page_raw(struct mtd_info *mtd, struct nand_chip >>> *nand, >>> +static int docg4_write_page_raw(struct mtd_info *mtd, struct nand_chip >>> *nand, >>> const uint8_t *buf, int oob_required) >>> { >>> return write_page(mtd, nand, buf, false); >>> } > > Hmm, this used to be a void function, returning the result of another > void function? I would think the compiler would have warned about > these issues before. Anyway, this is a useless 'return' statement and > can be killed with no real effect. Oops, yes, this must have been a careless mistake when I combined the read_page and read_page_raw functions into one, with wrappers for each case. Oddly, no compiler warning. I'm trying to get around to putting together a patch with a couple minor fixes for the docg4, and I'll add this to the list. Thanks Brian, Mike
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
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
On Mon, Jun 25, 2012 at 3:16 AM, Josh Wu <josh.wu@atmel.com> wrote: > According to your advice, I resent a new patch series, which merge this > patch with PMECC. Thanks, it looks good. BTW, this affects Mike Dunn, who was planning on touching the docg4.c portion. Mike: note that the write_page() functions in docg4.c have been changed to fix the "error" we were discussing with return codes (see l2-mtd-2.6.git). Brian
diff --git a/drivers/mtd/nand/bcm_umi_bch.c b/drivers/mtd/nand/bcm_umi_bch.c index 5914bb3..c8799a0 100644 --- a/drivers/mtd/nand/bcm_umi_bch.c +++ b/drivers/mtd/nand/bcm_umi_bch.c @@ -23,7 +23,7 @@ /* ---- Private Function Prototypes -------------------------------------- */ static int bcm_umi_bch_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, uint8_t *buf, int oob_required, int page); -static void bcm_umi_bch_write_page_hwecc(struct mtd_info *mtd, +static int bcm_umi_bch_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf, int oob_required); /* ---- Private Variables ------------------------------------------------ */ @@ -194,7 +194,7 @@ static int bcm_umi_bch_read_page_hwecc(struct mtd_info *mtd, * @oob_required: must write chip->oob_poi to OOB * ***************************************************************************/ -static void bcm_umi_bch_write_page_hwecc(struct mtd_info *mtd, +static int bcm_umi_bch_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf, int oob_required) { int sectorIdx = 0; @@ -214,4 +214,6 @@ static void bcm_umi_bch_write_page_hwecc(struct mtd_info *mtd, } bcm_umi_nand_write_buf(mtd, chip->oob_poi, mtd->oobsize); + + return 0; } diff --git a/drivers/mtd/nand/bf5xx_nand.c b/drivers/mtd/nand/bf5xx_nand.c index 3f1c185..ab0caa7 100644 --- a/drivers/mtd/nand/bf5xx_nand.c +++ b/drivers/mtd/nand/bf5xx_nand.c @@ -566,11 +566,13 @@ static int bf5xx_nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip return 0; } -static void bf5xx_nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, - const uint8_t *buf, int oob_required) +static int bf5xx_nand_write_page_raw(struct mtd_info *mtd, + struct nand_chip *chip, const uint8_t *buf, int oob_required) { bf5xx_nand_write_buf(mtd, buf, mtd->writesize); bf5xx_nand_write_buf(mtd, chip->oob_poi, mtd->oobsize); + + return 0; } /* diff --git a/drivers/mtd/nand/cafe_nand.c b/drivers/mtd/nand/cafe_nand.c index 41371ba..3c92a98 100644 --- a/drivers/mtd/nand/cafe_nand.c +++ b/drivers/mtd/nand/cafe_nand.c @@ -520,7 +520,7 @@ static struct nand_bbt_descr cafe_bbt_mirror_descr_512 = { }; -static void cafe_nand_write_page_lowlevel(struct mtd_info *mtd, +static int cafe_nand_write_page_lowlevel(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf, int oob_required) { @@ -531,6 +531,8 @@ static void cafe_nand_write_page_lowlevel(struct mtd_info *mtd, /* Set up ECC autogeneration */ cafe->ctl2 |= (1<<30); + + return 0; } static int cafe_nand_write_page(struct mtd_info *mtd, struct nand_chip *chip, @@ -542,9 +544,14 @@ static int cafe_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 cafe_nand_write_page()\n"); + return status; + } /* * Cached progamming disabled for now, Not sure if its worth the diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c index 0650aaf..e706a23 100644 --- a/drivers/mtd/nand/denali.c +++ b/drivers/mtd/nand/denali.c @@ -1028,7 +1028,7 @@ static void denali_setup_dma(struct denali_nand_info *denali, int op) /* writes a page. user specifies type, and this function handles the * configuration details. */ -static void write_page(struct mtd_info *mtd, struct nand_chip *chip, +static int write_page(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf, bool raw_xfer) { struct denali_nand_info *denali = mtd_to_denali(mtd); @@ -1078,6 +1078,8 @@ static void write_page(struct mtd_info *mtd, struct nand_chip *chip, denali_enable_dma(denali, false); dma_sync_single_for_cpu(denali->dev, addr, size, DMA_TO_DEVICE); + + return 0; } /* NAND core entry points */ @@ -1086,24 +1088,24 @@ static void write_page(struct mtd_info *mtd, struct nand_chip *chip, * writing a page with ECC or without is similar, all the work is done * by write_page above. * */ -static void denali_write_page(struct mtd_info *mtd, struct nand_chip *chip, +static int denali_write_page(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf, int oob_required) { /* for regular page writes, we let HW handle all the ECC * data written to the device. */ - write_page(mtd, chip, buf, false); + return write_page(mtd, chip, buf, false); } /* This is the callback that the NAND core calls to write a page without ECC. * raw access is similar to ECC page writes, so all the work is done in the * write_page() function above. */ -static void denali_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, +static int denali_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf, int oob_required) { /* for raw page writes, we want to disable ECC and simply write whatever data is in the buffer. */ - write_page(mtd, chip, buf, true); + return write_page(mtd, chip, buf, true); } static int denali_write_oob(struct mtd_info *mtd, struct nand_chip *chip, diff --git a/drivers/mtd/nand/docg4.c b/drivers/mtd/nand/docg4.c index a225e49..0f2ffd7 100644 --- a/drivers/mtd/nand/docg4.c +++ b/drivers/mtd/nand/docg4.c @@ -898,7 +898,7 @@ static void docg4_erase_block(struct mtd_info *mtd, int page) write_nop(docptr); } -static void write_page(struct mtd_info *mtd, struct nand_chip *nand, +static int write_page(struct mtd_info *mtd, struct nand_chip *nand, const uint8_t *buf, bool use_ecc) { struct docg4_priv *doc = nand->priv; @@ -950,15 +950,17 @@ static void write_page(struct mtd_info *mtd, struct nand_chip *nand, write_nop(docptr); writew(0, docptr + DOC_DATAEND); write_nop(docptr); + + return 0; } -static void docg4_write_page_raw(struct mtd_info *mtd, struct nand_chip *nand, +static int docg4_write_page_raw(struct mtd_info *mtd, struct nand_chip *nand, const uint8_t *buf, int oob_required) { return write_page(mtd, nand, buf, false); } -static void docg4_write_page(struct mtd_info *mtd, struct nand_chip *nand, +static int docg4_write_page(struct mtd_info *mtd, struct nand_chip *nand, const uint8_t *buf, int oob_required) { return write_page(mtd, nand, buf, true); diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c index 7842938..35d731d 100644 --- a/drivers/mtd/nand/fsl_elbc_nand.c +++ b/drivers/mtd/nand/fsl_elbc_nand.c @@ -766,11 +766,13 @@ static int fsl_elbc_read_page(struct mtd_info *mtd, struct nand_chip *chip, /* ECC will be calculated automatically, and errors will be detected in * waitfunc. */ -static void fsl_elbc_write_page(struct mtd_info *mtd, struct nand_chip *chip, +static int fsl_elbc_write_page(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf, int oob_required) { fsl_elbc_write_buf(mtd, buf, mtd->writesize); fsl_elbc_write_buf(mtd, chip->oob_poi, mtd->oobsize); + + return 0; } static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv) diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c index 9602c1b..47ba534 100644 --- a/drivers/mtd/nand/fsl_ifc_nand.c +++ b/drivers/mtd/nand/fsl_ifc_nand.c @@ -721,11 +721,13 @@ static int fsl_ifc_read_page(struct mtd_info *mtd, struct nand_chip *chip, /* ECC will be calculated automatically, and errors will be detected in * waitfunc. */ -static void fsl_ifc_write_page(struct mtd_info *mtd, struct nand_chip *chip, +static int fsl_ifc_write_page(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf, int oob_required) { fsl_ifc_write_buf(mtd, buf, mtd->writesize); fsl_ifc_write_buf(mtd, chip->oob_poi, mtd->oobsize); + + return 0; } static int fsl_ifc_chip_init_tail(struct mtd_info *mtd) diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c index a05b7b4..3bda330 100644 --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c @@ -930,7 +930,7 @@ exit_nfc: return ret; } -static void gpmi_ecc_write_page(struct mtd_info *mtd, struct nand_chip *chip, +static int gpmi_ecc_write_page(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf, int oob_required) { struct gpmi_nand_data *this = chip->priv; @@ -972,7 +972,7 @@ static void gpmi_ecc_write_page(struct mtd_info *mtd, struct nand_chip *chip, &payload_virt, &payload_phys); if (ret) { pr_err("Inadequate payload DMA buffer\n"); - return; + return 0; } ret = send_page_prepare(this, @@ -1002,6 +1002,8 @@ exit_auxiliary: nfc_geo->payload_size, payload_virt, payload_phys); } + + return 0; } /* diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index d47586c..ad9e9a9 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -1927,12 +1927,14 @@ out: * * Not for syndrome calculating ECC controllers, which use a special oob layout. */ -static void nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, +static int nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf, int oob_required) { chip->write_buf(mtd, buf, mtd->writesize); if (oob_required) chip->write_buf(mtd, chip->oob_poi, mtd->oobsize); + + return 0; } /** @@ -1944,7 +1946,7 @@ static void nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, * * We need a special oob layout and handling even when ECC isn't checked. */ -static void nand_write_page_raw_syndrome(struct mtd_info *mtd, +static int nand_write_page_raw_syndrome(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf, int oob_required) { @@ -1974,6 +1976,8 @@ static void nand_write_page_raw_syndrome(struct mtd_info *mtd, size = mtd->oobsize - (oob - chip->oob_poi); if (size) chip->write_buf(mtd, oob, size); + + return 0; } /** * nand_write_page_swecc - [REPLACEABLE] software ECC based page write function @@ -1982,7 +1986,7 @@ static void nand_write_page_raw_syndrome(struct mtd_info *mtd, * @buf: data buffer * @oob_required: must write chip->oob_poi to OOB */ -static void nand_write_page_swecc(struct mtd_info *mtd, struct nand_chip *chip, +static int nand_write_page_swecc(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf, int oob_required) { int i, eccsize = chip->ecc.size; @@ -1999,7 +2003,7 @@ static void nand_write_page_swecc(struct mtd_info *mtd, struct nand_chip *chip, for (i = 0; i < chip->ecc.total; i++) chip->oob_poi[eccpos[i]] = ecc_calc[i]; - chip->ecc.write_page_raw(mtd, chip, buf, 1); + return chip->ecc.write_page_raw(mtd, chip, buf, 1); } /** @@ -2009,7 +2013,7 @@ static void nand_write_page_swecc(struct mtd_info *mtd, struct nand_chip *chip, * @buf: data buffer * @oob_required: must write chip->oob_poi to OOB */ -static void nand_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, +static int nand_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf, int oob_required) { int i, eccsize = chip->ecc.size; @@ -2029,6 +2033,8 @@ static void nand_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, chip->oob_poi[eccpos[i]] = ecc_calc[i]; chip->write_buf(mtd, chip->oob_poi, mtd->oobsize); + + return 0; } /** @@ -2041,7 +2047,7 @@ static void nand_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, * The hw generator calculates the error syndrome automatically. Therefore we * need a special oob layout and handling. */ -static void nand_write_page_syndrome(struct mtd_info *mtd, +static int nand_write_page_syndrome(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf, int oob_required) { @@ -2075,6 +2081,8 @@ static void nand_write_page_syndrome(struct mtd_info *mtd, i = mtd->oobsize - (oob - chip->oob_poi); if (i) chip->write_buf(mtd, oob, i); + + return 0; } /** @@ -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; + } /* * Cached progamming disabled for now. Not sure if it's worth the diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c index 252aaef..86640f7 100644 --- a/drivers/mtd/nand/pxa3xx_nand.c +++ b/drivers/mtd/nand/pxa3xx_nand.c @@ -681,11 +681,13 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command, info->state = STATE_IDLE; } -static void pxa3xx_nand_write_page_hwecc(struct mtd_info *mtd, +static int pxa3xx_nand_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf, int oob_required) { chip->write_buf(mtd, buf, mtd->writesize); chip->write_buf(mtd, chip->oob_poi, mtd->oobsize); + + return 0; } static int pxa3xx_nand_read_page_hwecc(struct mtd_info *mtd, diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c index aa9b8a5..0700ad9 100644 --- a/drivers/mtd/nand/sh_flctl.c +++ b/drivers/mtd/nand/sh_flctl.c @@ -365,7 +365,7 @@ static int flctl_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, return 0; } -static void flctl_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, +static int flctl_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf, int oob_required) { int i, eccsize = chip->ecc.size; @@ -375,6 +375,8 @@ static void flctl_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) chip->write_buf(mtd, p, eccsize); + + return 0; } static void execmd_read_page_sector(struct mtd_info *mtd, int page_addr) diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h index 57977c6..4cb6c7fe 100644 --- a/include/linux/mtd/nand.h +++ b/include/linux/mtd/nand.h @@ -361,13 +361,13 @@ struct nand_ecc_ctrl { uint8_t *calc_ecc); int (*read_page_raw)(struct mtd_info *mtd, struct nand_chip *chip, uint8_t *buf, int oob_required, int page); - void (*write_page_raw)(struct mtd_info *mtd, struct nand_chip *chip, + int (*write_page_raw)(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf, int oob_required); int (*read_page)(struct mtd_info *mtd, struct nand_chip *chip, uint8_t *buf, int oob_required, int page); int (*read_subpage)(struct mtd_info *mtd, struct nand_chip *chip, uint32_t offs, uint32_t len, uint8_t *buf); - void (*write_page)(struct mtd_info *mtd, struct nand_chip *chip, + int (*write_page)(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf, int oob_required); int (*write_oob_raw)(struct mtd_info *mtd, struct nand_chip *chip, int page);
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'. This patch change the definition of 'write_page' fuction to return an 'int' value. It adds code to test the return value, and if negative, indicate an error happend when write page with ECC. To be consistent, it also changes the 'write_page_raw' function's return type from 'void' to 'int'. This patch also adds 'return 0' for all implementation for the ecc 'write_page' and 'write_page_raw' functions. Note: I couldn't compile-test all of these easily, as some had ARCH dependencies. Signed-off-by: Josh Wu <josh.wu@atmel.com> --- drivers/mtd/nand/bcm_umi_bch.c | 6 ++++-- drivers/mtd/nand/bf5xx_nand.c | 6 ++++-- drivers/mtd/nand/cafe_nand.c | 13 ++++++++++--- drivers/mtd/nand/denali.c | 12 +++++++----- drivers/mtd/nand/docg4.c | 8 +++++--- drivers/mtd/nand/fsl_elbc_nand.c | 4 +++- drivers/mtd/nand/fsl_ifc_nand.c | 4 +++- drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 6 ++++-- drivers/mtd/nand/nand_base.c | 29 +++++++++++++++++++++-------- drivers/mtd/nand/pxa3xx_nand.c | 4 +++- drivers/mtd/nand/sh_flctl.c | 4 +++- include/linux/mtd/nand.h | 4 ++-- 12 files changed, 69 insertions(+), 31 deletions(-)