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

[<Tudor.Ambarus@microchip.com>]

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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/