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