diff mbox series

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

Message ID 20240829032517.1517198-3-linchengming884@gmail.com
State New
Headers show
Series Add support for two-plane serial NAND flash | expand

Commit Message

Cheng Ming Lin Aug. 29, 2024, 3:25 a.m. UTC 
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.

Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw>
---
 drivers/mtd/nand/spi/macronix.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Comments

Miquel Raynal Aug. 30, 2024, 7:23 a.m. UTC | #1 
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
Cheng Ming Lin Aug. 30, 2024, 8:35 a.m. UTC | #2 
Hi Miquel,

Miquel Raynal <miquel.raynal@bootlin.com> 於 2024年8月30日 週五 下午3:23寫道：
>
> 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
>

Sure, I will update the commit titles as suggested.

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

No problem. I will adjust the formatting to have the two plane select bits on
separate lines and also rename it to SPINAND_HAS_PROG_PLANE_SELECT_BIT
to avoid any confusion.

> Thanks,
> Miquèl

Thanks,
Cheng Ming Lin
diff mbox series

Patch

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,
 		     SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout, NULL)),
 	SPINAND_INFO("MX35LF2GE4AD",
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x26, 0x03),
@@ -156,7 +157,7 @@  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_ECCINFO(&mx35lfxge4ab_ooblayout, NULL)),
 	SPINAND_INFO("MX35LF2G24AD-Z4I8",
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x64, 0x03),
@@ -174,7 +175,7 @@  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_ECCINFO(&mx35lfxge4ab_ooblayout, NULL)),
 	SPINAND_INFO("MX35LF4G24AD-Z4I8",
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x75, 0x03),
@@ -213,7 +214,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,
 		     SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout,
 				     mx35lf1ge4ab_ecc_get_status)),
 	SPINAND_INFO("MX35UF4G24AD",
@@ -223,7 +225,7 @@  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_ECCINFO(&mx35lfxge4ab_ooblayout,
 				     mx35lf1ge4ab_ecc_get_status)),
 	SPINAND_INFO("MX35UF4G24AD-Z4I8",
@@ -253,7 +255,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,
 		     SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout,
 				     mx35lf1ge4ab_ecc_get_status)),
 	SPINAND_INFO("MX35UF2G24AD",
@@ -263,7 +266,7 @@  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_ECCINFO(&mx35lfxge4ab_ooblayout,
 				     mx35lf1ge4ab_ecc_get_status)),
 	SPINAND_INFO("MX35UF2G24AD-Z4I8",