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
