From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754188AbbKLKcY (ORCPT ); Thu, 12 Nov 2015 05:32:24 -0500 Received: from mga02.intel.com ([134.134.136.20]:19182 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754035AbbKLKcS (ORCPT ); Thu, 12 Nov 2015 05:32:18 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,281,1444719600"; d="scan'208";a="835398836" Message-ID: <1447324339.31665.120.camel@linux.intel.com> Subject: Re: [PATCH v4 2/3] mtd: nand: Add support for Arasan Nand Flash Controller From: Andy Shevchenko To: punnaiah choudary kalluri Cc: Punnaiah Choudary Kalluri , David Woodhouse , Brian Norris , geert@linux-m68k.org, "michals@xilinx.com" , wangzhou1@hisilicon.com, Florian Fainelli , gsi@denx.de, haokexin@gmail.com, rogerq@ti.com, "linux-kernel@vger.kernel.org" , "linux-mtd@lists.infradead.org" , Punnaiah Choudary Date: Thu, 12 Nov 2015 12:32:19 +0200 In-Reply-To: References: <1446691741-8393-1-git-send-email-punnaia@xilinx.com> <1447077023.31665.72.camel@linux.intel.com> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.1-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2015-11-12 at 10:18 +0530, punnaiah choudary kalluri wrote: > On Mon, Nov 9, 2015 at 7:20 PM, Andy Shevchenko > 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 > > > + > > > +#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 Intel Finland Oy