Re: [PATCH v4 2/2] mtd: spinand: macronix: Use the flag in Macronix driver

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

Hi ChengMing,

linchengming884@gmail.com wrote on Thu, 29 Aug 2024 11:25:17 +0800:

> From: Cheng Ming Lin <chengminglin@mxic.com.tw>
> 
> Macronix serial NAND flash with a two-plane structure requires
> insertion of the Plane Select bit into the column address during
> the write_to_cache operation.
> 
> Additionally, for MX35{U,F}2G14AC and MX35LF2GE4AB, insertion of
> the Plane Select bit into the column address is required during
> the read_from_cache operation.
> 

PATH 1 is fine except the commit title, let me explain. Once applied in
the kernel tree, there is no cover letter anymore. So both titles would
be "Add support for the flag" and "Use the flag...", which is really
missing the important information as we don't know what this flag is
for. Furthermore, the fact that we decided to use a flag is an
implementation detail, what is important is the feature: setting the
plane select bit.

Can you please change the first commit title to:

mtd: spinand: Add support for setting plane select bits

and for the second, something like:

mtd: spinand: macronix: Flag parts needing explicit plane select

> Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw>
> ---
>  drivers/mtd/nand/spi/macronix.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/nand/spi/macronix.c b/drivers/mtd/nand/spi/macronix.c
> index 3f9e9c572854..f17cd4a6f4d0 100644
> --- a/drivers/mtd/nand/spi/macronix.c
> +++ b/drivers/mtd/nand/spi/macronix.c
> @@ -118,7 +118,8 @@ static const struct spinand_info macronix_spinand_table[] = {
>  		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
>  					      &write_cache_variants,
>  					      &update_cache_variants),
> -		     SPINAND_HAS_QE_BIT,
> +		     SPINAND_HAS_QE_BIT | SPINAND_HAS_PP_PLANE_SELECT_BIT |
> +		     SPINAND_HAS_READ_PLANE_SELECT_BIT,

And I know this is not what the normal coding style would ask for, but
I would prefer to have the two plane select bits on the same line if
possible, otherwise on two independent lines, so either:

		QE_BIT |
		PP_SELECT_BIT | READ SELECT_BIT,

or otherwise:

		QE_BIT |
		PP_SELECT_BIT |
		READ SELECT_BIT,

And finally, could we name the former

		SPINAND_HAS_PROG_PLANE_SELECT_BIT

? Because "PP" sounds a little bit too cryptic.

Thanks,
Miquèl

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