diff mbox series

mtd: spi-nor: core: Discard HW capabilities if no enable function

Message ID 20231215073413.71443-1-jaimeliao.tw@gmail.com
State Changes Requested
Delegated to: Michael Walle
Headers show
Series mtd: spi-nor: core: Discard HW capabilities if no enable function | expand

Commit Message

liao jaime Dec. 15, 2023, 7:34 a.m. UTC 
From: JaimeLiao <jaimeliao@mxic.com.tw>

Discard corresponding HW capabilities to prevent carrying the
wrong protocol if no QUAD/Octal DTR enable function hooked.

Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
---
 drivers/mtd/spi-nor/core.c  | 13 +++++++------
 include/linux/mtd/spi-nor.h |  3 +++
 2 files changed, 10 insertions(+), 6 deletions(-)

Comments

Michael Walle Dec. 19, 2023, 9:02 a.m. UTC | #1 
Hi,

> Discard corresponding HW capabilities to prevent carrying the
> wrong protocol if no QUAD/Octal DTR enable function hooked.
> 
> Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
> ---
>  drivers/mtd/spi-nor/core.c  | 13 +++++++------
>  include/linux/mtd/spi-nor.h |  3 +++
>  2 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 1c443fe568cf..a3988e8768b3 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2621,6 +2621,13 @@ static int spi_nor_default_setup(struct spi_nor 
> *nor,
>  	 */
>  	shared_mask = hwcaps->mask & params->hwcaps.mask;
> 
> +	/* Mask out Octal DTR if no enable function */
> +	if (!params->set_octal_dtr)
> +		shared_mask &= ~SNOR_HWCAPS_X_X_X_DTR;

please also create a new SNOR_HWCAPS_8_8_8_DTR. While this is
the same for now, I suspect the SNOR_HWCAPS_X_X_X_DTR is might also
carry the 2_2_2_DTR and 4_4_4_DTR flags, just like SNOR_HWCAPS_X_X_X.
In any case, it's easier to read and more consistent with the code
below if you use SNOR_HWCAPS_8_8_8_DTR.

> +
> +	if (!params->quad_enable)
> +		shared_mask &= ~SNOR_HWCAPS_4_4_4;
> +
>  	if (nor->spimem) {
>  		/*
>  		 * When called from spi_nor_probe(), all caps are set and we
> @@ -3093,9 +3100,6 @@ static int spi_nor_set_octal_dtr(struct spi_nor 
> *nor, bool enable)
>  {
>  	int ret;
> 
> -	if (!nor->params->set_octal_dtr)
> -		return 0;
> -

I'd keep these checks, just in case.

>  	if (!(nor->read_proto == SNOR_PROTO_8_8_8_DTR &&
>  	      nor->write_proto == SNOR_PROTO_8_8_8_DTR))
>  		return 0;
> @@ -3123,9 +3127,6 @@ static int spi_nor_set_octal_dtr(struct spi_nor 
> *nor, bool enable)
>   */
>  static int spi_nor_quad_enable(struct spi_nor *nor)
>  {
> -	if (!nor->params->quad_enable)
> -		return 0;
> -

same here.

Otherwise looks good.

-michael

>  	if (!(spi_nor_get_protocol_width(nor->read_proto) == 4 ||
>  	      spi_nor_get_protocol_width(nor->write_proto) == 4))
>  		return 0;
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index cdcfe0fd2e7d..c421018dadf7 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -266,6 +266,9 @@ struct spi_nor_hwcaps {
>  #define SNOR_HWCAPS_PP_8_8_8		BIT(22)
>  #define SNOR_HWCAPS_PP_8_8_8_DTR	BIT(23)
> 
> +#define SNOR_HWCAPS_4_4_4	(SNOR_HWCAPS_READ_4_4_4 |	\
> +				 SNOR_HWCAPS_PP_4_4_4)
> +
>  #define SNOR_HWCAPS_X_X_X	(SNOR_HWCAPS_READ_2_2_2 |	\
>  				 SNOR_HWCAPS_READ_4_4_4 |	\
>  				 SNOR_HWCAPS_READ_8_8_8 |	\
liao jaime Dec. 19, 2023, 9:19 a.m. UTC | #2 
Hi Michael

>
> Hi,
>
> > Discard corresponding HW capabilities to prevent carrying the
> > wrong protocol if no QUAD/Octal DTR enable function hooked.
> >
> > Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
> > ---
> >  drivers/mtd/spi-nor/core.c  | 13 +++++++------
> >  include/linux/mtd/spi-nor.h |  3 +++
> >  2 files changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index 1c443fe568cf..a3988e8768b3 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -2621,6 +2621,13 @@ static int spi_nor_default_setup(struct spi_nor
> > *nor,
> >        */
> >       shared_mask = hwcaps->mask & params->hwcaps.mask;
> >
> > +     /* Mask out Octal DTR if no enable function */
> > +     if (!params->set_octal_dtr)
> > +             shared_mask &= ~SNOR_HWCAPS_X_X_X_DTR;
>
> please also create a new SNOR_HWCAPS_8_8_8_DTR. While this is
> the same for now, I suspect the SNOR_HWCAPS_X_X_X_DTR is might also
> carry the 2_2_2_DTR and 4_4_4_DTR flags, just like SNOR_HWCAPS_X_X_X.
> In any case, it's easier to read and more consistent with the code
> below if you use SNOR_HWCAPS_8_8_8_DTR.
Got it.
I will prepare the next version.

>
> > +
> > +     if (!params->quad_enable)
> > +             shared_mask &= ~SNOR_HWCAPS_4_4_4;
> > +
> >       if (nor->spimem) {
> >               /*
> >                * When called from spi_nor_probe(), all caps are set and we
> > @@ -3093,9 +3100,6 @@ static int spi_nor_set_octal_dtr(struct spi_nor
> > *nor, bool enable)
> >  {
> >       int ret;
> >
> > -     if (!nor->params->set_octal_dtr)
> > -             return 0;
> > -
>
> I'd keep these checks, just in case.
OK

>
> >       if (!(nor->read_proto == SNOR_PROTO_8_8_8_DTR &&
> >             nor->write_proto == SNOR_PROTO_8_8_8_DTR))
> >               return 0;
> > @@ -3123,9 +3127,6 @@ static int spi_nor_set_octal_dtr(struct spi_nor
> > *nor, bool enable)
> >   */
> >  static int spi_nor_quad_enable(struct spi_nor *nor)
> >  {
> > -     if (!nor->params->quad_enable)
> > -             return 0;
> > -
>
> same here.
OK

>
> Otherwise looks good.
>
> -michael
>
> >       if (!(spi_nor_get_protocol_width(nor->read_proto) == 4 ||
> >             spi_nor_get_protocol_width(nor->write_proto) == 4))
> >               return 0;
> > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> > index cdcfe0fd2e7d..c421018dadf7 100644
> > --- a/include/linux/mtd/spi-nor.h
> > +++ b/include/linux/mtd/spi-nor.h
> > @@ -266,6 +266,9 @@ struct spi_nor_hwcaps {
> >  #define SNOR_HWCAPS_PP_8_8_8         BIT(22)
> >  #define SNOR_HWCAPS_PP_8_8_8_DTR     BIT(23)
> >
> > +#define SNOR_HWCAPS_4_4_4    (SNOR_HWCAPS_READ_4_4_4 |       \
> > +                              SNOR_HWCAPS_PP_4_4_4)
> > +
> >  #define SNOR_HWCAPS_X_X_X    (SNOR_HWCAPS_READ_2_2_2 |       \
> >                                SNOR_HWCAPS_READ_4_4_4 |       \
> >                                SNOR_HWCAPS_READ_8_8_8 |       \

Thanks
Jaime
diff mbox series

Patch

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 1c443fe568cf..a3988e8768b3 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2621,6 +2621,13 @@  static int spi_nor_default_setup(struct spi_nor *nor,
 	 */
 	shared_mask = hwcaps->mask & params->hwcaps.mask;
 
+	/* Mask out Octal DTR if no enable function */
+	if (!params->set_octal_dtr)
+		shared_mask &= ~SNOR_HWCAPS_X_X_X_DTR;
+
+	if (!params->quad_enable)
+		shared_mask &= ~SNOR_HWCAPS_4_4_4;
+
 	if (nor->spimem) {
 		/*
 		 * When called from spi_nor_probe(), all caps are set and we
@@ -3093,9 +3100,6 @@  static int spi_nor_set_octal_dtr(struct spi_nor *nor, bool enable)
 {
 	int ret;
 
-	if (!nor->params->set_octal_dtr)
-		return 0;
-
 	if (!(nor->read_proto == SNOR_PROTO_8_8_8_DTR &&
 	      nor->write_proto == SNOR_PROTO_8_8_8_DTR))
 		return 0;
@@ -3123,9 +3127,6 @@  static int spi_nor_set_octal_dtr(struct spi_nor *nor, bool enable)
  */
 static int spi_nor_quad_enable(struct spi_nor *nor)
 {
-	if (!nor->params->quad_enable)
-		return 0;
-
 	if (!(spi_nor_get_protocol_width(nor->read_proto) == 4 ||
 	      spi_nor_get_protocol_width(nor->write_proto) == 4))
 		return 0;
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index cdcfe0fd2e7d..c421018dadf7 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -266,6 +266,9 @@  struct spi_nor_hwcaps {
 #define SNOR_HWCAPS_PP_8_8_8		BIT(22)
 #define SNOR_HWCAPS_PP_8_8_8_DTR	BIT(23)
 
+#define SNOR_HWCAPS_4_4_4	(SNOR_HWCAPS_READ_4_4_4 |	\
+				 SNOR_HWCAPS_PP_4_4_4)
+
 #define SNOR_HWCAPS_X_X_X	(SNOR_HWCAPS_READ_2_2_2 |	\
 				 SNOR_HWCAPS_READ_4_4_4 |	\
 				 SNOR_HWCAPS_READ_8_8_8 |	\