Re: [PATCH v3 3/3] mtd: spi-nor: Favor the BFPT-parsed set_4byte_addr_mode method

[<Tudor.Ambarus@microchip.com>]

On 4/14/22 12:21, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-04-11 14:53, schrieb Tudor Ambarus:
>> JESD216 SFDP defines in the BFPT standard methods to enter and exit the
>> 4-Byte Address Mode. The flash parameters and settings that are
>> retrieved
>> from SFDP have higher precedence than the static initialized ones,
>> because
>> they should be more accurate and less error prone than those
>> initialized
>> statically. Favor the BFPT-parsed set_4byte_addr_mode method and use
>> the
>> generic core methods where possible.
>> This patch may introduce regressions in case BFPT contains wrong data.
>> The
>> fix is to introduce a post_bfpt() fixup hook and update the wrong BFPT
>> data.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
>> ---
>> v3: no changes
>>
>>  drivers/mtd/spi-nor/core.c      |  7 ++++++-
>>  drivers/mtd/spi-nor/macronix.c  | 10 ++++++++--
>>  drivers/mtd/spi-nor/micron-st.c |  9 ++++++---
>>  3 files changed, 20 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 4d45cda4f9d3..888516d98884 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -2416,6 +2416,8 @@ static void spi_nor_init_fixup_flags(struct
>> spi_nor *nor)
>>   */
>>  static void spi_nor_late_init_params(struct spi_nor *nor)
>>  {
>> +     struct spi_nor_flash_parameter *params = nor->params;
>> +
>>       if (nor->manufacturer && nor->manufacturer->fixups &&
>>           nor->manufacturer->fixups->late_init)
>>               nor->manufacturer->fixups->late_init(nor);
>> @@ -2423,6 +2425,10 @@ static void spi_nor_late_init_params(struct
>> spi_nor *nor)
>>       if (nor->info->fixups && nor->info->fixups->late_init)
>>               nor->info->fixups->late_init(nor);
>>
>> +     /* Default method kept for backward compatibility. */
>> +     if (!params->set_4byte_addr_mode)
>> +             params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode_brwr;
> 
> Can this be moved past..
> 
>> +
>>       spi_nor_init_flags(nor);
>>       spi_nor_init_fixup_flags(nor);
> 
> .. these two lines, so it is next to the "set default
> locking ops"?
> 

sure

>>
>> @@ -2490,7 +2496,6 @@ static void spi_nor_init_default_params(struct
>> spi_nor *nor)
>>       struct device_node *np = spi_nor_get_flash_node(nor);
>>
>>       params->quad_enable = spi_nor_sr2_bit1_quad_enable;
>> -     params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode_brwr;
>>       params->otp.org = &info->otp_org;
>>
>>       /* Default to 16-bit Write Status (01h) Command */
>> diff --git a/drivers/mtd/spi-nor/macronix.c
>> b/drivers/mtd/spi-nor/macronix.c
>> index 85e8655d362c..c267cbcc7f1d 100644
>> --- a/drivers/mtd/spi-nor/macronix.c
>> +++ b/drivers/mtd/spi-nor/macronix.c
>> @@ -105,12 +105,18 @@ static const struct flash_info
>> macronix_nor_parts[] = {
>>  static void macronix_nor_default_init(struct spi_nor *nor)
>>  {
>>       nor->params->quad_enable = spi_nor_sr1_bit6_quad_enable;
>> -     nor->params->set_4byte_addr_mode =
>> -             spi_nor_set_4byte_addr_mode_en4b_ex4b;
>> +}
>> +
>> +static void macronix_nor_late_init(struct spi_nor *nor)
>> +{
>> +     if (!nor->params->set_4byte_addr_mode)
>> +             nor->params->set_4byte_addr_mode =
>> +                     spi_nor_set_4byte_addr_mode_en4b_ex4b;
> 
> This is more of a general question. Can we have this in one
> line? IMHO this looks awful and since linux nowadays relaxed the
> 80 chars rule a bit and we have such long names.. I think it makes
> sense to allow some lines to be longer than 80 chars.

I still split my terminal in multiple windows and pretty much read
code in a 80 chars limit. But I won't argue with you, it seems you
care about it, so I'll change it.

> 
>>  }
>>
>>  static const struct spi_nor_fixups macronix_nor_fixups = {
>>       .default_init = macronix_nor_default_init,
>> +     .late_init = macronix_nor_late_init,
>>  };
>>
>>  const struct spi_nor_manufacturer spi_nor_macronix = {
>> diff --git a/drivers/mtd/spi-nor/micron-st.c
>> b/drivers/mtd/spi-nor/micron-st.c
>> index 2d1cbb1f37c8..9e9b107f2018 100644
>> --- a/drivers/mtd/spi-nor/micron-st.c
>> +++ b/drivers/mtd/spi-nor/micron-st.c
>> @@ -414,14 +414,17 @@ static void micron_st_nor_default_init(struct
>> spi_nor *nor)
>>       nor->flags |= SNOR_F_HAS_LOCK;
>>       nor->flags &= ~SNOR_F_HAS_16BIT_SR;
>>       nor->params->quad_enable = NULL;
>> -     nor->params->set_4byte_addr_mode =
>> -             spi_nor_set_4byte_addr_mode_wren_en4b_ex4b;
>>  }
>>
>>  static void micron_st_nor_late_init(struct spi_nor *nor)
>>  {
>> +     struct spi_nor_flash_parameter *params = nor->params;
>> +
>>       if (nor->info->mfr_flags & USE_FSR)
>> -             nor->params->ready = micron_st_nor_ready;
>> +             params->ready = micron_st_nor_ready;
>> +     if (!params->set_4byte_addr_mode)
>> +             params->set_4byte_addr_mode =
>> +                     spi_nor_set_4byte_addr_mode_wren_en4b_ex4b;
> 
> same here.
> 

sure

Thanks,
ta