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

[Yicong Yang <yangyicong@xxxxxxxxxxxxx>]

Hi Pratyush,


On 2020/4/24 21:30, Pratyush Yadav wrote:
> 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.

Seems type error. It should be
     !(enable || (nor->bouncebuf[0] & SR1_QUAD_EN_BIT6))
Thanks for pointing it out.

Thanks,
Yicong



>
>>  		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]);
>>  }


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