Re: [PATCH v4 2/3] mtd: nand: Add support for Arasan Nand Flash Controller

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, 2015-11-12 at 10:18 +0530, punnaiah choudary kalluri wrote:
> On Mon, Nov 9, 2015 at 7:20 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, 2015-11-05 at 08:19 +0530, Punnaiah Choudary Kalluri wrote:
> > > Added the basic driver for Arasan Nand Flash Controller used in
> > > Zynq UltraScale+ MPSoC. It supports only Hw Ecc and upto 24bit
> > > correction.
> > > 
> > 
> > > +config MTD_NAND_ARASAN
> > > +     tristate "Support for Arasan Nand Flash controller"
> > > +     depends on MTD_NAND
> > 
> > This looks useless since you can't see the item without MTD_NAND is
> > chosen.
> > 
> > > +     help
> > > +       Enables the driver for the Arasan Nand Flash controller
> > > on
> > > +       Zynq UltraScale+ MPSoC.
> > > +
> > >  endif # MTD_NAND
> > > 
> > 
> > +obj-$(CONFIG_MTD_NAND_ARASAN)          += arasan_nfc.o
> > 
> > "nfc" part a bit ambiguous since NFC might be Near Field
> > Communication.
> 
> This driver is under mtd/nand so, there is no point of confusion and
> in this context nfc is nand flash controller.

Imagine that at some point arasan (whatever) releases NFC chip, and
someone puts the driver under corresponding folder but with the same
file name (and driver name). Do you see a problem? I see two:
- if you built-in both how you supply command line parameters?
- some platform code may do request_module() or
platform_driver_register() with the name you provided as DRIVER_NAME.

So, I just suggest distinguishable name. But it's a call of NAND
subsystem maintainer.

> > 
> > Perhaps "nand_fc" or something like that?
> > 

> > > +#include <linux/platform_device.h>
> > > +
> > > +#define DRIVER_NAME                  "arasan_nfc"
> > 
> > Ditto.
> > 
> > > +static int anfc_device_ready(struct mtd_info *mtd,
> > > +                          struct nand_chip *chip)
> > > +{
> > > +     u8 status;
> > > +     unsigned long timeout = jiffies + STATUS_TIMEOUT;
> > > +
> > > +     do {
> > > +             chip->cmdfunc(mtd, NAND_CMD_STATUS, 0, 0);
> > > +             status = chip->read_byte(mtd);
> > > +             if (status & ONFI_STATUS_READY) {
> > 
> > > +                     if (status & ONFI_STATUS_FAIL)
> > > +                             return NAND_STATUS_FAIL;
> > 
> > This is invariant to the loop, perhaps move outside.
> 
> Nand device is ready means it is ready to accept next command and
> it is done with previous command. It doesn't mean that previous
> command is success, it can fail also.

This is style and logic comment. I do not object how NAND works.

> > 
> > > +                     break;
> > > +             }
> > > +             cpu_relax();
> > > +     } while (!time_after_eq(jiffies, timeout));

Just move it here. It will be the same, but unload busy loop.

Did I miss something?

> > > +
> > > +     if (time_after_eq(jiffies, timeout)) {
> > > +             pr_err("%s timed out\n", __func__);
> > 
> > dev_err?
> > 

> > > +static void anfc_read_buf(struct mtd_info *mtd, uint8_t *buf,
> > > int
> > > len)
> > > +{
> > > +     u32 i, pktcount, buf_rd_cnt = 0, pktsize;
> > 
> > Type for i looks unsigned int, why u32? Same question for all
> > variables
> > that are not used to directly program HW.
> > 

u32 and other derivatives mostly common when you program HW. Here for
simple stuff better to use plain types, therefore unsigned int.

> > > int len)
> > > +{
> > > +     u32 buf_wr_cnt = 0, pktcount = 1, i, pktsize;
> > 
> > Useless assignment of pktcount. Check all your definition blocks
> > for
> > similar thing.
> 
> what is the problem with u32 here ? may be i am missing something
> here but
> i really want to know the reason.

Ditto.

> > > +             writel(lower_32_bits(paddr), nfc->base +
> > > DMA_ADDR0_OFST);
> > > +             writel(upper_32_bits(paddr), nfc->base +
> > > DMA_ADDR1_OFST);
> > 
> > lo_hi_writeq();
> 
> Ok. let me check if this function is available across all
> the platforms.

The same spread as for writel().
If your HW allows you to do 64-bit writes on 64-bit platforms, just
use writeq(), but still include io-64-nonatomic-lo-hi.h (or how it's
called nowadays).

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy