Re: [PATCH v5 1/6] nand: spi: add basic blocks for infrastructure

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

On Mon, 10 Apr 2017 15:51:48 +0800
Peter Pan <peterpandong@micron.com> wrote:

> +
> +struct spinand_ecc_engine_ops {
> +	void (*get_ecc_status)(struct spinand_device *chip,
> +			       unsigned int status, unsigned int *corrected,
> +			       unsigned int *ecc_errors);

No need to specify ecc, we're interacting with an ECC engine, so I
guess it's already assumed.

	void (*get_stats)(struct spinand_device *chip,
			  unsigned int status,
			  unsigned int *corrected,
			  unsigned int *errors);

BTW, we probably need max_bitflips, unless corrected already contains
this information, in which case it should probably be clarified (better
name + kernel doc).

> +	void (*disable_ecc)(struct spinand_device *chip);
> +	void (*enable_ecc)(struct spinand_device *chip);

Ditto, just enable()/disable().

> +};
> +
> +enum spinand_ecc_type {
> +	SPINAND_ECC_ONDIE,
> +	SPINAND_ECC_HW,
> +};

Probably something we should extract from rawnand.h and move to nand.h
so that we can share the same definition (until we also share the ECC
engine interface).

> +
> +struct spinand_ecc_engine {
> +	u32 strength;
> +	u32 steps;
> +	const struct spinand_ecc_engine_ops *ops;
> +};

The same ECC engine engine can possibly be used with different NAND
devices. So what you're describing here is more an ECC engine user than
the ECC engine itself.

Ideally we should have:

struct nand_ecc_engine {
	/* Some info describing engine caps. */
	const struct spinand_ecc_engine_ops *ops;
};

struct nand_ecc_engine_user {
	u32 strength;
	u32 steps;
	struct nand_ecc_engine *engine;
};

Anyway, I think we can keep that for later.

BTW, would you mind keeping all that is ECC related outside of this
initial series, or at least, add it in separate patches?