Re: [PATCH 10/15] mtd: rawnand: Move all page cache related fields to a sub-struct

From: Schrempf Frieder <frieder.schrempf@kontron.de>
To: Miquel Raynal <miquel.raynal@bootlin.com>,
	Boris Brezillon <bbrezillon@kernel.org>,
	Richard Weinberger <richard@nod.at>,
	David Woodhouse <dwmw2@infradead.org>,
	Brian Norris <computersforpeace@gmail.com>,
	"Marek Vasut" <marek.vasut@gmail.com>,
	Tudor Ambarus <Tudor.Ambarus@microchip.com>
Cc: Vignesh R <vigneshr@ti.com>, Julien Su <juliensu@mxic.com.tw>,
	Masahiro Yamada <yamada.masahiro@socionext.com>,
	"linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Mason Yang <masonccyang@mxic.com.tw>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 10/15] mtd: rawnand: Move all page cache related fields to a sub-struct
Date: Sat, 23 Feb 2019 13:13:43 +0000	[thread overview]
Message-ID: <60aa5a1f-b67c-a6e0-f1b7-07ea9e6bc5e7@kontron.de> (raw)
In-Reply-To: <20190221091527.20497-11-miquel.raynal@bootlin.com>

On 21.02.19 10:15, Miquel Raynal wrote:
> From: Boris Brezillon <bbrezillon@kernel.org>
> 
> Looking at the field names it's hard to tell what ->data_buf, ->pagebuf
> and ->pagebuf_bitflips are for. Clarify that by moving those fields
> in a sub-struct named pagecache.
> 
> Signed-off-by: Boris Brezillon <bbrezillon@kernel.org>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>

