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

From: Pratyush Yadav <p.yadav@ti.com>
To: <Tudor.Ambarus@microchip.com>
Cc: macromorgan@hotmail.com, vigneshr@ti.com, jaimeliao@mxic.com.tw,
	richard@nod.at, esben@geanix.com, linux@rasmusvillemoes.dk,
	knaerzche@gmail.com, Nicolas.Ferre@microchip.com,
	michael@walle.cc, linux-mtd@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org, code@reto-schneider.ch,
	miquel.raynal@bootlin.com, heiko.thiery@gmail.com, sr@denx.de,
	mail@david-bauer.net, zhengxunli@mxic.com.tw
Subject: Re: [PATCH v2 11/35] mtd: spi-nor: winbond: Use manufacturer late_init() for OTP ops
Date: Mon, 11 Oct 2021 12:24:55 +0530	[thread overview]
Message-ID: <20211011065453.h7mnxpm4ywbaubbe@ti.com> (raw)
In-Reply-To: <aaab30e5-9ea0-c103-0791-e565470ebbc0@microchip.com>

On 01/10/21 11:54AM, Tudor.Ambarus@microchip.com wrote:
> 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:
> >> 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.
> 
> > 
> >>
> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> >> ---
> >>  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.

Ok, makes sense.

> 
> 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.

Which table has that information? BFPT DWORD 16? Anyway, I agree with 
keeping it after this patch set.

> 
> > 
> >> -     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.
> > 
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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