Re: [PATCH v2 16/35] mtd: spi-nor: core: Mark default_init() as deprecated

[<Tudor.Ambarus@microchip.com>]

On 8/16/21 10:36 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:
>> The goal is to remove the spagetti init of params. The flash should
>> be initialized by the SFDP data, and when SFDP tables are not defined,
>> by the flash_info flags. SFDP data can be corrected by the
>> post_{bfpt, sfdp} when wrong, and in case of flash_info flags init,
>> we'll use the late_init() hook, where checking for the
>> SPI_NOR_SKIP_SFDP flag.
> 
> Why depreciate it? It is not like we have external callers that we need
> to notify. We know and control all the users of this function. Just move
> all users to late_init() and delete this. You have already done a large
> part of that work in the previous patches. Why not convert all other
> callers as well? Is there some complicated piece of code that stops you
> from touching it for now?

Nothing complicated, but I don't have flashes to test. Let's take them one by one:

drivers/mtd/spi-nor/gigadevice.c:static void gd25q256_default_init(struct spi_nor *nor)
sets nor->params->quad_enable method. Quad enable is SFDP discoverable, if I replace
the default_init() with late_init() I'll risk to overwrite the method discovered by SFDP.

drivers/mtd/spi-nor/issi.c:     .default_init = issi_default_init,
same for ISSI. Here it is a manufacturer default init, which is worse. All flashes
have to be checked. This is another reason why I strongly prefer late_init() calls
to set flags per flash and not at manufacturer level.

drivers/mtd/spi-nor/micron-st.c:static void mt35xu512aba_default_init(struct spi_nor *nor)
you have this flash. If the octal_dtr_enable method is not SFDP discoverable, I can
move if to late_init(). I'm waiting for your call. I'll need a tested-by after I'll
make the patch.

drivers/mtd/spi-nor/micron-st.c:        .default_init = micron_st_default_init,
This one is hard to get rid of. I won't be surprised if these fields are overwritten
when parsing SFDP. Moving them in late_init() will nullify SFDP parsing.

drivers/mtd/spi-nor/spansion.c:static void s28hs512t_default_init(struct spi_nor *nor)
you have this flash. I'll move it to late init if octal_dtr_enable method is not SFDP
discoverable. I'll need a tested-by.

drivers/mtd/spi-nor/winbond.c:static void winbond_default_init(struct spi_nor *nor)
4byte addr mode is SFDP discoverable, if I move it in late_init() I'll overwrite what
I get from SFDP.

So just mark the hook as deprecated should be enough for now (at least for this release).
We'll probably never have all these flashes at hand, so maybe we'll agree for a time limit
after which we'll switch to late_init() or post_bfpt/sfpd() solely based on datasheet info.
If the datasheets will be wrong, we'll have bugs and people will scream, but nothing that
we can't handle/fix.

Cheers,
ta

> 
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>>  drivers/mtd/spi-nor/core.h | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> index 13d5c5edfd27..625f4eed75f1 100644
>> --- a/drivers/mtd/spi-nor/core.h
>> +++ b/drivers/mtd/spi-nor/core.h
>> @@ -289,9 +289,8 @@ struct spi_nor_flash_parameter {
>>
>>  /**
>>   * struct spi_nor_fixups - SPI NOR fixup hooks
>> - * @default_init: called after default flash parameters init. Used to tweak
>> - *                flash parameters when information provided by the flash_info
>> - *                table is incomplete or wrong.
>> + * @default_init: Deprecated. Use the post_{bfpt, sfdp}, or the late_init()
>> + *                hooks instead.
>>   * @post_bfpt: called after the BFPT table has been parsed
>>   * @post_sfdp: called after SFDP has been parsed (is also called for SPI NORs
>>   *             that do not support RDSFDP). Typically used to tweak various
>> --
>> 2.25.1
>>
> 
> --
> Regards,
> Pratyush Yadav
> Texas Instruments Inc.
> 

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