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

[Pratyush Yadav <p.yadav@ti.com>]

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()?

> + */
> +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.

>  }
>  
>  /**
> @@ -2643,26 +2663,6 @@ static void spi_nor_info_init_params(struct spi_nor *nor)
>  	spi_nor_init_uniform_erase_map(map, erase_mask, params->size);
>  }
>  
> -/**
> - * spi_nor_post_sfdp_fixups() - Updates the flash's parameters and settings
> - * after SFDP has been parsed (is also called for SPI NORs that do not
> - * support RDSFDP).
> - * @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/flash_info tables
> - * are incomplete or wrong).
> - */
> -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_late_init_params() - Late initialization of default flash parameters.
>   * @nor:	pointer to a 'struct spi_nor'
> @@ -2709,18 +2709,12 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
>   *    should be more accurate that the above.
>   *		spi_nor_sfdp_init_params()
>   *
> - *    Please note that there is a ->post_bfpt() fixup hook that can overwrite
> - *    the flash parameters and settings immediately after parsing the Basic
> - *    Flash Parameter Table.
> + *    Please note that there are ->post_{bfpt, sfdp}() fixup hooks that can
> + *    overwrite the flash parameters and settings immediately after table
> + *    parsing.
>   *
>   * which can be overwritten by:
> - * 4/ Post SFDP flash parameters initialization. Used to tweak various
> - *    parameters that could not be extracted by other means (i.e. when
> - *    information provided by the SFDP/flash_info tables are incomplete or
> - *    wrong).
> - *		spi_nor_post_sfdp_fixups()
> - *
> - * 5/ Late flash parameters initialization, used to initialize flash
> + * 4/ Late flash parameters initialization, used to initialize flash
>   * parameters that are not declared in the JESD216 SFDP standard.
>   *		spi_nor_late_init_params()
>   */
> @@ -2740,8 +2734,6 @@ static int spi_nor_init_params(struct spi_nor *nor)
>  	    !(nor->info->flags & SPI_NOR_SKIP_SFDP))
>  		spi_nor_sfdp_init_params(nor);
>  
> -	spi_nor_post_sfdp_fixups(nor);
> -
>  	spi_nor_late_init_params(nor);
>  
>  	return 0;
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/