> ---
>   drivers/mtd/nand/raw/nand_base.c | 28 ++++++++++++++--------------
>   include/linux/mtd/rawnand.h      | 18 +++++++++++-------
>   2 files changed, 25 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 07985d2b91ac..256d16903d54 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -493,8 +493,8 @@ static int nand_do_write_oob(struct nand_chip *chip, loff_t to,
>   	}
>   
>   	/* Invalidate the page cache, if we write to the cached page */
> -	if (page == chip->pagebuf)
> -		chip->pagebuf = -1;
> +	if (page == chip->pagecache.page)
> +		chip->pagecache.page = -1;
>   
>   	nand_fill_oob(chip, ops->oobbuf, ops->ooblen, ops);
>   
> @@ -3204,7 +3204,7 @@ static int nand_do_read_ops(struct nand_chip *chip, loff_t from,
>   			use_bufpoi = 0;
>   
>   		/* Is the current page in the buffer? */
> -		if (realpage != chip->pagebuf || oob) {
> +		if (realpage != chip->pagecache.page || oob) {
>   			bufpoi = use_bufpoi ? chip->data_buf : buf;
>   
>   			if (use_bufpoi && aligned)
> @@ -3230,7 +3230,7 @@ static int nand_do_read_ops(struct nand_chip *chip, loff_t from,
>   			if (ret < 0) {
>   				if (use_bufpoi)
>   					/* Invalidate page cache */
> -					chip->pagebuf = -1;
> +					chip->pagecache.page = -1;
>   				break;
>   			}
>   
> @@ -3239,11 +3239,11 @@ static int nand_do_read_ops(struct nand_chip *chip, loff_t from,
>   				if (!NAND_HAS_SUBPAGE_READ(chip) && !oob &&
>   				    !(mtd->ecc_stats.failed - ecc_failures) &&
>   				    (ops->mode != MTD_OPS_RAW)) {
> -					chip->pagebuf = realpage;
> -					chip->pagebuf_bitflips = ret;
> +					chip->pagecache.page = realpage;
> +					chip->pagecache.bitflips = ret;
>   				} else {
>   					/* Invalidate page cache */
> -					chip->pagebuf = -1;
> +					chip->pagecache.page = -1;
>   				}
>   				memcpy(buf, chip->data_buf + col, bytes);
>   			}
> @@ -3283,7 +3283,7 @@ static int nand_do_read_ops(struct nand_chip *chip, loff_t from,
>   			memcpy(buf, chip->data_buf + col, bytes);
>   			buf += bytes;
>   			max_bitflips = max_t(unsigned int, max_bitflips,
> -					     chip->pagebuf_bitflips);
> +					     chip->pagecache.bitflips);
>   		}
>   
>   		readlen -= bytes;
> @@ -4002,9 +4002,9 @@ static int nand_do_write_ops(struct nand_chip *chip, loff_t to,
>   	page = realpage & chip->pagemask;
>   
>   	/* Invalidate the page cache, when we write to the cached page */
> -	if (to <= ((loff_t)chip->pagebuf << chip->page_shift) &&
> -	    ((loff_t)chip->pagebuf << chip->page_shift) < (to + ops->len))
> -		chip->pagebuf = -1;
> +	if (to <= ((loff_t)chip->pagecache.page << chip->page_shift) &&
> +	    ((loff_t)chip->pagecache.page << chip->page_shift) < (to + ops->len))
> +		chip->pagecache.page = -1;
>   
>   	/* Don't allow multipage oob writes with offset */
>   	if (oob && ops->ooboffs && (ops->ooboffs + ops->ooblen > oobmaxlen)) {
> @@ -4241,9 +4241,9 @@ int nand_erase_nand(struct nand_chip *chip, struct erase_info *instr,
>   		 * Invalidate the page cache, if we erase the block which
>   		 * contains the current cached page.
>   		 */
> -		if (page <= chip->pagebuf && chip->pagebuf <
> +		if (page <= chip->pagecache.page && chip->pagecache.page <
>   		    (page + pages_per_block))
> -			chip->pagebuf = -1;
> +			chip->pagecache.page = -1;
>   
>   		if (chip->legacy.erase)
>   			status = chip->legacy.erase(chip,
> @@ -5817,7 +5817,7 @@ static int nand_scan_tail(struct nand_chip *chip)
>   	chip->state = FL_READY;
>   
>   	/* Invalidate the pagebuffer reference */
> -	chip->pagebuf = -1;
> +	chip->pagecache.page = -1;
>   
>   	/* Large page NAND with SOFT_ECC should support subpage reads */
>   	switch (ecc->mode) {
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index e62c3ab4e0f7..00a8795b215d 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -1019,10 +1019,10 @@ struct nand_legacy {
>    * @chipsize:		[INTERN] the size of one chip for multichip arrays
>    * @pagemask:		[INTERN] page number mask = number of (pages / chip) - 1
>    * @data_buf:		[INTERN] buffer for data, size is (page size + oobsize).
> - * @pagebuf:		[INTERN] holds the pagenumber which is currently in
> - *			data_buf.
> - * @pagebuf_bitflips:	[INTERN] holds the bitflip count for the page which is
> - *			currently in data_buf.
> + * @pagecache:		Structure containing page cache related fields
> + * @pagecache.bitflips:	Number of bitflips of the cached page
> + * @pagecache.page:	Page number currently in the cache. -1 means no page is
> + *			currently cached
>    * @subpagesize:	[INTERN] holds the subpagesize
>    * @id:			[INTERN] holds NAND ID
>    * @parameters:		[INTERN] holds generic parameters under an easily
> @@ -1069,8 +1069,12 @@ struct nand_chip {
>   	uint64_t chipsize;
>   	int pagemask;
>   	u8 *data_buf;
> -	int pagebuf;
> -	unsigned int pagebuf_bitflips;
> +
> +	struct {
> +		unsigned int bitflips;
> +		int page;
> +	} pagecache;
> +
>   	int subpagesize;
>   	uint8_t bits_per_cell;
>   	uint16_t ecc_strength_ds;
> @@ -1374,7 +1378,7 @@ void nand_deselect_target(struct nand_chip *chip);
>    */
>   static inline void *nand_get_data_buf(struct nand_chip *chip)
>   {
> -	chip->pagebuf = -1;
> +	chip->pagecache.page = -1;
>   
>   	return chip->data_buf;
>   }
> 
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/