Re: [PATCH v2 11/35] mtd: spi-nor: winbond: Use manufacturer late_init() for OTP ops

On 8/16/21 10:17 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:51AM, Tudor Ambarus wrote:

quoted

OTP info is not yet discoverable via SFDP, use late_init() to init
the OTP ops.

What do you mean by the "yet"? Does it mean that OTP info is planned to
be added to the next SFDP version? Or does it mean that it is possible
to discover it via SFDP but we just don't support it yet?

If it is neither and it just means "SFDP does not mention OTP at all",
like it is for locking, then you should just drop the "yet". I know this
is very nitpicky but it just caught my eye.

I will update according to your suggestion.

quoted

Signed-off-by: Tudor Ambarus <redacted>
---
 drivers/mtd/spi-nor/winbond.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
index 96573f61caf5..6be45d2291c6 100644
--- a/drivers/mtd/spi-nor/winbond.c
+++ b/drivers/mtd/spi-nor/winbond.c

@@ -147,17 +147,22 @@ static const struct spi_nor_otp_ops winbond_otp_ops = {
 static void winbond_default_init(struct spi_nor *nor)
 {
      nor->params->set_4byte_addr_mode = winbond_set_4byte_addr_mode;

Why not move this to late_init() as well?

4byte mode is SFDP discoverable. Ideally we would get rid of the default_init()
hook. Flashes that define SFDP will get the 4byte mode from SFDP, the others
by explicitly setting the late_init() hook. All these should be done at flash
level, not manufacturer level, otherwise it will be hard to guess who sets what,
and we can end up with fixups for fixups.

I'll parse the 4byte mode from SFDP soon, I think I have some patches somewhere.
But the series is getting big, so maybe I'll keep it after this patch set.

quoted

-     if (nor->params->otp.org->n_regions)
-             nor->params->otp.ops = &winbond_otp_ops;
 }

 static const struct spi_nor_fixups winbond_fixups = {
      .default_init = winbond_default_init,
 };

+static void winbond_late_init(struct spi_nor *nor)
+{
+     if (nor->params->otp.org->n_regions)
+             nor->params->otp.ops = &winbond_otp_ops;
+}
+
 const struct spi_nor_manufacturer spi_nor_winbond = {
      .name = "winbond",
      .parts = winbond_parts,
      .nparts = ARRAY_SIZE(winbond_parts),
      .fixups = &winbond_fixups,
+     .late_init = winbond_late_init,
 };
--
2.25.1

--
Regards,
Pratyush Yadav
Texas Instruments Inc.

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