Re: [PATCH v8 2/2] mtd: rawnand: nuvoton: add new driver for the Nuvoton MA35 SoC

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hello,

> +static int ma35_nand_attach_chip(struct nand_chip *chip)
> +{
> +	struct ma35_nand_info *nand = nand_get_controller_data(chip);
> +	struct ma35_nand_chip *nvtnand = to_ma35_nand(chip);
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	struct device *dev = mtd->dev.parent;
> +	u32 reg;
> +
> +	if (chip->options & NAND_BUSWIDTH_16) {
> +		dev_err(dev, "16 bits bus width not supported");
> +		return -EINVAL;
> +	}
> +
> +	nvtnand->nchunks = mtd->writesize / chip->ecc.steps;
> +	nvtnand->nchunks = (nvtnand->nchunks < 4) ? 1 : nvtnand->nchunks / 4;

This second division looks broken. Also, you probably don't want to do
that outside of the ON_HOST situation. Finally, you should probably
update chip->ecc.steps and chip->ecc.size to your final choice.

> +
> +	reg = readl(nand->regs + MA35_NFI_REG_NANDCTL) & (~PSIZE_MASK);
> +	if (mtd->writesize == 2048)
> +		writel(reg | PSIZE_2K, nand->regs + MA35_NFI_REG_NANDCTL);
> +	else if (mtd->writesize == 4096)
> +		writel(reg | PSIZE_4K, nand->regs + MA35_NFI_REG_NANDCTL);
> +	else if (mtd->writesize == 8192)
> +		writel(reg | PSIZE_8K, nand->regs + MA35_NFI_REG_NANDCTL);
> +
> +	switch (chip->ecc.engine_type) {
> +	case NAND_ECC_ENGINE_TYPE_ON_HOST:
> +		chip->options |= NAND_NO_SUBPAGE_WRITE | NAND_USES_DMA;

What is the reason for refusing subpage writes? This is not something
you can do later, so unless there is a good reason, please do not set
this flag.

> +		chip->ecc.write_page = ma35_nand_write_page_hwecc;
> +		chip->ecc.read_page  = ma35_nand_read_page_hwecc;
> +		chip->ecc.read_oob   = ma35_nand_read_oob_hwecc;
> +		return ma35_nand_hwecc_init(chip, nand);
> +	case NAND_ECC_ENGINE_TYPE_NONE:
> +	case NAND_ECC_ENGINE_TYPE_SOFT:
> +	case NAND_ECC_ENGINE_TYPE_ON_DIE:
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +

...

> +static int ma35_nand_chip_init(struct device *dev, struct ma35_nand_info *nand,
> +				 struct device_node *np)
> +{
> +	struct ma35_nand_chip *nvtnand;
> +	struct nand_chip *chip;
> +	struct mtd_info *mtd;
> +	int nsels;
> +	u32 tmp;
> +	int ret;
> +	int i;
> +
> +	if (!of_get_property(np, "reg", &nsels))

Please convert to device_property_ helpers. And remove the of include
once you no longer need it.

> +		return -ENODEV;
> +
> +	nsels /= sizeof(u32);
> +	if (!nsels || nsels > MA35_MAX_NSELS) {
> +		dev_err(dev, "invalid reg property size %d\n", nsels);
> +		return -EINVAL;
> +	}
> +
> +	nvtnand = devm_kzalloc(dev, struct_size(nvtnand, sels, nsels),
> +			      GFP_KERNEL);
> +	if (!nvtnand)
> +		return -ENOMEM;
> +
> +	nvtnand->nsels = nsels;
> +	for (i = 0; i < nsels; i++) {
> +		ret = of_property_read_u32_index(np, "reg", i, &tmp);
> +		if (ret) {
> +			dev_err(dev, "reg property failure : %d\n", ret);
> +			return ret;
> +		}
> +
> +		if (tmp >= MA35_MAX_NSELS) {
> +			dev_err(dev, "invalid CS: %u\n", tmp);
> +			return -EINVAL;
> +		}
> +
> +		if (test_and_set_bit(tmp, &nand->assigned_cs)) {
> +			dev_err(dev, "CS %u already assigned\n", tmp);
> +			return -EINVAL;
> +		}
> +
> +		nvtnand->sels[i] = tmp;
> +	}
> +

...

> +
> +	ret = mtd_device_register(mtd, NULL, 0);
> +	if (ret) {
> +		dev_err(dev, "MTD parse partition error\n");

probably useless error message?

> +		nand_cleanup(chip);
> +		return ret;
> +	}
> +
> +	list_add_tail(&nvtnand->node, &nand->chips);
> +
> +	return 0;
> +}

I believe next iteration should be the one, I'm rather happy with the
overall look.

Thanks,
Miquèl