Hi Yifeng, [...] > +static void rk_nfc_disable_clk(struct rk_nfc_clk *clk) > +{ > + if (!IS_ERR(clk->nfc_clk)) > + clk_disable_unprepare(clk->nfc_clk); > + clk_disable_unprepare(clk->ahb_clk); > +} > + > +static int rk_nfc_ooblayout_free(struct mtd_info *mtd, int section, > + struct mtd_oob_region *oob_region) > +{ > + struct nand_chip *chip = mtd_to_nand(mtd); > + > + if (section >= chip->ecc.steps) > + return -ERANGE; > + > + if (!section) { > + /* The first byte is bad block mask flag. */ > + oob_region->length = NFC_SYS_DATA_SIZE - 1; > + oob_region->offset = 1; > + } else { > + oob_region->length = NFC_SYS_DATA_SIZE; > + oob_region->offset = section * NFC_SYS_DATA_SIZE; > + } > + > + return 0; > +} > + > +static int rk_nfc_ooblayout_ecc(struct mtd_info *mtd, int section, > + struct mtd_oob_region *oob_region) > +{ > + struct nand_chip *chip = mtd_to_nand(mtd); > + > + if (section) > + return -ERANGE; > + > + oob_region->offset = NFC_SYS_DATA_SIZE * chip->ecc.steps; > + oob_region->length = mtd->oobsize - oob_region->offset; > + > + return 0; > +} > + > +static const struct mtd_ooblayout_ops rk_nfc_ooblayout_ops = { > + .free = rk_nfc_ooblayout_free, > + .ecc = rk_nfc_ooblayout_ecc, > +}; This looks like a copy of the core's nand_lp_ooblayout, better use the generic one than creation your own. > + > +static int rk_nfc_ecc_init(struct device *dev, struct mtd_info *mtd) > +{ > + struct nand_chip *chip = mtd_to_nand(mtd); > + struct rk_nfc *nfc = nand_get_controller_data(chip); > + struct nand_ecc_ctrl *ecc = &chip->ecc; > + const u8 *strengths = nfc->cfg->ecc_strengths; > + u8 max_strength, nfc_max_strength; > + int i; > + > + nfc_max_strength = nfc->cfg->ecc_strengths[0]; > + /* If optional dt settings not present. */ > + if (!ecc->size || !ecc->strength || > + ecc->strength > nfc_max_strength) { > + /* Use datasheet requirements. */ > + ecc->strength = chip->base.eccreq.strength; > + ecc->size = chip->base.eccreq.step_size; > + > + /* > + * Align eccstrength and eccsize. > + * This controller only supports 512 and 1024 sizes. > + */ > + if (chip->ecc.size < 1024) { > + if (mtd->writesize > 512) { > + chip->ecc.size = 1024; > + chip->ecc.strength <<= 1; > + } else { > + dev_err(dev, "ecc.size not supported\n"); > + return -EINVAL; > + } > + } else { > + chip->ecc.size = 1024; > + } > + > + ecc->steps = mtd->writesize / ecc->size; > + > + /* > + * HW ECC always request ECC bytes for 1024 bytes blocks. > + * 4 Bytes is oob for sys data. > + */ > + max_strength = ((mtd->oobsize / ecc->steps) - 4) * 8 / > + fls(8 * 1024); > + if (max_strength > nfc_max_strength) > + max_strength = nfc_max_strength; > + > + for (i = 0; i < 4; i++) { > + if (max_strength >= strengths[i]) > + break; > + } > + > + if (i >= 4) { > + dev_err(nfc->dev, "unsupported strength\n"); > + return -ENOTSUPP; > + } > + > + ecc->strength = strengths[i]; > + } > + ecc->steps = mtd->writesize / ecc->size; > + ecc->bytes = DIV_ROUND_UP(ecc->strength * fls(8 * 1024), 8); > + /* HW ECC always work with even numbers of ECC bytes. */ > + ecc->bytes = ALIGN(ecc->bytes, 2); > + > + rk_nfc_hw_ecc_setup(chip, ecc, ecc->strength); > + > + return 0; > +} > + > +static int rk_nfc_attach_chip(struct nand_chip *chip) > +{ > + struct mtd_info *mtd = nand_to_mtd(chip); > + struct device *dev = mtd->dev.parent; > + struct rk_nfc *nfc = nand_get_controller_data(chip); > + struct rk_nfc_nand_chip *rk_nand = to_rk_nand(chip); > + struct nand_ecc_ctrl *ecc = &chip->ecc; > + int len; > + int ret; > + > + if (chip->options & NAND_BUSWIDTH_16) { > + dev_err(dev, "16 bits bus width not supported"); > + return -EINVAL; > + } > + > + if (ecc->mode != NAND_ECC_HW) > + return 0; > + > + ret = rk_nfc_ecc_init(dev, mtd); > + if (ret) > + return ret; > + rk_nand->spare_per_sector = ecc->bytes + NFC_SYS_DATA_SIZE; > + > + /* Check buffer first, avoid duplicate alloc buffer. */ > + if (nfc->buffer) > + return 0; > + > + len = mtd->writesize + mtd->oobsize; > + nfc->buffer = devm_kzalloc(dev, len, GFP_KERNEL | GFP_DMA); > + if (!nfc->buffer) > + return -ENOMEM; > + > + nfc->page_buf = nfc->buffer; > + len = ecc->steps * NFC_MAX_OOB_PER_STEP; > + nfc->oob_buf = devm_kzalloc(dev, len, GFP_KERNEL | GFP_DMA); > + if (!nfc->oob_buf) { > + devm_kfree(dev, nfc->buffer); This is not needed I suppose and you should probably just return the error. > + nfc->buffer = NULL; > + nfc->oob_buf = NULL; > + return -ENOMEM; > + } > + > + chip->ecc.write_page_raw = rk_nfc_write_page_raw; > + chip->ecc.write_page = rk_nfc_write_page_hwecc; > + chip->ecc.write_oob_raw = rk_nfc_write_oob_std; > + chip->ecc.write_oob = rk_nfc_write_oob_std; > + > + chip->ecc.read_page_raw = rk_nfc_read_page_raw; > + chip->ecc.read_page = rk_nfc_read_page_hwecc; > + chip->ecc.read_oob_raw = rk_nfc_read_oob_std; > + chip->ecc.read_oob = rk_nfc_read_oob_std; > + > + return 0; > +} > + > +static const struct nand_controller_ops rk_nfc_controller_ops = { > + .attach_chip = rk_nfc_attach_chip, > + .exec_op = rk_nfc_exec_op, > + .setup_data_interface = rk_nfc_setup_data_interface, > +}; > + > +static int rk_nfc_nand_chip_init(struct device *dev, struct rk_nfc *nfc, > + struct device_node *np) > +{ > + struct rk_nfc_nand_chip *nand; Could you call it rknand or something similar, just so that it is easy to get the difference with the nand_chip structure. > + struct nand_chip *chip; > + struct mtd_info *mtd; > + int nsels; > + u32 tmp; > + int ret; > + int i; > + > + if (!of_get_property(np, "reg", &nsels)) > + return -ENODEV; > + nsels /= sizeof(u32); > + if (!nsels || nsels > NFC_MAX_NSELS) { > + dev_err(dev, "invalid reg property size %d\n", nsels); > + return -EINVAL; > + } > + > + nand = devm_kzalloc(dev, sizeof(*nand) + nsels * sizeof(u8), > + GFP_KERNEL); > + if (!nand) > + return -ENOMEM; > + > + nand->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 >= NFC_MAX_NSELS) { > + dev_err(dev, "invalid CS: %u\n", tmp); > + return -EINVAL; > + } > + > + if (test_and_set_bit(tmp, &nfc->assigned_cs)) { > + dev_err(dev, "CS %u already assigned\n", tmp); > + return -EINVAL; > + } > + > + nand->sels[i] = tmp; > + } > + > + chip = &nand->chip; > + chip->controller = &nfc->controller; > + > + nand_set_flash_node(chip, np); > + > + nand_set_controller_data(chip, nfc); > + > + chip->options |= NAND_USE_BOUNCE_BUFFER | NAND_NO_SUBPAGE_WRITE; > + chip->bbt_options = NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB; > + > + /* Set default mode in case dt entry is missing. */ > + chip->ecc.mode = NAND_ECC_HW; > + > + mtd = nand_to_mtd(chip); > + mtd->owner = THIS_MODULE; > + mtd->dev.parent = dev; > + > + if (!mtd->name) { > + dev_err(nfc->dev, "NAND label property is mandatory\n"); > + return -EINVAL; > + } > + > + mtd_set_ooblayout(mtd, &rk_nfc_ooblayout_ops); > + rk_nfc_hw_init(nfc); > + ret = nand_scan(chip, nsels); > + if (ret) > + return ret; > + > + if (chip->options & NAND_IS_BOOT_MEDIUM) { > + ret = of_property_read_u32(np, "rockchip,boot-blks", &tmp); > + nand->boot_blks = ret ? 0 : tmp; > + > + ret = of_property_read_u32(np, "rockchip,boot-ecc-strength", > + &tmp); > + nand->boot_ecc = ret ? chip->ecc.strength : tmp; > + } > + > + ret = mtd_device_register(mtd, NULL, 0); > + if (ret) { > + dev_err(dev, "mtd parse partition error\n"); > + nand_release(chip); nand_cleanup() here > + return ret; > + } > + > + list_add_tail(&nand->node, &nfc->chips); > + > + return 0; > +} > + > +static int rk_nfc_nand_chips_init(struct device *dev, struct rk_nfc *nfc) > +{ > + struct device_node *np = dev->of_node; > + struct device_node *nand_np; > + int ret = -EINVAL; > + int tmp; > + > + for_each_child_of_node(np, nand_np) { > + tmp = rk_nfc_nand_chip_init(dev, nfc, nand_np); > + if (tmp) { > + of_node_put(nand_np); > + return ret; > + } > + /* At least one nand chip is initialized. */ > + ret = 0; I think it's more readable if you count up the number of devices initialized by browsing the list of chips handled by the controller (list_is_empty() would work just fine). > + } > + > + return ret; > +} > + > +static struct nfc_cfg nfc_v6_cfg = { > + .type = NFC_V6, > + .ecc_strengths = {60, 40, 24, 16}, > + .ecc_cfgs = { > + 0x00040011, 0x00040001, 0x00000011, 0x00000001, > + }, > + .flctl_off = 0x08, > + .bchctl_off = 0x0C, > + .dma_cfg_off = 0x10, > + .dma_data_buf_off = 0x14, > + .dma_oob_buf_off = 0x18, > + .dma_st_off = 0x1C, > + .bch_st_off = 0x20, > + .randmz_off = 0x150, > + .int_en_off = 0x16C, > + .int_clr_off = 0x170, > + .int_st_off = 0x174, > + .oob0_off = 0x200, > + .oob1_off = 0x230, > + .ecc0 = { > + .err_flag_bit = 2, > + .low = 3, > + .low_mask = 0x1F, > + .low_bn = 5, > + .high = 27, > + .high_mask = 0x1, > + }, > + .ecc1 = { > + .err_flag_bit = 15, > + .low = 16, > + .low_mask = 0x1F, > + .low_bn = 5, > + .high = 29, > + .high_mask = 0x1, > + }, > +}; > + > +static struct nfc_cfg nfc_v8_cfg = { > + .type = NFC_V8, > + .ecc_strengths = {16, 16, 16, 16}, > + .ecc_cfgs = { > + 0x00000001, 0x00000001, 0x00000001, 0x00000001, > + }, > + .flctl_off = 0x08, > + .bchctl_off = 0x0C, > + .dma_cfg_off = 0x10, > + .dma_data_buf_off = 0x14, > + .dma_oob_buf_off = 0x18, > + .dma_st_off = 0x1C, > + .bch_st_off = 0x20, > + .bch_st_off = 0x20, > + .randmz_off = 0x150, > + .int_en_off = 0x16C, > + .int_clr_off = 0x170, > + .int_st_off = 0x174, > + .oob0_off = 0x200, > + .oob1_off = 0x230, > + .ecc0 = { > + .err_flag_bit = 2, > + .low = 3, > + .low_mask = 0x1F, > + .low_bn = 5, > + .high = 27, > + .high_mask = 0x1, > + }, > + .ecc1 = { > + .err_flag_bit = 15, > + .low = 16, > + .low_mask = 0x1F, > + .low_bn = 5, > + .high = 29, > + .high_mask = 0x1, > + }, > +}; > + > +static struct nfc_cfg nfc_v9_cfg = { > + .type = NFC_V9, > + .ecc_strengths = {70, 60, 40, 16}, > + .ecc_cfgs = { > + 0x00000001, 0x06000001, 0x04000001, 0x02000001, > + }, > + .flctl_off = 0x10, > + .bchctl_off = 0x20, > + .dma_cfg_off = 0x30, > + .dma_data_buf_off = 0x34, > + .dma_oob_buf_off = 0x38, > + .dma_st_off = 0x3C, > + .bch_st_off = 0x150, > + .randmz_off = 0x208, > + .int_en_off = 0x120, > + .int_clr_off = 0x124, > + .int_st_off = 0x128, > + .oob0_off = 0x200, > + .oob1_off = 0x204, > + .ecc0 = { > + .err_flag_bit = 2, > + .low = 3, > + .low_mask = 0x7F, > + .low_bn = 7, > + .high = 0, > + .high_mask = 0x0, > + }, > + .ecc1 = { > + .err_flag_bit = 18, > + .low = 19, > + .low_mask = 0x7F, > + .low_bn = 7, > + .high = 0, > + .high_mask = 0x0, > + }, > +}; > + > +static const struct of_device_id rk_nfc_id_table[] = { > + {.compatible = "rockchip,px30-nfc", > + .data = &nfc_v9_cfg }, > + {.compatible = "rockchip,rk2928-nfc", > + .data = &nfc_v6_cfg }, > + {.compatible = "rockchip,rv1108-nfc", > + .data = &nfc_v8_cfg }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, rk_nfc_id_table); > + > +static int rk_nfc_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct rk_nfc *nfc; > + struct resource *res; > + int ret, irq; > + > + nfc = devm_kzalloc(dev, sizeof(*nfc), GFP_KERNEL); > + if (!nfc) > + return -ENOMEM; > + > + nand_controller_init(&nfc->controller); > + INIT_LIST_HEAD(&nfc->chips); > + nfc->controller.ops = &rk_nfc_controller_ops; > + > + nfc->cfg = of_device_get_match_data(dev); > + nfc->dev = dev; > + > + init_completion(&nfc->done); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + nfc->regs = devm_ioremap_resource(dev, res); There is a devm_platform_ioremap_resource for that > + if (IS_ERR(nfc->regs)) { > + ret = PTR_ERR(nfc->regs); > + goto release_nfc; > + } > + > + nfc->clk.nfc_clk = devm_clk_get(dev, "nfc"); > + if (IS_ERR(nfc->clk.nfc_clk)) { > + dev_dbg(dev, "no nfc clk\n"); > + /* Some old device, sush as rk3066, has no nfc clk. */ I would drop the dbg trace and reining nfc_clk to NULL. Also it might be worth checking the compatible here to ensure this is allowed. > + } > + > + nfc->clk.ahb_clk = devm_clk_get(dev, "ahb"); > + if (IS_ERR(nfc->clk.ahb_clk)) { > + dev_err(dev, "no ahb clk\n"); > + ret = PTR_ERR(nfc->clk.ahb_clk); > + goto release_nfc; > + } > + > + ret = rk_nfc_enable_clk(dev, &nfc->clk); > + if (ret) > + goto release_nfc; > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > + dev_err(dev, "no nfc irq resource\n"); > + ret = -EINVAL; > + goto clk_disable; > + } > + > + writel(0, nfc->regs + nfc->cfg->int_en_off); > + ret = devm_request_irq(dev, irq, rk_nfc_irq, 0x0, "rk-nand", nfc); > + if (ret) { > + dev_err(dev, "failed to request nfc irq\n"); > + goto clk_disable; > + } > + > + platform_set_drvdata(pdev, nfc); > + > + ret = rk_nfc_nand_chips_init(dev, nfc); > + if (ret) { > + dev_err(dev, "failed to init nand chips\n"); > + goto clk_disable; > + } > + return 0; > + > +clk_disable: > + rk_nfc_disable_clk(&nfc->clk); > +release_nfc: > + return ret; > +} > + > +static int rk_nfc_remove(struct platform_device *pdev) > +{ > + struct rk_nfc *nfc = platform_get_drvdata(pdev); > + struct rk_nfc_nand_chip *nand; > + > + while (!list_empty(&nfc->chips)) { > + nand = list_first_entry(&nfc->chips, struct rk_nfc_nand_chip, > + node); > + nand_release(&nand->chip); nand_release has been removed (in Linus' branch since this week). > + list_del(&nand->node); > + } > + > + rk_nfc_disable_clk(&nfc->clk); > + > + return 0; > +} > + > +static int __maybe_unused rk_nfc_suspend(struct device *dev) > +{ > + struct rk_nfc *nfc = dev_get_drvdata(dev); > + > + rk_nfc_disable_clk(&nfc->clk); > + > + return 0; > +} > + > +static int __maybe_unused rk_nfc_resume(struct device *dev) > +{ > + struct rk_nfc *nfc = dev_get_drvdata(dev); > + struct rk_nfc_nand_chip *nand; > + struct nand_chip *chip; > + int ret; > + u32 i; > + > + ret = rk_nfc_enable_clk(dev, &nfc->clk); > + if (ret) > + return ret; > + > + /* Reset NAND chip if VCC was powered off. */ > + list_for_each_entry(nand, &nfc->chips, node) { > + chip = &nand->chip; > + for (i = 0; i < nand->nsels; i++) > + nand_reset(chip, i); > + } > + > + return 0; > +} > + > +static const struct dev_pm_ops rk_nfc_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(rk_nfc_suspend, rk_nfc_resume) > +}; > + > +static struct platform_driver rk_nfc_driver = { > + .probe = rk_nfc_probe, > + .remove = rk_nfc_remove, > + .driver = { > + .name = "rockchip-nfc", > + .of_match_table = rk_nfc_id_table, > + .pm = &rk_nfc_pm_ops, > + }, > +}; > + > +module_platform_driver(rk_nfc_driver); > + > +MODULE_LICENSE("Dual MIT/GPL"); > +MODULE_AUTHOR("Yifeng Zhao <yifeng.zhao@xxxxxxxxxxxxxx>"); > +MODULE_DESCRIPTION("Rockchip Nand Flash Controller Driver"); > +MODULE_ALIAS("platform:rockchip-nand-controller"); Thanks, Miquèl