RE: [LINUX PATCH v12 3/3] mtd: rawnand: arasan: Add support for Arasan NAND Flash Controller
From: Naga Sureshkumar Relli
Date: Mon Nov 12 2018 - 07:43:47 EST
Hi Boris & Martin,
> -----Original Message-----
> From: Boris Brezillon [mailto:boris.brezillon@xxxxxxxxxxx]
> Sent: Monday, November 12, 2018 4:28 PM
> To: Martin Lund <martin.lund@xxxxxxxxxxxxxxxxxx>
> Cc: Naga Sureshkumar Relli <nagasure@xxxxxxxxxx>; miquel.raynal@xxxxxxxxxxx;
> richard@xxxxxx; dwmw2@xxxxxxxxxxxxx; computersforpeace@xxxxxxxxx;
> marek.vasut@xxxxxxxxx; Michal Simek <michals@xxxxxxxxxx>; nagasuresh12@xxxxxxxxx;
> linux-mtd@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; robh@xxxxxxxxxx
> Subject: Re: [LINUX PATCH v12 3/3] mtd: rawnand: arasan: Add support for Arasan NAND
> Flash Controller
>
> On Mon, 12 Nov 2018 11:55:36 +0100
> Martin Lund <martin.lund@xxxxxxxxxxxxxxxxxx> wrote:
>
> > Hi Naga,
> >
> > Just a few review comments for the v12 version.
> >
> > On Fri, Nov 9, 2018 at 6:00 AM Naga Sureshkumar Relli
> > <naga.sureshkumar.relli@xxxxxxxxxx> wrote:
> > > +#define PKT_OFST 0x00
> > > +#define PKT_CNT_SHIFT 12
> > > +
> > > +#define MEM_ADDR1_OFST 0x04
> > > +#define MEM_ADDR2_OFST 0x08
> >
> > For the sake of readability I think *_OFFSET is preferred, especially
> > since the driver already includes short macro names. I think this is
> > similar to the EVNT vs EVENT point.
> > The majority of drivers in the Linux kernel do not shorten OFFSET -> OFST.
> >
> >
> > > +static void anfc_rw_pio_op(struct mtd_info *mtd, u8 *buf, int len,
> > > + bool do_read, int prog, int pktcount, int
> > > +pktsize) {
> > > + struct nand_chip *chip = mtd_to_nand(mtd);
> > > + struct anfc_nand_controller *nfc = to_anfc(chip->controller);
> > > + struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > > + u32 *bufptr = (u32 *)buf;
> > > + u32 cnt = 0, intr = 0;
> > > +
> > > + anfc_config_dma(nfc, 0);
> > > +
> > > + if (pktsize == 0)
> > > + pktsize = len;
> > > +
> > > + anfc_setpktszcnt(nfc, pktsize, pktcount);
> > > +
> > > + if (!achip->strength)
> > > + intr = MBIT_ERROR;
> > > +
> > > + if (do_read)
> > > + intr |= READ_READY;
> > > + else
> > > + intr |= WRITE_READY;
> > > + anfc_enable_intrs(nfc, intr);
> > > + writel(prog, nfc->base + PROG_OFST);
> > > + while (cnt < pktcount) {
> > > + anfc_wait_for_event(nfc);
> > > + cnt++;
> > > + if (cnt == pktcount)
> > > + anfc_enable_intrs(nfc, XFER_COMPLETE);
> > > + if (do_read)
> > > + ioread32_rep(nfc->base + DATA_PORT_OFST, bufptr,
> > > + pktsize / 4);
> > > + else
> > > + iowrite32_rep(nfc->base + DATA_PORT_OFST, bufptr,
> > > + pktsize / 4);
> > > + bufptr += (pktsize / 4);
> > > + if (cnt < pktcount)
> > > + anfc_enable_intrs(nfc, intr);
> > > + }
> > > + anfc_wait_for_event(nfc);
> > > +}
> >
> > Throughout the driver all calls to anfc_wait_for_event() ignores the
> > timeout return value. It would be nice to see some error handling in
> > case it times out - at minimum consider printing out an error message
> > since timeout on NAND operations are fairly critical and should
> > generally not occur. Perhaps even an argument can be made for
> > returning -ETIMEDOUT in case of timeout.
>
> Yes please, check anfc_wait_for_event() return code and propagate the error to the upper layer.
Ok, I will update
Thanks,
Naga Sureshkumar Relli