Re: [PATCH v2 2/6] nand: spi: add basic operations support

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

On Thu, 9 Mar 2017 09:43:36 +0800
Peter Pan <peterpansjtu@gmail.com> wrote:

> >> +
> >> +/**
> >> + * spinand_do_read_ops - read data from flash to buffer
> >> + * @mtd: MTD device structure
> >> + * @from: offset to read from
> >> + * @ops: oob ops structure
> >> + * Description:
> >> + *   Disable internal ECC before reading when MTD_OPS_RAW set.
> >> + */
> >> +static int spinand_do_read_ops(struct mtd_info *mtd, loff_t from,
> >> +                       struct mtd_oob_ops *ops)
> >> +{
> >> +     struct spinand_device *chip = mtd_to_spinand(mtd);
> >> +     struct nand_device *nand = mtd_to_nand(mtd);
> >> +     int ret;
> >> +     struct mtd_ecc_stats stats;
> >> +     unsigned int max_bitflips = 0;
> >> +     int oobreadlen = ops->ooblen;
> >> +     bool ecc_off = ops->mode == MTD_OPS_RAW;
> >> +     int ooblen = ops->mode == MTD_OPS_AUTO_OOB ?
> >> +             mtd->oobavail : mtd->oobsize;
> >> +
> >> +     if (unlikely(from >= mtd->size)) {
> >> +             pr_err("%s: attempt to read beyond end of device\n",
> >> +                             __func__);
> >> +             return -EINVAL;
> >> +     }  
> >
> > Again, we can an helper to check the boundaries in the generic NAND
> > code.  
> 
> Let's keep the code the same and move to generic NAND code later.
> What do you think Boris?

For things we know will be needed and are simple enough to provide, I'd
recommend doing the modifications now. That's a bit different when it
comes to providing a generic ECC engine infrastructure or trying to
move part of the page read/write logic into the generic NAND layer,
because we don't know where we're going yet.

> 
> >  
> >> +     if (oobreadlen > 0) {
> >> +             if (unlikely(ops->ooboffs >= ooblen)) {
> >> +                     pr_err("%s: attempt to start read outside oob\n",
> >> +                                     __func__);
> >> +                     return -EINVAL;
> >> +             }
> >> +             if (unlikely(ops->ooboffs + oobreadlen >
> >> +             (nand_len_to_pages(nand, mtd->size) - nand_offs_to_page(nand, from))
> >> +             * ooblen)) {
> >> +                     pr_err("%s: attempt to read beyond end of device\n",
> >> +                                     __func__);
> >> +                     return -EINVAL;
> >> +             }
> >> +             ooblen -= ops->ooboffs;
> >> +             ops->oobretlen = 0;
> >> +     }
> >> +     stats = mtd->ecc_stats;
> >> +     if (ecc_off)
> >> +             spinand_disable_ecc(chip);
> >> +     ret = spinand_read_pages(mtd, from, ops, &max_bitflips);
> >> +     if (ecc_off)
> >> +             spinand_enable_ecc(chip);
> >> +     if (ret)
> >> +             return ret;
> >> +
> >> +     if (mtd->ecc_stats.failed - stats.failed)
> >> +             return -EBADMSG;
> >> +
> >> +     return max_bitflips;
> >> +}
> >> +  
> >
> > [...]
> >  
> >> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> >> index f3d0351..ee447c1 100644
> >> --- a/include/linux/mtd/spinand.h
> >> +++ b/include/linux/mtd/spinand.h
> >> @@ -135,6 +135,8 @@ struct spinand_manufacturer_ops {
> >>       bool(*detect)(struct spinand_device *chip);
> >>       int (*init)(struct spinand_device *chip);
> >>       void (*cleanup)(struct spinand_device *chip);
> >> +     void (*build_column_addr)(struct spinand_device *chip,
> >> +                               struct spinand_op *op, u32 page, u32 column);  
> >
> > Okay, I think Arnaud was right, maybe we should have vendor specific
> > ops for basic operations like ->prepare_read/write_op(), instead of
> > having these ->get_dummy() and ->build_column_addr() hooks.
> > Or maybe just a ->prepare_op() hook that can prepare things for any
> > basic operation (read, write, ...).  
> 
> I prefer ->prepare_read_op() and ->prepare_write_op. Fix this in v3

I'd like to have Arnaud's feedback on this. Can you wait a bit before
sending a new version?