Re: [patch/RESEND 2.6.29-rc3-git] NAND: davinci_nand driver

Andrew Morton
2009-02-26T20:33:49Z

On Tue, 24 Feb 2009 15:09:54 -0800
David Brownell <david-b@pacbell.net> wrote:

> From: David Brownell <dbrownell@users.sourceforge.net>
>
> This is a device driver for the NAND flash controller found on the
> various DaVinci family chips. It handles up to four SoC chipselects,
> and some flavors of secondary chipselect (e.g. based on upper bits
> of the address bus) as used with some multichip packages. (Including
> the 2 GByte chips used on some TI devel boards.)
>
> The 1-bit ECC hardware is supported (3 bytes ECC per 512 bytes data);
> but not yet the newer 4-bit ECC (10 bytes ECC per 512 bytes data), as
> available on chips like the DM355 or OMAP-L137 and needed with the
> more error-prone MLC NAND chips.
>
> This is a cleaned-up version of code that's been in use for several
> years now; sanity checked with the new drivers/mtd/tests.
>
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
>
> ...
>
> + * Copyright (C) 2006 Texas Instruments.
> + *
> + * Ported to 2.6.23 Copyright (C) 2008 by
> + * Sander Huijsen <Shuijsen@optelecom-nkf.com>
> + * Troy Kisky <troy.kisky@boundarydevices.com>
> + * Dirk Behme <Dirk.Behme@gmail.com>

hm. What's the story with authorship, attributions and signoffs here?

>
> ...
>
> +#ifdef CONFIG_MTD_PARTITIONS
> +static inline int mtd_has_partitions(void) { return 1; }
> +#else
> +static inline int mtd_has_partitions(void) { return 0; }
> +#endif
> +
> +#ifdef CONFIG_MTD_CMDLINE_PARTS
> +static inline int mtd_has_cmdlinepart(void) { return 1; }
> +#else
> +static inline int mtd_has_cmdlinepart(void) { return 0; }
> +#endif

These definitions shouldn't be buried in a .c file.

>
> ...
>
> +static void nand_davinci_hwctl_1bit(struct mtd_info *mtd, int mode)
> +{
> + struct davinci_nand_info *info;
> + u32 retval;

The identifier `retval' is usually used to identify the value which
this function will return.

> + unsigned long flags;
> +
> + info = to_davinci_nand(mtd);
> +
> + /* Reset ECC hardware */
> + nand_davinci_readecc_1bit(mtd);
> +
> + spin_lock_irqsave(&davinci_nand_lock, flags);
> +
> + /* Restart ECC hardware */
> + retval = davinci_nand_readl(info, NANDFCR_OFFSET);
> + retval |= BIT(8 + info->core_chipsel);
> + davinci_nand_writel(info, NANDFCR_OFFSET, retval);
> +
> + spin_unlock_irqrestore(&davinci_nand_lock, flags);
> +}
> +
> +/*
> + * Read hardware ECC value and pack into three bytes
> + */
> +static int nand_davinci_calculate_1bit(struct mtd_info *mtd,
> + const u_char *dat, u_char *ecc_code)
> +{
> + unsigned int ecc_val = nand_davinci_readecc_1bit(mtd);
> + unsigned int tmp = (ecc_val & 0x0fff) | ((ecc_val & 0x0fff0000) >> 4);

argh.

> + /* invert so that erased block ecc is correct */
> + tmp = ~tmp;
> + ecc_code[0] = (u_char)(tmp);
> + ecc_code[1] = (u_char)(tmp >> 8);
> + ecc_code[2] = (u_char)(tmp >> 16);

Is there some library facility which is being open-coded here?

> + return 0;
> +}
> +
>
> ...
>
> +static int __init nand_davinci_probe(struct platform_device *pdev)
> +{
> + struct davinci_nand_pdata *pdata = pdev->dev.platform_data;
> + struct davinci_nand_info *info;
> + struct resource *res1;
> + struct resource *res2;
> + void __iomem *vaddr;
> + void __iomem *base;
> + int ret;
> + u32 val;
> + nand_ecc_modes_t ecc_mode;
> +
> + /* which external chipselect will we be managing? */
> + if (pdev->id < 0 || pdev->id > 3)
> + return -ENODEV;
> +
> + info = kzalloc(sizeof(*info), GFP_KERNEL);
> + if (!info) {
> + dev_err(&pdev->dev, "unable to allocate memory\n");
> + ret = -ENOMEM;
> + goto err_nomem;
> + }
> +
> + platform_set_drvdata(pdev, info);
> +
> + res1 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + res2 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + if (!res1 || !res2) {
> + dev_err(&pdev->dev, "resource missing\n");
> + ret = -EINVAL;
> + goto err_res;

This leaks `info'.

Please check all the error path unwinding here.

> + }
> +
>
> ...
>