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

From: <Tudor.Ambarus@microchip.com>
To: <p.yadav@ti.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: Fri, 1 Oct 2021 11:54:50 +0000	[thread overview]
Message-ID: <aaab30e5-9ea0-c103-0791-e565470ebbc0@microchip.com> (raw)
In-Reply-To: <20210816191744.k6ufbgrrwh6whhnc@ti.com>

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.

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.

> 
>> -     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 MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/