Re: [PATCH v3 3/8] nand: spi: add basic blocks for infrastructure

Boris Brezillon
2017-03-16T09:55:56Z

On Thu, 16 Mar 2017 14:47:32 +0800
Peter Pan <peterpandong@micron.com> wrote:

> +
> +#define SPINAND_CAP_RD_X1 BIT(0)
> +#define SPINAND_CAP_RD_X2 BIT(1)
> +#define SPINAND_CAP_RD_X4 BIT(2)
> +#define SPINAND_CAP_RD_DUAL BIT(3)
> +#define SPINAND_CAP_RD_QUAD BIT(4)
> +#define SPINAND_CAP_WR_X1 BIT(5)
> +#define SPINAND_CAP_WR_X2 BIT(6)
> +#define SPINAND_CAP_WR_X4 BIT(7)
> +#define SPINAND_CAP_WR_DUAL BIT(8)
> +#define SPINAND_CAP_WR_QUAD BIT(9)
> +#define SPINAND_CAP_HW_ECC BIT(10)

Empty line please.

> +struct spinand_controller {
> + struct spinand_controller_ops *ops;
> + u32 caps;
> + void *priv;

Nope, the ->priv field is a per-device private data, so it should be
placed in struct spinand_device (see below), otherwise, if you have the
same controller connected to 2 different chips, each time you call
spinand_set_controller_data() you will overwrite the ->priv value.

Each spinand_controller should then inherit from struct
spinand_controller:

struct my_spinand_controller {
struct spinand_controller base;

/* put your SPI NAND controller specific fields here. */
};

> +};
> +
> +/**
> + * struct spinand_device - SPI-NAND Private Flash Chip Data
> + * @base: NAND device instance
> + * @lock: protection lock
> + * @name: name of the chip
> + * @id: ID structure
> + * @read_cache_op: Opcode of read from cache
> + * @write_cache_op: Opcode of program load
> + * @buf: buffer for read/write data
> + * @oobbuf: buffer for read/write oob
> + * @rw_mode: read/write mode of SPI NAND chip
> + * @controller: SPI NAND controller instance
> + * @manufacturer: SPI NAND manufacturer instance, describe
> + * manufacturer related objects
> + * @ecc_engine: SPI NAND ECC engine instance
> + */
> +struct spinand_device {
> + struct nand_device base;
> + struct mutex lock;
> + char *name;
> + struct spinand_id id;
> + u8 read_cache_op;
> + u8 write_cache_op;
> + u8 *buf;
> + u8 *oobbuf;
> + u32 rw_mode;
> + struct spinand_controller *controller;

struct {
struct spinand_controller *controller;
void *priv;
} controller;

> + struct {
> + const struct spinand_manufacturer *manu;
> + void *priv;
> + } manufacturer;
> + struct {
> + struct spinand_ecc_engine *engine;
> + void *context;
> + } ecc;
> +};