Re: [PATCH] mtd: spi-nor: core: Set mtd->eraseregions for non-uniform erase map

[Michael Walle <mwalle@kernel.org>]

Hi,

> -static void spi_nor_set_mtd_info(struct spi_nor *nor)
> +static int spi_nor_set_mtd_eraseregions(struct spi_nor *nor)
> +{
> +	struct spi_nor_erase_map *map = &nor->params->erase_map;
> +	struct spi_nor_erase_region *region = map->regions;
> +	struct mtd_info *mtd = &nor->mtd;
> +	struct mtd_erase_region_info *mtd_region;
> +	u32 erase_size;
> +	u8 erase_mask;
> +	int n_regions, i, j;
> +
> +	for (i = 0; !spi_nor_region_is_last(&region[i]); i++)
> +		;

Please put that into a helper which returns the number of regions.

FWIW, I really dislike the magic around encoding all sorts of stuff
into the offset. It makes the code just hard to read.


> +
> +	n_regions = i + 1;
> +	mtd_region = devm_kcalloc(nor->dev, n_regions, sizeof(*mtd_region),
> +				  GFP_KERNEL);

Who's the owner? mtd->dev or nor->dev?

> +	if (!mtd_region)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < n_regions; i++) {
> +		if (region[i].offset & SNOR_OVERLAID_REGION) {

Btw. what is an overlaid region? I couldn't find any comment
about it.

> +			erase_size = region[i].size;
> +		} else {
> +			erase_mask = region[i].offset & SNOR_ERASE_TYPE_MASK;
> +
> +			for (j = SNOR_ERASE_TYPE_MAX - 1; j >= 0; j--) {
> +				if (erase_mask & BIT(j)) {
> +					erase_size = map->erase_type[j].size;
> +					break;
> +				}
> +			}
> +		}
> +		mtd_region[i].erasesize = erase_size;
> +		mtd_region[i].numblocks = div64_ul(region[i].size, erase_size);
> +		mtd_region[i].offset = region[i].offset &
> +				       ~SNOR_ERASE_FLAGS_MASK;
> +	}
> +
> +	mtd->numeraseregions = n_regions;
> +	mtd->eraseregions = mtd_region;
> +
> +	return 0;
> +}
> +
> +static int spi_nor_set_mtd_info(struct spi_nor *nor)
>  {
>  	struct mtd_info *mtd = &nor->mtd;
>  	struct device *dev = nor->dev;
> @@ -3439,6 +3483,11 @@ static void spi_nor_set_mtd_info(struct spi_nor 
> *nor)
>  	mtd->_resume = spi_nor_resume;
>  	mtd->_get_device = spi_nor_get_device;
>  	mtd->_put_device = spi_nor_put_device;
> +
> +	if (spi_nor_has_uniform_erase(nor))
> +		return 0;
> +
> +	return spi_nor_set_mtd_eraseregions(nor);

mtd->erasesize is set somewhere else, please move it into this
function, because it will also have a special case for the
non_uniform flashes. Maybe we'll need our own erasesize stored
together with the opcode.

Also this should be written as

if (!spi_nor_has_uniform_erase(nor))
   spi_nor_set_mtd_eraseregions(nor);

return 0;

-michael

>  }
> 
>  static int spi_nor_hw_reset(struct spi_nor *nor)
> @@ -3531,7 +3580,9 @@ int spi_nor_scan(struct spi_nor *nor, const char 
> *name,
>  		return ret;
> 
>  	/* No mtd_info fields should be used up to this point. */
> -	spi_nor_set_mtd_info(nor);
> +	ret = spi_nor_set_mtd_info(nor);
> +	if (ret)
> +		return ret;
> 
>  	dev_info(dev, "%s (%lld Kbytes)\n", info->name,
>  			(long long)mtd->size >> 10);

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