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

From: Boris Brezillon <bbrezillon@kernel.org>
To: Miquel Raynal <miquel.raynal@bootlin.com>,
	<linux-arm-kernel@lists.infradead.org>,
	Mason Yang <masonccyang@mxic.com.tw>
Cc: Vignesh R <vigneshr@ti.com>,
	Tudor Ambarus <Tudor.Ambarus@microchip.com>,
	Julien Su <juliensu@mxic.com.tw>,
	Richard Weinberger <richard@nod.at>,
	Schrempf Frieder <frieder.schrempf@kontron.de>,
	Marek Vasut <marek.vasut@gmail.com>,
	linux-mtd@lists.infradead.org,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Brian Norris <computersforpeace@gmail.com>,
	David Woodhouse <dwmw2@infradead.org>
Subject: Re: [RFC PATCH 03/27] mtd: nand: Introduce the ECC engine abstraction
Date: Thu, 21 Feb 2019 12:16:25 +0100	[thread overview]
Message-ID: <20190221121625.33a7f6c0@kernel.org> (raw)
In-Reply-To: <20190221100216.25255-4-miquel.raynal@bootlin.com>

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?

>  	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.

> +	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.

> +};
> +
> +/**
> + * 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).

> +	unsigned int algo;
>  	unsigned int strength;
>  	unsigned int step_size;
> +	unsigned int maximize;
> +	unsigned int flags;

maximize could be a flag.

>  };
>  
>  #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);
>  };

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