[PATCH 06/12] mpc5121: Added NAND Flash Controller driver.
Grant Likely
grant.likely at secretlab.ca
Thu May 7 06:59:30 EST 2009
On Wed, May 6, 2009 at 2:15 PM, Wolfgang Denk <wd at denx.de> wrote:
> From: Piotr Ziecik <kosmo at semihalf.com>
>
> This patch adds NAND Flash Controller driver for MPC5121
> revision 2. All device features, except hardware ECC and
> power management, are supported.
>
> Signed-off-by: Piotr Ziecik <kosmo at semihalf.com>
> Signed-off-by: Wolfgang Denk <wd at denx.de>
> Cc: <linux-mtd at lists.infradead.org>
> Cc: Grant Likely <grant.likely at secretlab.ca>
> Cc: John Rigby <jcrigby at gmail.com>
> ---
> arch/powerpc/include/asm/mpc5121_nfc.h | 100 +++
> arch/powerpc/platforms/512x/mpc512x_shared.c | 1 +
> drivers/mtd/nand/Kconfig | 7 +
> drivers/mtd/nand/Makefile | 1 +
> drivers/mtd/nand/mpc5121_nfc.c | 855 ++++++++++++++++++++++++++
> 5 files changed, 964 insertions(+), 0 deletions(-)
> create mode 100644 arch/powerpc/include/asm/mpc5121_nfc.h
> create mode 100644 drivers/mtd/nand/mpc5121_nfc.c
>
> diff --git a/arch/powerpc/include/asm/mpc5121_nfc.h b/arch/powerpc/include/asm/mpc5121_nfc.h
> new file mode 100644
> index 0000000..b96a5b9
> --- /dev/null
> +++ b/arch/powerpc/include/asm/mpc5121_nfc.h
[... bunch of #defines trimmed]
There is only one user of this. It should be moved into the driver .c
file. Otherwise okay.
> diff --git a/arch/powerpc/platforms/512x/mpc512x_shared.c b/arch/powerpc/platforms/512x/mpc512x_shared.c
> index d8cd579..7135d89 100644
> --- a/arch/powerpc/platforms/512x/mpc512x_shared.c
> +++ b/arch/powerpc/platforms/512x/mpc512x_shared.c
> @@ -71,6 +71,7 @@ void __init mpc512x_init_IRQ(void)
> static struct of_device_id __initdata of_bus_ids[] = {
> { .compatible = "fsl,mpc5121-immr", },
> { .compatible = "fsl,mpc5121-localbus", },
> + { .compatible = "fsl,mpc5121-nfc", },
This doesn't look right. Shouldn't the NAND controller be hanging of
the IMMR node?
> {},
> };
>
> diff --git a/drivers/mtd/nand/mpc5121_nfc.c b/drivers/mtd/nand/mpc5121_nfc.c
> new file mode 100644
> index 0000000..a8da4db
> --- /dev/null
> +++ b/drivers/mtd/nand/mpc5121_nfc.c
> @@ -0,0 +1,855 @@
> +/*
> + * Copyright 2004-2008 Freescale Semiconductor, Inc.
> + * Copyright 2009 Semihalf.
> + *
> + * Based on original driver from Freescale Semiconductor
> + * written by John Rigby <jrigby at freescale.com> on basis
> + * of drivers/mtd/nand/mxc_nand.c. Reworked and extended
> + * Piotr Ziecik <kosmo at semihalf.com>.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
> + * MA 02110-1301, USA.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/nand.h>
> +#include <linux/mtd/partitions.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +
> +#include <asm/mpc512x.h>
> +#include <asm/mpc5121_nfc.h>
> +
> +#define DRV_NAME "mpc5121_nfc"
> +#define DRV_VERSION "0.5"
Is this really necessary (especially the DRV_VERSION thing)?
> +/* Wait for operation complete */
> +static void mpc5121_nfc_done(struct mtd_info *mtd)
> +{
> + struct nand_chip *chip = mtd->priv;
> + struct mpc5121_nfc_prv *prv = chip->priv;
> + int rv;
> +
> + if ((nfc_read(mtd, NFC_CONFIG2) & NFC_INT) == 0) {
> + nfc_clear(mtd, NFC_CONFIG1, NFC_INT_MASK);
> + rv = wait_event_timeout(prv->irq_waitq,
> + (nfc_read(mtd, NFC_CONFIG2) & NFC_INT), NFC_TIMEOUT);
> +
> + if (!rv)
> + printk(KERN_WARNING DRV_NAME
Throughout this driver printk() calls should really be dev_*() calls.
> +/*
> + * Read NFC configuration from Reset Config Word
> + *
> + * NFC is configured during reset in basis of information stored
> + * in Reset Config Word. There is no other way to set NAND block
> + * size, spare size and bus width.
> + */
> +static int mpc5121_nfc_read_hw_config(struct mtd_info *mtd)
> +{
> + struct nand_chip *chip = mtd->priv;
> + struct mpc512x_reset_module *rm;
> + struct device_node *rmnode;
> + uint rcw_pagesize = 0;
> + uint rcw_sparesize = 0;
> + uint rcw_width;
> + uint rcwh;
> + uint romloc, ps;
> +
> + rmnode = of_find_compatible_node(NULL, NULL, "fsl,mpc5121-reset");
> + if (!rmnode) {
> + printk(KERN_ERR DRV_NAME ": Missing 'fsl,mpc5121-reset' "
> + "node in device tree!\n");
> + return -ENODEV;
> + }
A little ugly, but I won't gripe too much about it. I'd rather see a
platform code export some kind of api for manipulating the reset
module (I assume that multiple drivers will need to fiddle with it),
but I'm willing to be pragmatic here in the interest of getting it
merged. It can always be reworked later with little risk.
> +static struct of_platform_driver mpc5121_nfc_driver = {
> + .owner = THIS_MODULE,
> + .name = DRV_NAME,
> + .match_table = mpc5121_nfc_match,
> + .probe = mpc5121_nfc_probe,
> + .remove = __exit_p(mpc5121_nfc_remove),
> + .suspend = NULL,
> + .resume = NULL,
2 unnecessary lines. gcc initializes unset fields to 0.
> + .driver = {
> + .name = DRV_NAME,
> + .owner = THIS_MODULE,
> + },
Duplicate settings. Don't need to set both .name/.owner and
.driver.name and .driver.owner.
> +};
> +
> +static int __init mpc5121_nfc_init(void)
> +{
> + if (of_register_platform_driver(&mpc5121_nfc_driver) != 0) {
> + printk(KERN_ERR DRV_NAME ": Driver register failed!\n");
> + return -ENODEV;
> + }
> + return 0;
Heh. "return of_register_platform_driver(&mpc5121_nfc_driver);" is
sufficient and accepted here.
> +}
> +
> +static void __exit mpc5121_nfc_cleanup(void)
> +{
> + of_unregister_platform_driver(&mpc5121_nfc_driver);
> +}
> +
> +module_init(mpc5121_nfc_init);
Please move this line to just below the mpc5121_nfc_init function.
> +module_exit(mpc5121_nfc_cleanup);
> +
> +MODULE_AUTHOR("Freescale Semiconductor, Inc.");
> +MODULE_DESCRIPTION("MPC5121 NAND MTD driver");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION(DRV_VERSION);
As I alluded to above, I really don't like the MODULE_VERSION think in
mainline code.
I only skimmed the driver, but that being said, other than my comments
it looks pretty good to me.
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
More information about the Linuxppc-dev
mailing list