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

[Pratyush Yadav <p.yadav@xxxxxx>]

Hi Yicong,

You generally shouldn't mark a series as "RFC" if you intend it to get 
merged in.

On 12/05/20 07:26PM, 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 | 53 ++++++++++++++++++++++++++++++++--------------
>  drivers/mtd/spi-nor/core.h |  8 +++----
>  2 files changed, 41 insertions(+), 20 deletions(-)

Reviewed-by: Pratyush Yadav <p.yadav@xxxxxx>

Nits below.

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index cc68ea8..72e8d8b 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1907,15 +1907,17 @@ 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
> + * spi_nor_sr1_bit6_quad_enable() - Set/Unset the Quad Enable BIT(6) in the
> + *                                  Status
>   * Register 1.

The "Register 1" should be on the same line as the "Status above".

>   * @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,45 +1925,59 @@ 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)))

I still think writing it as:

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

is slightly more readable. But maybe it's just me so this is OK I guess.

>  		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]);
>  }
>  
>  /**
> - * spi_nor_sr2_bit1_quad_enable() - set the Quad Enable BIT(1) in the Status
> + * spi_nor_sr2_bit1_quad_enable() - set/unset the Quad Enable BIT(1) in the
> + *                                  Status
>   * Register 2.

The "Register 2" should be on the same line as the "Status above".

>   * @nor:       pointer to a 'struct spi_nor'.
> + * @enable:	true to enter quad mode. false to leave quad mode.
>   *
>   * Bit 1 of the Status Register 2 is the QE bit for Spansion like QSPI memories.
>   *
>   * Return: 0 on success, -errno otherwise.
>   */
> -int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor)
> +int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor, bool enable)
>  {
>  	int ret;
[...]
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 6f2f6b2..719a31d 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -219,7 +219,7 @@ struct spi_nor_flash_parameter {
>  
>  	struct spi_nor_erase_map        erase_map;
>  
> -	int (*quad_enable)(struct spi_nor *nor);
> +	int (*quad_enable)(struct spi_nor *nor, bool enable);

Update the comment above reflecting that @quad_enable "enables/disables 
SPI NOR quad mode".

>  	int (*set_4byte_addr_mode)(struct spi_nor *nor, bool enable);
>  	u32 (*convert_addr)(struct spi_nor *nor, u32 addr);
>  	int (*setup)(struct spi_nor *nor, const struct spi_nor_hwcaps *hwcaps);

-- 
Regards,
Pratyush Yadav
Texas Instruments India

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