Re: [PATCH v2 1/2] mtd: spi-nor: core: add flag for doing optional SFDP

[Tudor Ambarus]

On 6/6/24 14:59, Michael Walle wrote:
> On Thu Jun 6, 2024 at 3:31 PM CEST, Tudor Ambarus wrote:
>> On 6/3/24 14:09, Esben Haabendal wrote:
>>> A dedicated flag for triggering call to
>>> spi_nor_sfdp_init_params_deprecated() allows enabling optional SFDP read
>>> and parse, with fallback to legacy flash parameters, without having dual,
>>> quad or octal parameters set in the legacy flash parameters.
>>>
>>> With this, spi-nor flash parts without SFDP that is replaced with a
>>> different flash NOR flash part that does have SFDP, but shares the same
>>> manufacturer and device ID is easily handled.
>>>
>>> Signed-off-by: Esben Haabendal <esben@xxxxxxxxxx>
>>> ---
>>>  drivers/mtd/spi-nor/core.c | 3 ++-
>>>  drivers/mtd/spi-nor/core.h | 1 +
>>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>> index 3e1f1913536b..1c4d66fc993b 100644
>>> --- a/drivers/mtd/spi-nor/core.c
>>> +++ b/drivers/mtd/spi-nor/core.c
>>> @@ -2933,7 +2933,8 @@ static void spi_nor_init_params_deprecated(struct spi_nor *nor)
>>>  
>>>  	spi_nor_manufacturer_init_params(nor);
>>>  
>>> -	if (nor->info->no_sfdp_flags & (SPI_NOR_DUAL_READ |
>>> +	if (nor->info->no_sfdp_flags & (SPI_NOR_TRY_SFDP |
>>
>> I don't like that we update deprecated methods. The solution though is
>> elegant.
> 
> I actually had the same concern. But currently there is no
> non-deprecated way to handle this case, right?
> 
> Right now we have the following cases:
>  (1) pure SFDP parsing
>  (2) non-SFDP flashes with static configuration only
>  (3) legacy implementation, where the magic flags decide whether we
>      use SFDP
> 
> Which case is eventually used depends on the ID of the flash -
> assuming there will only be IDs which either fall into (1) *or* (2).
> That assumption is clearly wrong :)
> 
> I'd propose a new case in spi_nor_init_params()
>  (4) try SFDP with a fallback to the static flags from the
>      flash_info db.
> 

that's not that bad, but I would avoid doing it if it's not common. You
also have to update the core a bit, you can't use no_sfdp_flags &
TRY_SFDP, it's misleading. Does it worth it?

I won't oppose too much, but to me it feels that we're trying to keep
alive a dead man.