[PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces

[Brian Norris]

Hi Shmulik,

On Tue, Apr 17, 2012 at 7:29 AM, Shmulik Ladkani
<shmulik.ladkani at gmail.com> wrote:
> Only verified execution in nand_base.c and nand.h, see below.

Thanks, this is helpful!

> On Mon, 16 Apr 2012 15:35:54 -0700 Brian Norris <computersforpeace at gmail.com> wrote:
>> @@ -1371,23 +1376,23 @@ static int nand_read_page_syndrome(struct mtd_info *mtd, struct nand_chip *chip,
>>               chip->read_buf(mtd, p, eccsize);
>>
>>               if (chip->ecc.prepad) {
>> -                     chip->read_buf(mtd, oob, chip->ecc.prepad);
>> -                     oob += chip->ecc.prepad;
>> +                     chip->read_buf(mtd, oobbuf, chip->ecc.prepad);
>> +                     oobbuf += chip->ecc.prepad;
>>               }
>>
>>               chip->ecc.hwctl(mtd, NAND_ECC_READSYN);
>> -             chip->read_buf(mtd, oob, eccbytes);
>> -             stat = chip->ecc.correct(mtd, p, oob, NULL);
>> +             chip->read_buf(mtd, oobbuf, eccbytes);
>> +             stat = chip->ecc.correct(mtd, p, oobbuf, NULL);
>>
>>               if (stat < 0)
>>                       mtd->ecc_stats.failed++;
>>               else
>>                       mtd->ecc_stats.corrected += stat;
>>
>> -             oob += eccbytes;
>> +             oobbuf += eccbytes;
>>
>>               if (chip->ecc.postpad) {
>> -                     chip->read_buf(mtd, oob, chip->ecc.postpad);
>> +                     chip->read_buf(mtd, oobbuf, chip->ecc.postpad);
>>                       oob += chip->ecc.postpad;
>
> Missed convertion of 'oob' in the last line above.
> s/oob/oobbuf/

Yes, good catch. Thanks.

> Same problem in the following lines of 'nand_read_page_syndrome'.
> Should be:
>
> @@ -1393,9 +1393,9 @@ static int nand_read_page_syndrome(struct mtd_info *mtd, struct nand_chip *chip,
>        }
>
>        /* Calculate remaining oob bytes */
> -       i = mtd->oobsize - (oob - chip->oob_poi);
> +       i = mtd->oobsize - (oobbuf - chip->oob_poi);
>        if (i)
> -               chip->read_buf(mtd, oob, i);
> +               chip->read_buf(mtd, oobbuf, i);
>
>        return 0;
>  }
>
>> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
>> index 1482340..be9ee1f 100644
>> --- a/include/linux/mtd/nand.h
>> +++ b/include/linux/mtd/nand.h
>> @@ -363,15 +363,15 @@ struct nand_ecc_ctrl {
>>       int (*correct)(struct mtd_info *mtd, uint8_t *dat, uint8_t *read_ecc,
>>                       uint8_t *calc_ecc);
>>       int (*read_page_raw)(struct mtd_info *mtd, struct nand_chip *chip,
>> -                     uint8_t *buf, int page);
>> +                     uint8_t *buf, uint8_t *oob, int page);
>>       void (*write_page_raw)(struct mtd_info *mtd, struct nand_chip *chip,
>> -                     const uint8_t *buf);
>> +                     const uint8_t *buf, const uint8_t *oob);
>>       int (*read_page)(struct mtd_info *mtd, struct nand_chip *chip,
>> -                     uint8_t *buf, int page);
>> +                     uint8_t *buf, uint8_t *oob, int page);
>>       int (*read_subpage)(struct mtd_info *mtd, struct nand_chip *chip,
>>                       uint32_t offs, uint32_t len, uint8_t *buf);
>
> No need to pass 'oob' to 'read_subpage'?

No. It seems like 'read_subpage' is already assumed not to read OOB
data; it only has one user, in nand_base.c, and this user has the
condition '!oob'. (My driver simply avoids subpage read/write
entirely, BTW, so I'm not too familiar with subpages.)

Thanks a lot for the review! This lets me know I should take a closer
look at my patch submissions...this pair of patches went through a lot
of revision before sending, so the 'oob' vs. 'oobbuf' thing got a
little convoluted.

Brian