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

[punnaiah choudary kalluri]

On Thu, Nov 12, 2015 at 4:02 PM, Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> 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@xxxxxxxxxxxxxxx> 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.
>

Ok. Thanks. I will rename this file.

>> >
>> > 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.
>

It can be done. I will modify accordingly


Thanks,
Punnaiah

> 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@xxxxxxxxxxxxxxx>
> Intel Finland Oy
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/