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

Brian Norris
2016-03-08T18:17:40Z

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