Hi Yifeng, Yifeng Zhao <yifeng.zhao@xxxxxxxxxxxxxx> wrote on Tue, 3 Mar 2020 17:47:34 +0800: > This driver supports Rockchip NFC (NAND Flash Controller) found on RK3308, > RK3368, RKPX30, RV1108 and other SOCs. The driver has been tested using > 8-bit NAND interface on the ARM based RK3308 platform. > > Support Rockchip NFC versions: > - V6: ECC 16, 24, 40 and 60 bits per 1KB data. Found on RK3066, RK3368, > RK3229, RK3188, RK3288, RK3128, RKPX3SE, RKPx3, RKPX5, RK3036 and > RK3126. > - V8: ECC 16 bits per 1KB data. Found on RV1108/7, RK3308. > - V9: ECC 16, 40, 60 and 70 bits per 1KB data. Found on RK3326, RKPX30. > > Support feature: > - Read full page data by DMA. > - Support HW ECC(one step is 1KB). > - Support 2 - 32K page size. > - Support 4 CS(depend on Soc) > > Limitations: > - Unsupport 512B ecc step. > - Raw page read and write without ecc redundancy code. So could not support > raw data dump and restore. > - Untested on some SOCs. > - Unsupport subpage. > - Unsupport randomizer. > - The original bad block mask is not supported. It is recommended to use > the BBT(bad block table). > > Signed-off-by: Yifeng Zhao <yifeng.zhao@xxxxxxxxxxxxxx> > --- > > Changes in v3: None > Changes in v2: > - Fix compile error. > - Include header files sorted by file name > > drivers/mtd/nand/raw/Kconfig | 7 + > drivers/mtd/nand/raw/Makefile | 1 + > drivers/mtd/nand/raw/rockchip_nand.c | 1229 ++++++++++++++++++++++++++ > 3 files changed, 1237 insertions(+) > create mode 100644 drivers/mtd/nand/raw/rockchip_nand.c > > diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig > index a80a46bb5b8b..8313b12a9d85 100644 > --- a/drivers/mtd/nand/raw/Kconfig > +++ b/drivers/mtd/nand/raw/Kconfig > @@ -433,6 +433,13 @@ config MTD_NAND_MESON > Enables support for NAND controller on Amlogic's Meson SoCs. > This controller is found on Meson SoCs. > > +config MTD_NAND_ROCKCHIP > + tristate "Rockchip NAND controller" > + depends on ARCH_ROCKCHIP || COMPILE_TEST > + depends on HAS_IOMEM > + help > + Enables support for NAND controller on Rockchip SoCs. > + > config MTD_NAND_GPIO > tristate "GPIO assisted NAND controller" > depends on GPIOLIB || COMPILE_TEST > diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile > index 2d136b158fb7..8bafa59b8940 100644 > --- a/drivers/mtd/nand/raw/Makefile > +++ b/drivers/mtd/nand/raw/Makefile > @@ -58,6 +58,7 @@ obj-$(CONFIG_MTD_NAND_TEGRA) += tegra_nand.o > obj-$(CONFIG_MTD_NAND_STM32_FMC2) += stm32_fmc2_nand.o > obj-$(CONFIG_MTD_NAND_MESON) += meson_nand.o > obj-$(CONFIG_MTD_NAND_CADENCE) += cadence-nand-controller.o > +obj-$(CONFIG_MTD_NAND_ROCKCHIP) += rockchip_nand.o > > nand-objs := nand_base.o nand_legacy.o nand_bbt.o nand_timings.o nand_ids.o > nand-objs += nand_onfi.o > diff --git a/drivers/mtd/nand/raw/rockchip_nand.c b/drivers/mtd/nand/raw/rockchip_nand.c > new file mode 100644 > index 000000000000..efeda609fbf2 > --- /dev/null > +++ b/drivers/mtd/nand/raw/rockchip_nand.c > @@ -0,0 +1,1229 @@ > +// SPDX-License-Identifier: GPL-2.0 OR MIT > +/* > + * Rockchip NAND Flash controller driver. > + * Copyright (C) 2020 Rockchip Inc. > + * Authors: Yifeng Zhao <yifeng.zhao@xxxxxxxxxxxxxx> > + */ > + > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/dma-mapping.h> > +#include <linux/dmaengine.h> > +#include <linux/interrupt.h> > +#include <linux/iopoll.h> > +#include <linux/mtd/rawnand.h> > +#include <linux/mtd/mtd.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > + > +/* > + * NFC Page Data Layout: > + * 1024 Bytes Data + 4Bytes sys data + 28Bytes~124Bytes ecc + > + * 1024 Bytes Data + 4Bytes sys data + 28Bytes~124Bytes ecc + > + * ...... > + * NAND Page Data Layout: > + * 1024 * n Data + m Bytes oob > + * Original Bad Block Mask Location: > + * first byte of oob(spare) > + * nand_chip->oob_poi data layout: > + * 4Bytes sys data + .... + 4Bytes sys data + ecc data > + */ > + > +/* NAND controller register definition */ > +#define NFC_VERSION_9 0x56393030 > +#define NFC_READ (0) > +#define NFC_WRITE (1) > +#define NFC_FMCTL (0x00) > +#define FMCTL_CE_SEL_M 0xFF > +#define FMCTL_CE_SEL(x) (1 << (x)) > +#define FMCTL_WP BIT(8) > +#define FMCTL_RDY BIT(9) > +#define NFC_FMWAIT (0x04) > +#define NFC_FLCTL_V6 (0x08) > +#define NFC_FLCTL_V9 (0x10) > +#define FLCTL_RST BIT(0) > +#define FLCTL_WR (1) /* 0: read, 1: write */ > +#define FLCTL_XFER_ST BIT(2) > +#define FLCTL_XFER_EN BIT(3) > +#define FLCTL_ACORRECT BIT(10) /* auto correct error bits */ > +#define FLCTL_XFER_READY BIT(20) > +#define FLCTL_XFER_SECTOR (22) > +#define FLCTL_TOG_FIX BIT(29) > +#define NFC_BCHCTL_V6 (0x0C) > +#define BCHCTL_BANK_M (7 << 5) > +#define BCHCTL_BANK (5) > +#define NFC_BCHCTL_V9 (0x20) > +#define NFC_DMA_CFG_V6 (0x10) > +#define NFC_DMA_CFG_V9 (0x30) > +#define DMA_ST BIT(0) > +#define DMA_WR (1) /* 0: write, 1: read */ > +#define DMA_EN BIT(2) > +#define DMA_AHB_SIZE (3) /* 0: 1, 1: 2, 2: 4 */ > +#define DMA_BURST_SIZE (6) /* 0: 1, 3: 4, 5: 8, 7: 16 */ > +#define DMA_INC_NUM (9) /* 1 - 16 */ > +#define NFC_DMA_DATA_BUF_V6 (0x14) > +#define NFC_DMA_DATA_BUF_V9 (0x34) > +#define NFC_DMA_OOB_BUF_V6 (0x18) > +#define NFC_DMA_OOB_BUF_V9 (0x38) > +#define NFC_DMA_ST_V6 (0x1C) > +#define NFC_DMA_ST_V9 (0x3C) > +#define NFC_BCH_ST_V6 (0x20) > +#define NFC_BCH_ST_V9 (0x150) > +#define BCH_ST_ERR0_V6 BIT(2) > +#define BCH_ST_ERR1_V6 BIT(15) > +#define BCH_ST_ERR0_V9 BIT(2) > +#define BCH_ST_ERR1_V9 BIT(18) > +#define ECC_ERR_CNT0_V6(x) (((((x) & (0x1F << 3)) >> 3) \ > + | (((x) & (1 << 27)) >> 22)) & 0x3F) > +#define ECC_ERR_CNT1_V6(x) (((((x) & (0x1F << 16)) >> 16) \ > + | (((x) & (1 << 29)) >> 24)) & 0x3F) > +#define ECC_ERR_CNT0_V9(x) (((x) & (0x7F << 3)) >> 3) > +#define ECC_ERR_CNT1_V9(x) (((x) & (0x7F << 19)) >> 19) > +#define NFC_RANDMZ_V6 (0x150) > +#define NFC_RANDMZ_V9 (0x208) > +#define NFC_VER_V6 (0x160) > +#define NFC_VER_V9 (0x80) > +#define NFC_INTEN_V6 (0x16C) > +#define NFC_INTEN_V9 (0x120) > +#define NFC_INTCLR_V6 (0x170) > +#define NFC_INTCLR_V9 (0x124) > +#define NFC_INTST_V6 (0x174) > +#define NFC_INTST_V9 (0x128) > +#define INT_DMA BIT(0) > +#define NFC_OOB0_V6 (0x200) > +#define NFC_OOB0_V9 (0x200) > +#define NFC_OOB1_V6 (0x230) > +#define NFC_OOB1_V9 (0x204) > +#define NFC_BANK (0x800) > +#define BANK_DATA (0x00) > +#define BANK_ADDR (0x04) > +#define BANK_CMD (0x08) > +#define NFC_SRAM0 (0x1000) > +#define NFC_SRAM1 (0x1400) > + > +#define THIS_NAME "rk-nand" > + > +#define RK_TIMEOUT (500000) > +#define RK_NAND_MAX_NSELS (4) /* Some Soc only has 1 or 2 CSs */ > +#define NFC_SYS_DATA_SIZE (4) /* 4 bytes sys data in oob pre 1024 data */ > +#define RK_DEFAULT_CLOCK_RATE (150 * 1000 * 1000) /* 150 Mhz*/ > +#define ACCTIMING(csrw, rwpw, rwcs) ((csrw) << 12 | (rwpw) << 5 | (rwcs)) > + > +struct rk_nfc_nand_chip { > + struct list_head node; > + struct nand_chip nand; > + > + u32 spare_per_sector; > + u32 oob_buf_per_sector; > + > + int nsels; > + u8 sels[0]; > + /* nothing after this field */ > +}; > + > +struct rk_nfc_clk { > + int nfc_rate; > + struct clk *nfc_clk; > + struct clk *ahb_clk; > +}; > + > +struct rk_nfc { > + struct nand_controller controller; > + struct rk_nfc_clk clk; > + > + struct device *dev; > + void __iomem *regs; > + int nfc_version; > + int max_ecc_strength; > + int selected_bank; > + int band_offset; > + > + struct completion done; > + struct list_head chips; > + > + u8 *buffer; > + u8 *page_buf; > + u32 *oob_buf; > + > + unsigned long assigned_cs; > +}; > + > +static inline struct rk_nfc_nand_chip *to_rk_nand(struct nand_chip *nand) > +{ > + return container_of(nand, struct rk_nfc_nand_chip, nand); > +} > + > +static inline u8 *data_ptr(struct nand_chip *chip, const u8 *p, int i) > +{ > + return (u8 *)p + i * chip->ecc.size; > +} > + > +static inline u8 *oob_ptr(struct nand_chip *chip, int i) > +{ > + u8 *poi; > + > + poi = chip->oob_poi + i * NFC_SYS_DATA_SIZE; > + > + return poi; > +} > + > +static inline int rk_data_len(struct nand_chip *chip) > +{ > + struct rk_nfc_nand_chip *rk_nand = to_rk_nand(chip); > + > + return chip->ecc.size + rk_nand->spare_per_sector; > +} > + > +static inline u8 *rk_data_ptr(struct nand_chip *chip, int i) > +{ > + struct rk_nfc *nfc = nand_get_controller_data(chip); > + > + return nfc->buffer + i * rk_data_len(chip); > +} > + > +static inline u8 *rk_oob_ptr(struct nand_chip *chip, int i) > +{ > + struct rk_nfc *nfc = nand_get_controller_data(chip); > + > + return nfc->buffer + i * rk_data_len(chip) + chip->ecc.size; > +} > + > +static inline void nfc_writel(struct rk_nfc *nfc, u32 val, u32 reg) > +{ > + writel(val, nfc->regs + reg); > +} I don't think these nfc_read/write{l,w,b} are useful... > + > +static inline void nfc_writew(struct rk_nfc *nfc, u16 val, u32 reg) > +{ > + writew(val, nfc->regs + reg); > +} > + > +static inline void nfc_writeb(struct rk_nfc *nfc, u8 val, u32 reg) > +{ > + writeb(val, nfc->regs + reg); > +} > + > +static inline u32 nfc_readl(struct rk_nfc *nfc, u32 reg) > +{ > + return readl_relaxed(nfc->regs + reg); > +} > + > +static inline u16 nfc_readw(struct rk_nfc *nfc, u32 reg) > +{ > + return readw_relaxed(nfc->regs + reg); > +} > + > +static inline u8 nfc_readb(struct rk_nfc *nfc, u32 reg) > +{ > + return readb_relaxed(nfc->regs + reg); > +} > + > +static void rk_nfc_select_chip(struct nand_chip *nand, int chip) > +{ > + struct rk_nfc *nfc = nand_get_controller_data(nand); > + struct rk_nfc_nand_chip *rk_nand = to_rk_nand(nand); > + u32 val; > + > + if (chip < 0) { > + nfc->selected_bank = -1; > + return; > + } > + > + nfc->selected_bank = rk_nand->sels[chip]; > + nfc->band_offset = NFC_BANK + nfc->selected_bank * 0x100; > + > + val = nfc_readl(nfc, NFC_FMCTL); > + val &= ~FMCTL_CE_SEL_M; > + val |= FMCTL_CE_SEL(nfc->selected_bank); > + > + nfc_writel(nfc, val, NFC_FMCTL); > +} > + > +static int rk_nfc_dev_ready(struct nand_chip *nand) > +{ > + struct rk_nfc *nfc = nand_get_controller_data(nand); > + > + if (nfc_readl(nfc, NFC_FMCTL) & FMCTL_RDY) > + return 1; > + > + return 0; > +} > + > +static void rk_nfc_cmd_ctrl(struct nand_chip *chip, int dat, > + unsigned int ctrl) > +{ > + struct rk_nfc *nfc = nand_get_controller_data(chip); > + int reg_offset = nfc->band_offset; > + > + if (ctrl & NAND_ALE) > + reg_offset += BANK_ADDR; > + else if (ctrl & NAND_CLE) > + reg_offset += BANK_CMD; > + > + if (dat != NAND_CMD_NONE) > + nfc_writeb(nfc, dat & 0xFF, reg_offset); > +} > + > +static inline void rk_nfc_wait_ioready(struct rk_nfc *nfc) > +{ > + int rc; > + u32 val; > + > + rc = readl_poll_timeout_atomic(nfc->regs + NFC_FMCTL, val, > + val & FMCTL_RDY, 10, RK_TIMEOUT); > + if (rc < 0) > + dev_err(nfc->dev, "data not ready\n"); > +} > + > +static inline u8 rk_nfc_read_byte(struct nand_chip *chip) > +{ > + struct rk_nfc *nfc = nand_get_controller_data(chip); > + > + return nfc_readb(nfc, nfc->band_offset + BANK_DATA); > +} > + > +static void rk_nfc_read_buf(struct nand_chip *chip, u8 *buf, int len) > +{ > + int i; > + > + for (i = 0; i < len; i++) > + buf[i] = rk_nfc_read_byte(chip); > +} > + > +static void rk_nfc_write_byte(struct nand_chip *chip, u8 byte) > +{ > + struct rk_nfc *nfc = nand_get_controller_data(chip); > + > + nfc_writeb(nfc, byte, nfc->band_offset + BANK_DATA); > +} > + > +static void rk_nfc_write_buf(struct nand_chip *chip, const u8 *buf, int len) > +{ > + int i; > + > + for (i = 0; i < len; i++) > + rk_nfc_write_byte(chip, buf[i]); > +} > + > +static int rk_nfc_setup_data_interface(struct nand_chip *chip, int csline, > + const struct nand_data_interface *conf) > +{ > + struct rk_nfc *nfc = nand_get_controller_data(chip); > + const struct nand_sdr_timings *timings; > + u32 rate, tc2rw, trwpw, trw2c; > + u32 temp; > + > + if (csline == NAND_DATA_IFACE_CHECK_ONLY) > + return 0; > + > + /* not onfi nand flash */ > + if (!chip->parameters.onfi) > + return 0; > + > + timings = nand_get_sdr_timings(conf); > + if (IS_ERR(timings)) > + return -ENOTSUPP; > + > + rate = clk_get_rate(nfc->clk.nfc_clk); > + > + /* turn clock rate into KHZ */ > + rate /= 1000; > + > + tc2rw = trw2c = 1; tc2rw = 1; trw2c = 1; > + > + trwpw = max(timings->tWC_min, timings->tRC_min) / 1000; > + trwpw = DIV_ROUND_UP(trwpw * rate, 1000000); > + > + temp = timings->tREA_max / 1000; > + temp = DIV_ROUND_UP(temp * rate, 1000000); > + > + if (trwpw < temp) > + trwpw = temp; > + > + if (trwpw > 6) { What is 6 ? Would deserve a define and an explanation! > + tc2rw++; > + trw2c++; > + trwpw -= 2; > + } > + > + /* > + * ACCON: access timing control register > + * ------------------------------------- > + * 31:18: reserved > + * 17:12: csrw, clock cycles from the falling edge of CSn to the > + falling edge of RDn or WRn > + * 11:11: reserved > + * 10:05: rwpw, the width of RDn or WRn in processor clock cycles > + * 04:00: rwcs, clock cycles from the rising edge of RDn or WRn to the > + rising edge of CSn > + */ > + temp = ACCTIMING(tc2rw, trwpw, trw2c); > + nfc_writel(nfc, temp, NFC_FMWAIT); > + > + return 0; > +} > + A comment here explaining what the next function does and why would be nice. > +static void rk_nfc_format_page(struct mtd_info *mtd, const u8 *buf) > +{ > + struct nand_chip *chip = mtd_to_nand(mtd); > + struct rk_nfc *nfc = nand_get_controller_data(chip); > + u32 i; > + > + memset(nfc->buffer, 0xff, mtd->writesize + mtd->oobsize); > + swap(chip->oob_poi[0], chip->oob_poi[7]); > + for (i = 0; i < chip->ecc.steps; i++) { > + if (buf) > + memcpy(rk_data_ptr(chip, i), data_ptr(chip, buf, i), > + chip->ecc.size); > + > + memcpy(rk_oob_ptr(chip, i), oob_ptr(chip, i), > + NFC_SYS_DATA_SIZE); > + } > +} > + > +static void rk_nfc_xfer_start(struct rk_nfc *nfc, u8 rw, u8 n_KB, > + dma_addr_t dma_data, dma_addr_t dma_oob) > +{ > + u32 dma_reg, fl_reg, bch_reg; > + > + dma_reg = DMA_ST | ((!rw) << DMA_WR) | DMA_EN | (2 << DMA_AHB_SIZE) | > + (7 << DMA_BURST_SIZE) | (16 << DMA_INC_NUM); > + > + fl_reg = (rw << FLCTL_WR) | FLCTL_XFER_EN | FLCTL_ACORRECT | > + (n_KB << FLCTL_XFER_SECTOR) | FLCTL_TOG_FIX; > + > + if (nfc->nfc_version == 6) { I would prefer using switch statements any time you check the version. The version should be an enum. You can also define a platform data structure for the register offsets that have the same name, but not necessarily the same offset. Then you can reference the right value directly. eg. struct rk_nfc_plat_data { u32 nfc_bchctl_off; ... }; struct rk_nfc_plat_data rk_nfc_v6_plat_data = { nfc_bchctl_off = ...; ... }; bch_reg = readl(pdata->nfc_bchctl_off); > + bch_reg = nfc_readl(nfc, NFC_BCHCTL_V6); > + bch_reg = (bch_reg & (~BCHCTL_BANK_M)) | > + (nfc->selected_bank << BCHCTL_BANK); > + nfc_writel(nfc, bch_reg, NFC_BCHCTL_V6); > + > + nfc_writel(nfc, dma_reg, NFC_DMA_CFG_V6); > + nfc_writel(nfc, (u32)dma_data, NFC_DMA_DATA_BUF_V6); > + nfc_writel(nfc, (u32)dma_oob, NFC_DMA_OOB_BUF_V6); > + nfc_writel(nfc, fl_reg, NFC_FLCTL_V6); > + fl_reg |= FLCTL_XFER_ST; > + nfc_writel(nfc, fl_reg, NFC_FLCTL_V6); > + } else { > + nfc_writel(nfc, dma_reg, NFC_DMA_CFG_V9); > + nfc_writel(nfc, (u32)dma_data, NFC_DMA_DATA_BUF_V9); > + nfc_writel(nfc, (u32)dma_oob, NFC_DMA_OOB_BUF_V9); > + nfc_writel(nfc, fl_reg, NFC_FLCTL_V9); > + fl_reg |= FLCTL_XFER_ST; > + nfc_writel(nfc, fl_reg, NFC_FLCTL_V9); > + } > +} > + > +static int rk_nfc_wait_for_xfer_done(struct rk_nfc *nfc) > +{ > + u32 reg; > + int ret = 0; > + void __iomem *ptr; > + > + if (nfc->nfc_version == 6) > + ptr = nfc->regs + NFC_FLCTL_V6; > + else > + ptr = nfc->regs + NFC_FLCTL_V9; > + > + ret = readl_poll_timeout_atomic(ptr, reg, > + reg & FLCTL_XFER_READY, > + 10, RK_TIMEOUT); > + if (ret) > + dev_err(nfc->dev, "timeout reg=%x\n", reg); > + > + return ret; > +} > + > +static int rk_nfc_write_page(struct mtd_info *mtd, struct nand_chip *chip, > + const u8 *buf, int page, int raw) > +{ > + struct rk_nfc *nfc = nand_get_controller_data(chip); > + struct nand_ecc_ctrl *ecc = &chip->ecc; > + u8 *oob; > + dma_addr_t dma_data, dma_oob; > + int oob_step = (ecc->bytes > 60) ? 128 : 64; > + int pages_per_blk = mtd->erasesize / mtd->writesize; > + u32 reg; > + int ret = 0, i; > + > + nand_prog_page_begin_op(chip, page, 0, NULL, 0); > + > + if (!raw) { > + memcpy(nfc->page_buf, buf, mtd->writesize); > + memset(nfc->oob_buf, 0xff, oob_step * ecc->steps); > + /* > + * The first 8 blocks is stored loader, the first > + * 32 bits of oob need link to next page address > + * in the same block for Bootrom. > + * Swap the first oob with the seventh oob, > + * and bad block mask save at seventh oob. > + */ > + swap(chip->oob_poi[0], chip->oob_poi[7]); > + for (i = 0; i < ecc->steps; i++) { > + oob = chip->oob_poi + i * NFC_SYS_DATA_SIZE; > + reg = (oob[2] << 16) | (oob[3] << 24); > + if (!i && page < pages_per_blk * 8) > + reg |= (page & (pages_per_blk - 1)) * 4; > + else > + reg |= oob[0] | (oob[1] << 8); > + > + if (nfc->nfc_version == 6) > + nfc->oob_buf[i * oob_step / 4] = reg; > + else > + nfc->oob_buf[i] = reg; > + } > + > + dma_data = dma_map_single(nfc->dev, (void *)nfc->page_buf, > + mtd->writesize, DMA_TO_DEVICE); > + dma_oob = dma_map_single(nfc->dev, nfc->oob_buf, > + ecc->steps * oob_step, > + DMA_TO_DEVICE); > + > + init_completion(&nfc->done); > + if (nfc->nfc_version == 6) > + nfc_writel(nfc, INT_DMA, NFC_INTEN_V6); > + else > + nfc_writel(nfc, INT_DMA, NFC_INTEN_V9); > + > + rk_nfc_xfer_start(nfc, NFC_WRITE, ecc->steps, dma_data, > + dma_oob); > + ret = wait_for_completion_timeout(&nfc->done, > + msecs_to_jiffies(100)); > + if (!ret) > + ret = -ETIMEDOUT; > + ret = rk_nfc_wait_for_xfer_done(nfc); > + > + dma_unmap_single(nfc->dev, dma_data, mtd->writesize, > + DMA_TO_DEVICE); > + dma_unmap_single(nfc->dev, dma_oob, ecc->steps * oob_step, > + DMA_TO_DEVICE); > + } else { > + rk_nfc_write_buf(chip, buf, mtd->writesize + + mtd->oobsize); > + } > + > + if (ret) > + return ret; > + > + return nand_prog_page_end_op(chip); > +} > + > +static int rk_nfc_write_page_raw(struct nand_chip *chip, const u8 *buf, > + int oob_on, int page) > +{ > + struct mtd_info *mtd = nand_to_mtd(chip); > + struct rk_nfc *nfc = nand_get_controller_data(chip); > + > + rk_nfc_format_page(mtd, buf); > + return rk_nfc_write_page(mtd, chip, nfc->buffer, page, 1); I think you should avoid calling ->write_page. You will avoid an indentation level in this function and clarify what write_page_raw do. Same for read, and the _oob alternative. Also, I'm sure write_buf does not need to be exported and you can just move the actual code in this function. > +} > + > +static int rk_nfc_write_oob_std(struct nand_chip *chip, int page) > +{ > + return rk_nfc_write_page_raw(chip, NULL, 1, page); > +} > + > +static int rk_nfc_read_page(struct mtd_info *mtd, struct nand_chip *chip, > + u32 data_offs, u32 readlen, > + u8 *buf, int page, int raw) > +{ > + struct rk_nfc *nfc = nand_get_controller_data(chip); > + struct nand_ecc_ctrl *ecc = &chip->ecc; > + dma_addr_t dma_data, dma_oob; > + int oob_step = (ecc->bytes > 60) ? 128 : 64; > + int bitflips = 0; > + int ret, i, bch_st; > + u8 *oob; > + u32 tmp; > + > + nand_read_page_op(chip, page, 0, NULL, 0); > + if (!raw) { > + dma_data = dma_map_single(nfc->dev, nfc->page_buf, > + mtd->writesize, > + DMA_FROM_DEVICE); > + dma_oob = dma_map_single(nfc->dev, nfc->oob_buf, > + ecc->steps * oob_step, > + DMA_FROM_DEVICE); > + init_completion(&nfc->done); > + if (nfc->nfc_version == 6) > + nfc_writel(nfc, INT_DMA, NFC_INTEN_V6); > + else > + nfc_writel(nfc, INT_DMA, NFC_INTEN_V9); > + rk_nfc_xfer_start(nfc, NFC_READ, ecc->steps, dma_data, > + dma_oob); > + ret = wait_for_completion_timeout(&nfc->done, > + msecs_to_jiffies(100)); > + if (!ret) > + dev_warn(nfc->dev, "read ahb/dma done timeout\n"); > + rk_nfc_wait_for_xfer_done(nfc); > + dma_unmap_single(nfc->dev, dma_data, mtd->writesize, > + DMA_FROM_DEVICE); > + dma_unmap_single(nfc->dev, dma_oob, ecc->steps * oob_step, > + DMA_FROM_DEVICE); > + > + for (i = 0; i < ecc->steps; i++) { > + oob = chip->oob_poi + i * NFC_SYS_DATA_SIZE; > + if (nfc->nfc_version == 6) > + tmp = nfc->oob_buf[i * oob_step / 4]; > + else > + tmp = nfc->oob_buf[i]; > + *oob++ = (u8)tmp; > + *oob++ = (u8)(tmp >> 8); > + *oob++ = (u8)(tmp >> 16); > + *oob++ = (u8)(tmp >> 24); > + } > + swap(chip->oob_poi[0], chip->oob_poi[7]); > + if (nfc->nfc_version == 6) { > + for (i = 0; i < ecc->steps / 2; i++) { > + bch_st = nfc_readl(nfc, NFC_BCH_ST_V6 + i * 4); > + if (bch_st & BCH_ST_ERR0_V6 || > + bch_st & BCH_ST_ERR1_V6) { > + mtd->ecc_stats.failed++; > + bitflips = -1; > + } else { > + ret = ECC_ERR_CNT0_V6(bch_st); > + mtd->ecc_stats.corrected += ret; > + bitflips = max_t(u32, bitflips, ret); > + > + ret = ECC_ERR_CNT1_V6(bch_st); > + mtd->ecc_stats.corrected += ret; > + bitflips = max_t(u32, bitflips, ret); > + } > + } > + } else { > + for (i = 0; i < ecc->steps / 2; i++) { > + bch_st = nfc_readl(nfc, NFC_BCH_ST_V9 + i * 4); > + if (bch_st & BCH_ST_ERR0_V9 || > + bch_st & BCH_ST_ERR0_V9) { > + mtd->ecc_stats.failed++; > + bitflips = -1; > + } else { > + ret = ECC_ERR_CNT0_V9(bch_st); > + mtd->ecc_stats.corrected += ret; > + bitflips = max_t(u32, bitflips, ret); > + looks strange, a switch would be better probably. > + ret = ECC_ERR_CNT1_V9(bch_st); > + mtd->ecc_stats.corrected += ret; > + bitflips = max_t(u32, bitflips, ret); > + } > + } > + } > + memcpy(buf, nfc->page_buf, mtd->writesize); > + > + if (bitflips == -1) > + dev_err(nfc->dev, "read_page %x %x %x %x %x %x\n", > + page, bitflips, bch_st, ((u32 *)buf)[0], > + ((u32 *)buf)[1], (u32)dma_data); > + } else { > + rk_nfc_read_buf(chip, buf, mtd->writesize + mtd->oobsize); > + } space > + return bitflips; > +} > + > +static int rk_nfc_write_page_hwecc(struct nand_chip *chip, const u8 *buf, > + int oob_on, int page) > +{ > + return rk_nfc_write_page(nand_to_mtd(chip), chip, buf, page, 0); > +} What is the purpose of this indirection? > + > +static int rk_nfc_read_page_hwecc(struct nand_chip *chip, u8 *p, int oob_on, > + int pg) > +{ > + struct mtd_info *mtd = nand_to_mtd(chip); > + > + return rk_nfc_read_page(mtd, chip, 0, mtd->writesize, p, pg, 0); > +} > + > +static int rk_nfc_read_page_raw(struct nand_chip *chip, u8 *buf, int oob_on, > + int page) > +{ > + struct mtd_info *mtd = nand_to_mtd(chip); > + struct rk_nfc *nfc = nand_get_controller_data(chip); > + int i, ret; > + > + ret = rk_nfc_read_page(mtd, chip, 0, mtd->writesize, nfc->buffer, > + page, 1); > + if (ret < 0) > + return ret; > + > + for (i = 0; i < chip->ecc.steps; i++) { > + memcpy(oob_ptr(chip, i), rk_oob_ptr(chip, i), > + NFC_SYS_DATA_SIZE); > + > + if (buf) > + memcpy(data_ptr(chip, buf, i), rk_data_ptr(chip, i), > + chip->ecc.size); > + } > + swap(chip->oob_poi[0], chip->oob_poi[7]); > + > + return ret; > +} > + > +static int rk_nfc_read_oob_std(struct nand_chip *chip, int page) > +{ > + return rk_nfc_read_page_raw(chip, NULL, 1, page); > +} > + > +static inline void rk_nfc_hw_init(struct rk_nfc *nfc) > +{ > + u32 val; > + > + val = nfc_readl(nfc, NFC_VER_V9); > + if (val == NFC_VERSION_9) { > + nfc->nfc_version = 9; > + nfc->max_ecc_strength = 70; > + } else { > + nfc->nfc_version = 6; > + val = nfc_readl(nfc, NFC_VER_V6); > + if (val == 0x801) > + nfc->max_ecc_strength = 16; > + else > + nfc->max_ecc_strength = 60; > + } > + > + /* disable flash wp */ > + nfc_writel(nfc, FMCTL_WP, NFC_FMCTL); > + /* config default timing */ > + nfc_writel(nfc, 0x1081, NFC_FMWAIT); > + /* disable randomizer and dma */ > + > + if (nfc->nfc_version == 6) { > + nfc_writel(nfc, 0, NFC_RANDMZ_V6); > + nfc_writel(nfc, 0, NFC_DMA_CFG_V6); > + nfc_writel(nfc, FLCTL_RST, NFC_FLCTL_V6); > + } else { > + nfc_writel(nfc, 0, NFC_RANDMZ_V9); > + nfc_writel(nfc, 0, NFC_DMA_CFG_V9); > + nfc_writel(nfc, FLCTL_RST, NFC_FLCTL_V9); > + } > +} > + > +static irqreturn_t rk_nfc_irq(int irq, void *id) > +{ > + struct rk_nfc *nfc = id; > + u32 sta, ien; > + > + if (nfc->nfc_version == 6) { > + sta = nfc_readl(nfc, NFC_INTST_V6); > + ien = nfc_readl(nfc, NFC_INTEN_V6); > + } else { > + sta = nfc_readl(nfc, NFC_INTST_V9); > + ien = nfc_readl(nfc, NFC_INTEN_V9); > + } > + > + if (!(sta & ien)) > + return IRQ_NONE; > + if (nfc->nfc_version == 6) { > + nfc_writel(nfc, sta, NFC_INTCLR_V6); > + nfc_writel(nfc, ~sta & ien, NFC_INTEN_V6); > + } else { > + nfc_writel(nfc, sta, NFC_INTCLR_V9); > + nfc_writel(nfc, ~sta & ien, NFC_INTEN_V9); > + } > + complete(&nfc->done); > + > + return IRQ_HANDLED; > +} > + > +static int rk_nfc_enable_clk(struct device *dev, struct rk_nfc_clk *clk) > +{ > + int ret; > + > + ret = clk_prepare_enable(clk->nfc_clk); > + if (ret) { > + dev_err(dev, "failed to enable nfc clk\n"); > + return ret; > + } > + > + ret = clk_prepare_enable(clk->ahb_clk); > + if (ret) { > + dev_err(dev, "failed to enable ahb clk\n"); > + clk_disable_unprepare(clk->nfc_clk); > + return ret; > + } > + > + return 0; > +} > + > +static void rk_nfc_disable_clk(struct rk_nfc_clk *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, > +}; > + > +static int rk_nfc_hw_ecc_setup(struct mtd_info *mtd, > + struct nand_ecc_ctrl *ecc, > + uint32_t strength) > +{ > + struct nand_chip *nand = mtd_to_nand(mtd); > + struct rk_nfc *nfc = nand_get_controller_data(nand); > + u32 reg; > + > + ecc->strength = strength; > + ecc->steps = mtd->writesize / ecc->size; > + ecc->bytes = DIV_ROUND_UP(ecc->strength * 14, 8); > + /* HW ECC always work with even numbers of ECC bytes */ > + ecc->bytes = ALIGN(ecc->bytes, 2); > + > + if (nfc->nfc_version == 6) { > + switch (ecc->strength) { > + case 60: > + reg = 0x00040010; > + break; > + case 40: > + reg = 0x00040000; > + break; > + case 24: > + reg = 0x00000010; > + break; > + case 16: > + reg = 0x00000000; > + break; > + default: > + return -EINVAL; > + } > + nfc_writel(nfc, reg, NFC_BCHCTL_V6); > + } else { > + switch (ecc->strength) { > + case 70: > + reg = 0x00000001; > + break; > + case 60: > + reg = 0x06000001; > + break; > + case 40: > + reg = 0x04000001; > + break; > + case 16: > + reg = 0x02000001; > + break; > + default: > + return -EINVAL; > + } > + nfc_writel(nfc, reg, NFC_BCHCTL_V9); > + } > + > + return 0; > +} > + > +static int rk_nfc_ecc_init(struct device *dev, struct mtd_info *mtd) > +{ > + struct nand_chip *nand = mtd_to_nand(mtd); > + struct rk_nfc *nfc = nand_get_controller_data(nand); > + struct nand_ecc_ctrl *ecc = &nand->ecc; > + static u8 strengths_v9[4] = {70, 60, 40, 16}; > + static u8 strengths_v6[4] = {60, 40, 24, 16}; > + u8 *strengths; > + u32 max_strength; > + int i; > + > + /* support only ecc hw mode */ > + if (ecc->mode != NAND_ECC_HW) { > + dev_err(dev, "ecc.mode not supported\n"); > + return -EINVAL; No, please support the absence of ECC (might be useful for development/testing in some conditions) and having sofware ECC support. > + } > + > + /* if optional dt settings not present */ > + if (!ecc->size || !ecc->strength || > + ecc->strength > nfc->max_ecc_strength) { > + /* use datasheet requirements */ > + ecc->strength = nand->base.eccreq.strength; > + ecc->size = nand->base.eccreq.step_size; > + > + /* > + * align eccstrength and eccsize > + * this controller only supports 512 and 1024 sizes > + */ > + if (nand->ecc.size < 1024) { > + if (mtd->writesize > 512) { > + nand->ecc.size = 1024; > + nand->ecc.strength <<= 1; > + } else { > + dev_err(dev, "ecc.size not supported\n"); > + return -EINVAL; > + } > + } else { > + nand->ecc.size = 1024; > + } > + > + ecc->steps = mtd->writesize / ecc->size; > + max_strength = ((mtd->oobsize / ecc->steps) - 4) * 8 / 14; > + if (max_strength > nfc->max_ecc_strength) > + max_strength = nfc->max_ecc_strength; > + > + strengths = strengths_v9; > + if (nfc->nfc_version == 6) > + strengths = strengths_v6; > + > + 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]; > + } > + rk_nfc_hw_ecc_setup(mtd, ecc, ecc->strength); > + dev_info(dev, "eccsize %d eccstrength %d\n", > + nand->ecc.size, nand->ecc.strength); space > + 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, "16bits buswidth not supported"); > + return -EINVAL; > + } > + > + 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; ^ Avoid these extra spaces in all your return statements. Please run scripts/checkpatch.pl --strict > + > + nfc->page_buf = nfc->buffer; > + len = ecc->steps * 128; ^ A definition of this would be great too > + nfc->oob_buf = devm_kzalloc(dev, len, GFP_KERNEL | GFP_DMA); > + if (!nfc->oob_buf) { > + devm_kfree(dev, nfc->buffer); > + nfc->buffer = NULL; > + nfc->oob_buf = NULL; > + return -ENOMEM; > + } > + > + return 0; > +} > + > +static const struct nand_controller_ops rk_nfc_controller_ops = { > + .attach_chip = rk_nfc_attach_chip, > + .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 *chip; > + struct nand_chip *nand; > + 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 > RK_NAND_MAX_NSELS) { > + dev_err(dev, "invalid reg property size %d\n", nsels); > + return -EINVAL; > + } > + > + chip = devm_kzalloc(dev, sizeof(*chip) + nsels * sizeof(u8), > + GFP_KERNEL); > + if (!chip) > + return -ENOMEM; > + > + chip->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 >= RK_NAND_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; > + } > + > + chip->sels[i] = tmp; > + } > + > + nand = &chip->nand; > + nand->controller = &nfc->controller; > + > + nand_set_flash_node(nand, np); > + nand_set_controller_data(nand, nfc); > + > + nand->options |= NAND_USE_BOUNCE_BUFFER | NAND_NO_SUBPAGE_WRITE; > + nand->bbt_options = NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB; > + nand->legacy.dev_ready = rk_nfc_dev_ready; > + nand->legacy.select_chip = rk_nfc_select_chip; > + nand->legacy.write_byte = rk_nfc_write_byte; > + nand->legacy.write_buf = rk_nfc_write_buf; > + nand->legacy.read_byte = rk_nfc_read_byte; > + nand->legacy.read_buf = rk_nfc_read_buf; > + nand->legacy.cmd_ctrl = rk_nfc_cmd_ctrl; You should not implement any legacy hooks. > + > + /* set default mode in case dt entry is missing */ > + nand->ecc.mode = NAND_ECC_HW; > + > + nand->ecc.write_page_raw = rk_nfc_write_page_raw; > + nand->ecc.write_page = rk_nfc_write_page_hwecc; > + nand->ecc.write_oob_raw = rk_nfc_write_oob_std; > + nand->ecc.write_oob = rk_nfc_write_oob_std; > + > + nand->ecc.read_page_raw = rk_nfc_read_page_raw; > + nand->ecc.read_page = rk_nfc_read_page_hwecc; > + nand->ecc.read_oob_raw = rk_nfc_read_oob_std; > + nand->ecc.read_oob = rk_nfc_read_oob_std; > + > + mtd = nand_to_mtd(nand); > + mtd->owner = THIS_MODULE; > + mtd->dev.parent = dev; > + mtd->name = THIS_NAME; > + mtd_set_ooblayout(mtd, &rk_nfc_ooblayout_ops); > + rk_nfc_hw_init(nfc); > + ret = nand_scan(nand, nsels); > + if (ret) > + return ret; > + ret = mtd_device_register(mtd, NULL, 0); > + if (ret) { > + dev_err(dev, "mtd parse partition error\n"); > + nand_release(nand); > + return ret; > + } > + > + list_add_tail(&chip->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; > + } > + return ret; > +} > + > +static const struct of_device_id rk_nfc_id_table[] = { > + {.compatible = "rockchip,nfc"}, > + {} > +}; > +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->dev = dev; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + nfc->regs = devm_ioremap_resource(dev, res); > + if (IS_ERR(nfc->regs)) { > + ret = PTR_ERR(nfc->regs); > + goto release_nfc; > + } > + nfc->clk.nfc_clk = devm_clk_get(dev, "clk_nfc"); > + if (IS_ERR(nfc->clk.nfc_clk)) { > + dev_err(dev, "no clk\n"); > + ret = PTR_ERR(nfc->clk.nfc_clk); > + goto release_nfc; > + } > + nfc->clk.ahb_clk = devm_clk_get(dev, "clk_ahb"); > + if (IS_ERR(nfc->clk.ahb_clk)) { > + dev_err(dev, "no pad clk\n"); > + ret = PTR_ERR(nfc->clk.ahb_clk); > + goto release_nfc; > + } > + if (of_property_read_u32(dev->of_node, "clock-rates", > + &nfc->clk.nfc_rate)) > + nfc->clk.nfc_rate = RK_DEFAULT_CLOCK_RATE; > + clk_set_rate(nfc->clk.nfc_clk, nfc->clk.nfc_rate); > + > + 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; > + } > + > + if (nfc->nfc_version == 6) > + nfc_writel(nfc, 0, NFC_INTEN_V6); > + else > + nfc_writel(nfc, 0, NFC_INTEN_V9); > + 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 *chip; > + > + while (!list_empty(&nfc->chips)) { > + chip = list_first_entry(&nfc->chips, struct rk_nfc_nand_chip, > + node); > + nand_release(&chip->nand); > + list_del(&chip->node); > + } > + > + rk_nfc_disable_clk(&nfc->clk); > + > + return 0; > +} > + > +#ifdef CONFIG_PM_SLEEP > +static int rk_nfc_suspend(struct device *dev) > +{ > + struct rk_nfc *nfc = dev_get_drvdata(dev); > + > + rk_nfc_disable_clk(&nfc->clk); > + > + return 0; > +} > + > +static int rk_nfc_resume(struct device *dev) > +{ > + struct rk_nfc *nfc = dev_get_drvdata(dev); > + struct rk_nfc_nand_chip *chip; > + struct nand_chip *nand; > + int ret; > + u32 i; > + > + udelay(200); > + > + ret = rk_nfc_enable_clk(dev, &nfc->clk); > + if (ret) > + return ret; > + > + /* reset NAND chip if VCC was powered off */ > + list_for_each_entry(chip, &nfc->chips, node) { > + nand = &chip->nand; > + for (i = 0; i < chip->nsels; i++) > + nand_reset(nand, i); > + } > + > + return 0; > +} > + > +static SIMPLE_DEV_PM_OPS(rk_nfc_pm_ops, rk_nfc_suspend, rk_nfc_resume); I think you can define a dev_pm_ops stucture to contain a SET_SYSTEM_SLEEP_PM_OPS(*suspend, *resume) and this way you can entirely get rid of the #ifdef conditionals. > +#endif > + > +static struct platform_driver rk_nfc_driver = { > + .probe = rk_nfc_probe, > + .remove = rk_nfc_remove, > + .driver = { > + .name = THIS_NAME, > + .of_match_table = rk_nfc_id_table, > +#ifdef CONFIG_PM_SLEEP > + .pm = &rk_nfc_pm_ops, > +#endif > + }, > +}; > + > +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"); Thanks, Miquèl