Re: [PATCH v2 15/35] mtd: spi-nor: core: Call spi_nor_post_sfdp_fixups() only when SFDP is defined

Tudor.Ambarus
2021-10-01T12:31:46Z

On 8/16/21 10:31 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:
>> spi_nor_post_sfdp_fixups() was called even when there were no SFDP
>> tables defined and the function name was misleading.
>>
>> We introduced the late_init() hook which is used to tweak various
>> parameters that could not be extracted by other means, i.e. when
>> parameters are not defined in the JESD216 SFDP standard, or when
>> the flash_info flags are incomplete.
>>
>> Use spi_nor_post_sfdp_fixups() just to fix SFDP data. post_sfdp()
>> hook is as of now used just by s28hs512t, mt35xu512aba, and both
>> support SFDP, there's no functional change with this patch.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>> drivers/mtd/spi-nor/core.c | 66 +++++++++++++++++---------------------
>> 1 file changed, 29 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 15ccc9994215..1f38fa8ab2fa 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -2509,6 +2509,25 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor)
>> nor->info->fixups->default_init(nor);
>> }
>>
>> +/**
>> + * spi_nor_post_sfdp_fixups() - Updates the flash's parameters and settings
>> + * after SFDP has been parsed.
>> + * @nor: pointer to a 'struct spi_nor'
>> + *
>> + * Typically used to tweak various parameters that could not be extracted by
>> + * other means (i.e. when information provided by the SFDP tables are
>> + * incomplete or wrong).
>
> Do we want to keep the "incomplete" here? Wouldn't incomplete
> information (like missing parameter tables) be more suited for
> late_init()?
>

will update, thanks

>> + */
>> +static void spi_nor_post_sfdp_fixups(struct spi_nor *nor)
>> +{
>> + if (nor->manufacturer && nor->manufacturer->fixups &&
>> + nor->manufacturer->fixups->post_sfdp)
>> + nor->manufacturer->fixups->post_sfdp(nor);
>> +
>> + if (nor->info->fixups && nor->info->fixups->post_sfdp)
>> + nor->info->fixups->post_sfdp(nor);
>> +}
>> +
>> /**
>> * spi_nor_sfdp_init_params() - Initialize the flash's parameters and settings
>> * based on JESD216 SFDP standard.
>> @@ -2523,11 +2542,12 @@ static void spi_nor_sfdp_init_params(struct spi_nor *nor)
>>
>> memcpy(&sfdp_params, nor->params, sizeof(sfdp_params));
>>
>> - if (spi_nor_parse_sfdp(nor)) {
>> - memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
>> - nor->addr_width = 0;
>> - nor->flags &= ~SNOR_F_4B_OPCODES;
>> - }
>> + if (!spi_nor_parse_sfdp(nor))
>> + return spi_nor_post_sfdp_fixups(nor);
>
> Huh. I didn't know you could do return foo() in a void function if foo()
> is also void. Dunno how I feel about this though. It definitely confused
> me for a bit.
>
>> +
>> + memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
>> + nor->addr_width = 0;
>> + nor->flags &= ~SNOR_F_4B_OPCODES;
>
> I feel like the new flow makes these 3 lines more confusing. Earlier,
> these were called under if (spi_nor_parse_sfdp()) so it was a bit easier
> to make the connection that these are undoing the changes performed by
> that function. Now it is a little harder to spot. I think a comment is
> in order.
>

I'll revisit this when reaching patch 27/35.

cheers,
ta