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

Am 2021-08-16 21:06, schrieb Pratyush Yadav:

On 27/07/21 07:51AM, Tudor Ambarus wrote:

quoted

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.

quoted

Signed-off-by: Tudor Ambarus <redacted>
---
 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel