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

[Pratyush Yadav]

On 01/10/21 11:54AM, Tudor.Ambarus at 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 at 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.