Re: [PATCH] mtd: spi-nor: sst: Factor out common write operation to `sst_nor_write_data()`

From: Pratyush Yadav <pratyush@kernel.org>
To: "Csókás, Bence" <csokas.bence@prolan.hu>
Cc: <linux-mtd@lists.infradead.org>,  <linux-kernel@vger.kernel.org>,
	"Tudor Ambarus" <tudor.ambarus@linaro.org>,
	 Pratyush Yadav <pratyush@kernel.org>,
	 Michael Walle <mwalle@kernel.org>,
	 Miquel Raynal <miquel.raynal@bootlin.com>,
	 Richard Weinberger <richard@nod.at>,
	Vignesh Raghavendra <vigneshr@ti.com>
Subject: Re: [PATCH] mtd: spi-nor: sst: Factor out common write operation to `sst_nor_write_data()`
Date: Mon, 29 Jul 2024 16:55:43 +0200	[thread overview]
Message-ID: <mafs07cd42qog.fsf@kernel.org> (raw)
In-Reply-To: <20240710091401.1282824-1-csokas.bence@prolan.hu> ("Csókás, Bence"'s message of "Wed, 10 Jul 2024 11:14:01 +0200")

Hi,

On Wed, Jul 10 2024, Csókás, Bence wrote:

> Writing to the Flash in `sst_nor_write()` is a 3-step process:
> first an optional one-byte write to get 2-byte-aligned, then the
> bulk of the data is written out in vendor-specific 2-byte writes.
> Finally, if there's a byte left over, another one-byte write.
> This was implemented 3 times in the body of `sst_nor_write()`.
> To reduce code duplication, factor out these sub-steps to their
> own function.
>
> Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu>

Applied to spi-nor/next, thanks!

FYI, I spotted a couple small issues and fixed them up when applying.
See below...

> ---
>
> Notes:
>     RFC: I'm thinking of removing SPINOR_OP_BP in favor of
>     SPINOR_OP_PP (they have the same value). SPINOR_OP_PP
>     is the "standard" name for the elementary unit-sized
>     (1 byte, in the case of NOR) write operation. I find it
>     confusing to have two names for the same operation,
>     so in a followup I plan to remove the vendor-specific
>     name in favor of the standard one.
>
>  drivers/mtd/spi-nor/sst.c | 39 +++++++++++++++++++--------------------
>  1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
> index 180b7390690c..fec71689e644 100644
> --- a/drivers/mtd/spi-nor/sst.c
> +++ b/drivers/mtd/spi-nor/sst.c
> @@ -167,6 +167,21 @@ static const struct flash_info sst_nor_parts[] = {
>  	}
>  };
>  
> +static int sst_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
> +			const u_char *buf)

Whitespace issue, checkpatch complains on this. It should be aligned
with the opening parenthesis above.

> +{
> +	u8 op = (len == 1) ? SPINOR_OP_BP : SPINOR_OP_AAI_WP;
> +	int ret;
> +
> +	nor->program_opcode = op;
> +	ret = spi_nor_write_data(nor, to, 1, buf);
> +	if (ret < 0)
> +		return ret;
> +	WARN(ret != len, "While writing %i byte written %i bytes\n", len, ret);

I get a build warning because of incorrect format specifier. Should use
%zu since len is size_t.

> +
> +	return spi_nor_wait_till_ready(nor);
> +}
> +
>  static int sst_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>  			 size_t *retlen, const u_char *buf)
>  {
[...]

-- 
Regards,
Pratyush Yadav

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/