[PATCH v2 17/35] mtd: spi-nor: Introduce spi_nor_nonsfdp_flags_init()

[Michael Walle]

Am 2021-10-22 14:42, schrieb Tudor.Ambarus at microchip.com:
> On 10/22/21 3:10 PM, Pratyush Yadav wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> On 22/10/21 01:21PM, Michael Walle wrote:
>>> Am 2021-08-17 12:24, schrieb Pratyush Yadav:
>>>> On 27/07/21 07:52AM, Tudor Ambarus wrote:
>>>>> Used to initialize the NOR flags for settings that are not defined
>>>>> in the JESD216 SFDP standard, thus can not be retrieved when 
>>>>> parsing
>>>>> SFDP. No functional change.
>>>> 
>>>> I am worried if the order in which these flags are set can cause 
>>>> some
>>>> subtle bugs.
>>>> 
>>>> I can see one instance of it with SNOR_F_HAS_LOCK.
>>>> spi_nor_late_init_params() checks for SNOR_F_HAS_LOCK and if there 
>>>> are
>>>> no locking ops specified, it sets the default locking ops. This 
>>>> works
>>>> fine before this patch because the flag is set before the function 
>>>> is
>>>> called. But now, the flag will be set _after_ the function is 
>>>> called,
>>>> and so you will never be able to set the default flags.
>>> 
>>> Maybe we should just forbid to look at the SNOR_F_ flags in these
>>> functions. Instead the information could also be deduced by looking 
>>> at
>>> the SPI_NOR_ flags.
> 
> not true.

I guess you mean that some init flash init functions might set it, too.
Right? IMHO this goes into the spaghetti code direction again.
spi_nor_late_init_params() must not look at the SNOR_F_ flags. There
are too much inter-function dependencies and it is really hard to
follow the call chain.

The locking ops should also go into the (static) flash init which only
depends on the SPI_NOR_ flags. If SNOR_F_HAS_LOCK will be added at
in an init function, then this code should also take care of setting
the correct locking ops. Maybe both can be set together with a helper.

-michael