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

[Tudor.Ambarus at microchip.com]

On 10/22/21 3:59 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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.

I don't think I understand what you are referring to. The flash_info flags
are used just to set their SNOR_F correspondents. Take SNOR_F_4B_OPCODES for
example. A flash that can't discover 4b opcodes support when parsing SFDP,
will set the SPI_NOR_4B_OPCODES flash_info flag. Flashes that can discover it,
will ignore/not set the SPI_NOR_4B_OPCODES flag, and let the SFDP do its thing:
the SFDP code will set the SNOR_F_4B_OPCODES flag. The only flags that are used
across the SPI NOR core are the nor->flags (SNOR_F).
> 
> 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

we don't have to specify the locking methods, we have generic ones. A single
flag is enough.

> in an init function, then this code should also take care of setting

what init function? We're just setting some flags.

> the correct locking ops. Maybe both can be set together with a helper.