Re: [PATCH] mtd: spi-nor: core: Discard HW capabilities if no enable function

From: Michael Walle <michael@walle.cc>
To: Jaime Liao <jaimeliao.tw@gmail.com>
Cc: linux-mtd@lists.infradead.org, tudor.ambarus@linaro.org,
	pratyush@kernel.org, miquel.raynal@bootlin.com,
	leoyu@mxic.com.tw, jaimeliao@mxic.com.tw
Subject: Re: [PATCH] mtd: spi-nor: core: Discard HW capabilities if no enable function
Date: Tue, 19 Dec 2023 10:02:57 +0100	[thread overview]
Message-ID: <6eeb2dbb4f0ee4e0c8bfe381aa28ef3f@walle.cc> (raw)
In-Reply-To: <20231215073413.71443-1-jaimeliao.tw@gmail.com>

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

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