Re: [PATCH 04/11] nand: spi: Add read function support

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Tue, 21 Feb 2017 18:06:03 +0800
Peter Pan <peterpansjtu@gmail.com> wrote:



> >> +/**
> >> + * spi_nand_transfer_oob - transfer oob to client buffer
> >> + * @chip: SPI-NAND device structure
> >> + * @oob: oob destination address
> >> + * @ops: oob ops structure
> >> + * @len: size of oob to transfer
> >> + */
> >> +static void spi_nand_transfer_oob(struct spi_nand_chip *chip, u8 *oob,
> >> +                               struct mtd_oob_ops *ops, size_t len)
> >> +{
> >> +     struct mtd_info *mtd = spi_nand_to_mtd(chip);
> >> +     int ret;
> >> +
> >> +     switch (ops->mode) {
> >> +
> >> +     case MTD_OPS_PLACE_OOB:
> >> +     case MTD_OPS_RAW:
> >> +             memcpy(oob, chip->oobbuf + ops->ooboffs, len);
> >> +             return;
> >> +
> >> +     case MTD_OPS_AUTO_OOB:
> >> +             ret = mtd_ooblayout_get_databytes(mtd, oob, chip->oobbuf,
> >> +                                               ops->ooboffs, len);
> >> +             BUG_ON(ret);
> >> +             return;
> >> +
> >> +     default:
> >> +             BUG();  
> >
> > Let's try to avoid these BUG_ON() calls. If you want transfer_oob() to
> > report an error, change the function prototype and check the ret code
> > in the caller.  
> 
> I will remove the BUG_ON()s in v2. Boris, I think this part can be put in
> new NAND core, what do you think?

Well, assuming we move the ->oob buffer in nand_device struct, then
yes. That's exactly for this reason I wanted to see a version of the
SPI NAND framework before merging the "interface-agnostic NAND layer".
I guess we'll find other candidates for factorization, but I also think
we should not try to get the perfect implementation before merging
something (IOW, let's keep this improvement on our TODO list).

> 
> >  
> >> +     }
> >> +}