Re: [PATCH v2 15/35] mtd: spi-nor: core: Call spi_nor_post_sfdp_fixups() only when SFDP is defined

On 8/16/21 10:31 PM, Pratyush Yadav wrote:

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

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

quoted

spi_nor_post_sfdp_fixups() was called even when there were no SFDP
tables defined and the function name was misleading.

We introduced the late_init() hook which is used to tweak various
parameters that could not be extracted by other means, i.e. when
parameters are not defined in the JESD216 SFDP standard, or when
the flash_info flags are incomplete.

Use spi_nor_post_sfdp_fixups() just to fix SFDP data. post_sfdp()
hook is as of now used just by s28hs512t, mt35xu512aba, and both
support SFDP, there's no functional change with this patch.

Signed-off-by: Tudor Ambarus <redacted>
---
 drivers/mtd/spi-nor/core.c | 66 +++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 37 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 15ccc9994215..1f38fa8ab2fa 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c

@@ -2509,6 +2509,25 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor)
              nor->info->fixups->default_init(nor);
 }

+/**
+ * spi_nor_post_sfdp_fixups() - Updates the flash's parameters and settings
+ * after SFDP has been parsed.
+ * @nor:     pointer to a 'struct spi_nor'
+ *
+ * Typically used to tweak various parameters that could not be extracted by
+ * other means (i.e. when information provided by the SFDP tables are
+ * incomplete or wrong).

Do we want to keep the "incomplete" here? Wouldn't incomplete
information (like missing parameter tables) be more suited for
late_init()?

will update, thanks

quoted

+ */
+static void spi_nor_post_sfdp_fixups(struct spi_nor *nor)
+{
+     if (nor->manufacturer && nor->manufacturer->fixups &&
+         nor->manufacturer->fixups->post_sfdp)
+             nor->manufacturer->fixups->post_sfdp(nor);
+
+     if (nor->info->fixups && nor->info->fixups->post_sfdp)
+             nor->info->fixups->post_sfdp(nor);
+}
+
 /**
  * spi_nor_sfdp_init_params() - Initialize the flash's parameters and settings
  * based on JESD216 SFDP standard.

@@ -2523,11 +2542,12 @@ static void spi_nor_sfdp_init_params(struct spi_nor *nor)

      memcpy(&sfdp_params, nor->params, sizeof(sfdp_params));

-     if (spi_nor_parse_sfdp(nor)) {
-             memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
-             nor->addr_width = 0;
-             nor->flags &= ~SNOR_F_4B_OPCODES;
-     }
+     if (!spi_nor_parse_sfdp(nor))
+             return spi_nor_post_sfdp_fixups(nor);

Huh. I didn't know you could do return foo() in a void function if foo()
is also void. Dunno how I feel about this though. It definitely confused
me for a bit.

quoted

+
+     memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
+     nor->addr_width = 0;
+     nor->flags &= ~SNOR_F_4B_OPCODES;

I feel like the new flow makes these 3 lines more confusing. Earlier,
these were called under if (spi_nor_parse_sfdp()) so it was a bit easier
to make the connection that these are undoing the changes performed by
that function. Now it is a little harder to spot. I think a comment is
in order.

I'll revisit this when reaching patch 27/35.

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