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

Michael Walle
2021-10-22T13:06:14Z

Am 2021-10-22 14:42, schrieb Tudor.Ambarus@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