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

[Boris Brezillon <boris.brezillon@free-electrons.com>]

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;
> +};