Re: [PATCH 2/7] mtd: spi-nor: core: Report correct name in case of ID collisions

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

On 02/07/21 05:41PM, Tudor Ambarus wrote:
> Provide a way to report the correct flash name in case of ID collisions.
> There will be a single flash_info entry when flash IDs collide, and the
> differentiation between the flash types will be made at runtime
> if possible.

I am not sure if having one entry for all flashes with a collision is a 
good idea. For example, say we have one flash which supports SFDP and 
that's all we need for it. Another flash with the same ID does not 
support SFDP so it needs the SPI_NOR_DUAL_READ, etc. flags. How would 
you handle this case with the same entry? You would have to set all the 
flags in the disambiguation function. And nor->info is declared as const 
so you can't change the flags in there. Any code checking for 
info->flags would not work properly for these type of flashes. Wouldn't 
it be better to have multiple entries with the same ID and just pick the 
one we need in the disambiguation function?

Anyway, if you do decide to go with this approach, comments below.

> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/core.c  | 5 ++++-
>  include/linux/mtd/spi-nor.h | 2 ++
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 3d9f3698fb7b..d528e28995c6 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3208,7 +3208,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	/* Configure OTP parameters and ops */
>  	spi_nor_otp_init(nor);
>  
> -	dev_info(dev, "%s (%lld Kbytes)\n", info->name,
> +	if (!nor->name)
> +		nor->name = info->name;
> +
> +	dev_info(dev, "%s (%lld Kbytes)\n", nor->name,
>  			(long long)mtd->size >> 10);

You also need to update the usage of info->name in 
spi_nor_debugfs_init() and partname_show() in sysfs.c

>  
>  	dev_dbg(dev,
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index f67457748ed8..bd92acdd1899 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -369,6 +369,7 @@ struct spi_nor_flash_parameter;
>   * @bouncebuf:		bounce buffer used when the buffer passed by the MTD
>   *                      layer is not DMA-able
>   * @bouncebuf_size:	size of the bounce buffer
> + * @name:		used to point to correct name in case of ID collisions.
>   * @info:		SPI NOR part JEDEC MFR ID and other info
>   * @manufacturer:	SPI NOR manufacturer
>   * @page_size:		the page size of the SPI NOR
> @@ -399,6 +400,7 @@ struct spi_nor {
>  	struct spi_mem		*spimem;
>  	u8			*bouncebuf;
>  	size_t			bouncebuf_size;
> +	const char *name;
>  	const struct flash_info	*info;
>  	const struct spi_nor_manufacturer *manufacturer;
>  	u32			page_size;
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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