Re: [RESEND PATCH 2/5] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver

[Boris Brezillon]

On Thu, 24 May 2018 10:46:27 +0200
Stefan Agner <stefan@xxxxxxxx> wrote:

> Hi Boris,
> 
> Thanks for the initial review! One small question below:
> 
> On 23.05.2018 16:18, Boris Brezillon wrote:
> > Hi Stefan,
> > 
> > On Tue, 22 May 2018 14:07:06 +0200
> > Stefan Agner <stefan@xxxxxxxx> wrote:  
> >> +
> >> +struct tegra_nand {
> >> +	void __iomem *regs;
> >> +	struct clk *clk;
> >> +	struct gpio_desc *wp_gpio;
> >> +
> >> +	struct nand_chip chip;
> >> +	struct device *dev;
> >> +
> >> +	struct completion command_complete;
> >> +	struct completion dma_complete;
> >> +	bool last_read_error;
> >> +
> >> +	dma_addr_t data_dma;
> >> +	void *data_buf;
> >> +	dma_addr_t oob_dma;
> >> +	void *oob_buf;
> >> +
> >> +	int cur_chip;
> >> +};  
> > 
> > This struct should be split in 2 structures: one representing the NAND
> > controller and one representing the NAND chip:
> > 
> > struct tegra_nand_controller {
> > 	struct nand_hw_control base;
> > 	void __iomem *regs;
> > 	struct clk *clk;
> > 	struct device *dev;
> > 	struct completion command_complete;
> > 	struct completion dma_complete;
> > 	bool last_read_error;
> > 	int cur_chip;
> > };
> > 
> > struct tegra_nand {
> > 	struct nand_chip base;
> > 	dma_addr_t data_dma;
> > 	void *data_buf;
> > 	dma_addr_t oob_dma;
> > 	void *oob_buf;
> > };  
> 
> Is there a particular reason why you would leave DMA buffers in the chip
> structure? It seems that is more a controller thing...

The size of those buffers is likely to be device dependent, so if you
have several NANDs connected to the controller, you'll either have to
have one buffer at the controller level which is max(all-chip-buf-size)
or a buffer per device.

Also, do you really need these buffers? The core already provide some
which are suitable for DMA (chip->oob_poi and chip->data_buf).

> 
> If I move them, then struct tegra_nand would be basically empty. Can I
> just use struct nand_chip and have no driver specific chip abstraction?

Sure.