Re: [RFC PATCH 1/2] mtd: spi-nor: Add capability to disable flash quad mode

[Pratyush Yadav <me@xxxxxxxxxxxxxxxxx>]

Hi Yicong,

On 24/04/20 05:20PM, Yicong Yang wrote:
> Previous we didn't provide a way to disable the flash's quad mode.
> Which means we cannot do some cleanup works when to remove or
> poweroff the flash, like what set 4-byte address mode does in
> spi_nor_restore().
> 
> Add the capability to disable the flash quad mode, by introducing
> an enable flag in the flash parameters quad_enable() hooks and
> related functions.
> 
> Signed-off-by: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
> ---
>  drivers/mtd/spi-nor/core.c | 38 ++++++++++++++++++++++++++++----------
>  drivers/mtd/spi-nor/core.h |  8 ++++----
>  2 files changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index cc68ea8..d0516e8 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1910,12 +1910,13 @@ static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>   * spi_nor_sr1_bit6_quad_enable() - Set the Quad Enable BIT(6) in the Status

Nitpick: This should probably be changed to "Set/unset the Quad 
Enable...". Same in other places.

>   * Register 1.
>   * @nor:	pointer to a 'struct spi_nor'
> + * @enable:	true to enter quad mode. false to leave quad mode.
>   *
>   * Bit 6 of the Status Register 1 is the QE bit for Macronix like QSPI memories.
>   *
>   * Return: 0 on success, -errno otherwise.
>   */
> -int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor)
> +int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor, bool enable)
>  {
>  	int ret;
> 
> @@ -1923,10 +1924,14 @@ int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor)
>  	if (ret)
>  		return ret;
> 
> -	if (nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)
> +	if ((enable && (nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)) ||
> +	    ~(enable || (nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)))

This condition will always evaluate to true.

Since (enable || (nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)) is always a 
boolean, it will evaluate to 0 or 1. And ~0 is 0xFFFFFFFF and ~1 is 
0xFFFFFFFE, both of which evaluate to true.

IIUC, this condition is supposed to mean "If we want to enable and it is 
already enabled, do nothing. Or, if we want to disable and it is already 
disabled, do nothing." This might be what you were going for:

	if ((enable && (nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)) ||
	    (!enable && !(nor->bouncebuf[0] & SR1_QUAD_EN_BIT6)))

Same for other places this pattern appears.

>  		return 0;
> 
> -	nor->bouncebuf[0] |= SR1_QUAD_EN_BIT6;
> +	if (enable)
> +		nor->bouncebuf[0] |= SR1_QUAD_EN_BIT6;
> +	else
> +		nor->bouncebuf[0] &= ~SR1_QUAD_EN_BIT6;
> 
>  	return spi_nor_write_sr1_and_check(nor, nor->bouncebuf[0]);
>  }

-- 
Regards,
Pratyush Yadav

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