Re: [PATCH] goldfish: NAND flash driver

[Alan Cox <alan@linux.intel.com>]

> > +	writel(cmdp, base + NAND_COMMAND);
> What guarantee do you have on the order of writes here ? Isn't a
> write barrier required here ?

Its a virtual platform powered by an emulator - so no barriers needed
that I can see.

> > +	spin_lock_irqsave(&nand->lock, irq_flags);
> Why this spin_lock and not a mutex ? I didn't see any interrupts used
> in this driver, have I missed something ?

The driver doesn't require it not sure about all the callers.
> 
> > +	if (goldfish_nand_cmd_with_params(mtd, cmd, addr, len,
> > ptr, &rv)) {
> > +		writel(mtd - nand->mtd, base + NAND_DEV);
> > +		writel((u32)(addr >> 32), base + NAND_ADDR_HIGH);
> > +		writel((u32)addr, base + NAND_ADDR_LOW);
> > +		writel(len, base + NAND_TRANSFER_SIZE);
> > +		writel((u32)ptr, base + NAND_DATA);
> > +		writel(cmd, base + NAND_COMMAND);
> > +		rv = readl(base + NAND_RESULT);
> Same question here on the order of the read wrt to previous writes.

reads wont pass write anyway as its a sane platform.

> > +	if (ofs + len > mtd->size)
> > +		goto invalid_arg;
> I don't think that test is required, the MTD API gives already that
> guarantee AFAIR.

Ok

> > +	nand->cmd_params = devm_kzalloc(&pdev->dev,
> > +					sizeof(struct cmd_params),
> > GFP_KERNEL);
> > +	if (!nand->cmd_params)
> > +		return -1;
> > +
> > +	paddr = __pa(nand->cmd_params);
> That looks weird (the __pa()) usage. I thought drivers should not use
> __pa() directly.

Will look at using dma_alloc_coherent for it.

> > +	spin_lock_irqsave(&nand->lock, irq_flags);
> Again same spin_lock question.

I'm very wary of changing this but will take a look. It's actually not
that important because its not real flash so it has unusually excellent
performance via the emulator.

Alan