lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240830092303.00ca7cc6@xps-13>
Date: Fri, 30 Aug 2024 09:23:03 +0200
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: Cheng Ming Lin <linchengming884@...il.com>
Cc: vigneshr@...com, linux-mtd@...ts.infradead.org,
 linux-kernel@...r.kernel.org, richard@....at, alvinzhou@...c.com.tw,
 leoyu@...c.com.tw, Cheng Ming Lin <chengminglin@...c.com.tw>
Subject: Re: [PATCH v4 2/2] mtd: spinand: macronix: Use the flag in Macronix
 driver

Hi ChengMing,

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

> From: Cheng Ming Lin <chengminglin@...c.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@...c.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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