[PATCH v2 09/35] mtd: spi-nor: atmel: Use flash late_init() for locking

[Michael Walle]

Am 2021-08-16 21:06, schrieb Pratyush Yadav:
> On 27/07/21 07:51AM, Tudor Ambarus wrote:
>> Locking is not described in JESD216 SFDP standard, place the locking
>> init in late_init().

Btw, we should differentiate between the block protection
bits and individual block locking. At least the latter is described
in the SFDP (I've seen it in the XTX SFDP, haven't checked the
standard yet).

> You are chaning the order of setting the locking ops here. Earlier, 
> they
> were set before we parsed SFDP. Now they are set after we parse SFDP.
> Though I don't see it making much of a difference.
> 
>> 
>> Signed-off-by: Tudor Ambarus <tudor.ambarus at microchip.com>
>> ---
>>  drivers/mtd/spi-nor/atmel.c | 30 +++++++++++-------------------
>>  1 file changed, 11 insertions(+), 19 deletions(-)
>> 
>> diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c
>> index 1fea5cab492c..b937ef734e55 100644
>> --- a/drivers/mtd/spi-nor/atmel.c
>> +++ b/drivers/mtd/spi-nor/atmel.c
>> @@ -48,15 +48,11 @@ static const struct spi_nor_locking_ops 
>> atmel_at25fs_locking_ops = {
>>  	.is_locked = atmel_at25fs_is_locked,
>>  };
>> 
>> -static void atmel_at25fs_default_init(struct spi_nor *nor)
>> +static void atmel_at25fs_late_init(struct spi_nor *nor)
>>  {
>>  	nor->params->locking_ops = &atmel_at25fs_locking_ops;
>>  }
>> 
>> -static const struct spi_nor_fixups atmel_at25fs_fixups = {
>> -	.default_init = atmel_at25fs_default_init,
>> -};
>> -
>>  /**
>>   * atmel_set_global_protection - Do a Global Protect or Unprotect 
>> command
>>   * @nor:	pointer to 'struct spi_nor'
>> @@ -146,34 +142,30 @@ static const struct spi_nor_locking_ops 
>> atmel_global_protection_ops = {
>>  	.is_locked = atmel_is_global_protected,
>>  };
>> 
>> -static void atmel_global_protection_default_init(struct spi_nor *nor)
>> +static void atmel_global_protection_late_init(struct spi_nor *nor)
>>  {
>>  	nor->params->locking_ops = &atmel_global_protection_ops;
>>  }
>> 
>> -static const struct spi_nor_fixups atmel_global_protection_fixups = {
>> -	.default_init = atmel_global_protection_default_init,
>> -};
>> -
>>  static const struct flash_info atmel_parts[] = {
>>  	/* Atmel -- some are (confusingly) marketed as "DataFlash" */
>>  	{ "at25fs010",  INFO(0x1f6601, 0, 32 * 1024,   4, SECT_4K | 
>> SPI_NOR_HAS_LOCK)
>> -		.fixups = &atmel_at25fs_fixups },
>> +		.late_init = atmel_at25fs_late_init },
>>  	{ "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K | 
>> SPI_NOR_HAS_LOCK)
>> -		.fixups = &atmel_at25fs_fixups },
>> +		.late_init = atmel_at25fs_late_init },
>> 
>>  	{ "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8,
>>  			     SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE)
>> -			.fixups = &atmel_global_protection_fixups },
>> +		.late_init = atmel_global_protection_late_init },
> 
> Won't you be better off setting this in the manufacturer late_init()? 
> It
> seems common for most atmel flashes.
> 
> Of course, this would cause a problem for atmel flashes that don't have
> this at all, since we would set locking for those as well. But I think
> we can avoid that by checking for SNOR_F_HAS_LOCK in
> spi_nor_register_locking_ops().

+1

-michael