Re: [RFC PATCH 03/27] mtd: nand: Introduce the ECC engine abstraction

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Boris,

Boris Brezillon <bbrezillon@kernel.org> wrote on Thu, 21 Feb 2019
12:16:25 +0100:

> On Thu, 21 Feb 2019 11:01:52 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> 
> > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> > index 30f0fb02abe2..9e3b018d0b83 100644
> > --- a/include/linux/mtd/nand.h
> > +++ b/include/linux/mtd/nand.h
> > @@ -82,8 +82,14 @@ struct nand_pos {
> >  	unsigned int page;
> >  };
> >  
> > +enum nand_page_io_req_type {
> > +	NAND_PAGE_READ = 0,
> > +	NAND_PAGE_WRITE,
> > +};
> > +
> >  /**
> >   * struct nand_page_io_req - NAND I/O request object
> > + * @type: the type of page I/O: read or write
> >   * @pos: the position this I/O request is targeting
> >   * @dataoffs: the offset within the page
> >   * @datalen: number of data bytes to read from/write to this page
> > @@ -99,6 +105,7 @@ struct nand_pos {
> >   * specific commands/operations.
> >   */
> >  struct nand_page_io_req {
> > +	enum nand_page_io_req_type type;  
> 
> Can you add the reqtype enum and type field (+ patch the iterator
> helpers) in a separate patch?

Sure.

> 
> >  	struct nand_pos pos;
> >  	unsigned int dataoffs;
> >  	unsigned int datalen;
> > @@ -116,13 +123,35 @@ struct nand_page_io_req {
> >  };
> >  
> >  /**
> > - * struct nand_ecc_req - NAND ECC requirements
> > + * struct nand_ecc_conf - NAND ECC configuration
> > + * @strength: ECC strength
> > + * @step_size: Number of bytes per step
> > + * @total: Total number of bytes used for storing ECC codes, this is used by
> > + *         generic OOB layouts
> > + */
> > +struct nand_ecc_conf {  
> 
> Please do the s/nand_ecc_req/nand_ecc_conf/ in a separate patch.

Ok.

> 
> > +	unsigned int strength;
> > +	unsigned int step_size;
> > +	unsigned int total;  
> 
> Do we really need to add this total field here? Looks like something
> that should be kept private to the ECC engine implementation.

It was initially private, but I realized it was needed by generic parts
(including for instance the generic OOB layouts) so it could not be
made private.

I just moved the 'total' entry out of the struct nand_ecc_conf and
moved it in the struct nand_ecc_ctx. It is still public but not in the
very generic "conf" structure anymore.

> 
> > +};
> > +
> > +/**
> > + * struct nand_ecc_user_conf - User desired ECC configuration
> > + * @mode: ECC mode
> > + * @algo: ECC algorithm
> >   * @strength: ECC strength
> >   * @step_size: ECC step/block size
> > + * @maximize: ECC parameters must be maximized depending on the device
> > + *            capabilities
> > + * @flags: User flags
> >   */
> > -struct nand_ecc_req {
> > +struct nand_ecc_user_conf {
> > +	int mode;  
> 
> We should definitely name that one differently ('provider' maybe).

Changed to provider.

> 
> > +	unsigned int algo;
> >  	unsigned int strength;
> >  	unsigned int step_size;
> > +	unsigned int maximize;
> > +	unsigned int flags;  
> 
> maximize could be a flag.

It is now.

> 
> >  };
> >  
> >  #define NAND_ECCREQ(str, stp) { .strength = (str), .step_size = (stp) }
> > @@ -157,11 +186,76 @@ struct nand_ops {
> >  	bool (*isbad)(struct nand_device *nand, const struct nand_pos *pos);
> >  };  
> 


Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel