Re: [PATCH 2/3] mtd: mediatek: driver for MTK Smart Device Gen1 NAND

[Brian Norris <computersforpeace@gmail.com>]

On Tue, Mar 08, 2016 at 05:24:37PM +0100, Boris Brezillon wrote:
> On Wed,  2 Mar 2016 12:00:12 -0500
> Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> wrote:
> > +static void mtk_nfc_hw_reset(struct mtk_nfc_host *host)
> > +{
> > +	unsigned long timeout = MTK_RESET_TIMEOUT;
> > +	struct device *dev = host->dev;
> > +	u32 val;
> > +
> > +	/* reset the state machine, data fifo and fdm data */
> > +	mtk_nfi_writel(host, CON_FIFO_FLUSH | CON_NFI_RST, MTKSDG1_NFI_CON);
> > +	timeout += jiffies;
> > +	do {
> > +		val = mtk_nfi_readl(host, MTKSDG1_NFI_MASTER_STA);
> > +		val &= MASTER_STA_MASK;
> > +		if (!val)
> > +			return;
> > +		usleep_range(50, 100);
> > +
> > +	} while (time_before(jiffies, timeout));
> 
> You may want to use readl_relaxed_poll_timeout() (even though there's
> no way to specify a range).
> This comment applies to all the places where you're implementing this
> kind of loop.

What's more, this timeout loop (and probably many of the others) is
wrong. You need to do one last status check before declaring a timeout,
since the device may become ready while you're sleeping. It's the same
problem as we've resolved here:

http://git.infradead.org/l2-mtd.git/commitdiff/9ebfdf5b18493f338237ef9861a555c2f79b0c17
Subject: "mtd: nand: check status before reporting timeout"

readl_relaxed_poll_timeout() gets this right, of course.

> > +
> > +	dev_warn(dev, "nfi master active after in reset [0x%x] = 0x%x\n",
> > +		MTKSDG1_NFI_MASTER_STA, val);
> > +};

While we're at it: you have a stray semicolon after your function
definition.

Brian