diff mbox

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

Message ID 200902261515.22197.david-b@pacbell.net
State New, archived
Headers show

Commit Message

David Brownell Feb. 26, 2009, 11:15 p.m. UTC 
On Thursday 26 February 2009, Andrew Morton wrote:

> > + * 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?

Written by TI (PSP team in India, ISTR) with no individual
authorship credited, and shipped with a MontaVista 2.6.10
kernel.  Ported as noted; I could presumably add my own
copyright given recent updates I've made.  Likewise Felipe
Balbi.  Kevin Hilman has signed off on various patches as
part of merging them to the DaVinci tree.

(To the TI team reading this via the DaVinci list:  I think
Andrew is hinting that a Signed-off-By from a TI person
would be a Nice Thing.  Same for Dirk, and maybe others.)


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

I will send along a patch to move them to <linux/mtd/...> headers,
now that there seems to be a bit of recognition that the current
ifdef-centric approach in the MTD mapping drivers is trouble.  ;)


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

True; resolved in the appended fixup patch.


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

It seems the best-dressed pirates have parrots named "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?

I don't know of such a facility:  24-bit integer into 3-byte buffer.


> > +	return 0;
> > +}
> > +
> >
> > ...
> >
> > +static int __init nand_davinci_probe(struct platform_device *pdev)
> > +{
> > +		... deletia ...
> > +
> > +	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.

OK -- that does look buggish.

(Kevin -- I suggest you merge this to the DaVinci tree to
make the eventual resync-with-mainline easier.)

Comments

Felipe Balbi Feb. 27, 2009, 5:52 p.m. UTC | #1 
On Thu, Feb 26, 2009 at 03:15:21PM -0800, David Brownell wrote:
> (To the TI team reading this via the DaVinci list:  I think
> Andrew is hinting that a Signed-off-By from a TI person
> would be a Nice Thing.  Same for Dirk, and maybe others.)

If it's really necessary, feel free to add mine:

Signed-off-by: Felipe Balbi <me@felipebalbi.com>
Rajashekhara, Sudhakar March 2, 2009, 8:33 a.m. UTC | #2 
I and one of my colleague Vinod, who is no more with TI, worked on this driver. I can sign-off this driver.

Signed-off-by: Sudhakar Rajashekhara <sudhakar.raj@ti.com>

Regards, Sudhakar
> 
> On Thu, Feb 26, 2009 at 03:15:21PM -0800, David Brownell wrote:
> > (To the TI team reading this via the DaVinci list:  I think Andrew 
> > is hinting that a Signed-off-By from a TI person would be a Nice 
> > Thing.  Same for Dirk, and maybe others.)
> 
> If it's really necessary, feel free to add mine:
> 
> Signed-off-by: Felipe Balbi <me@felipebalbi.com>
> 
> --
> balbi
> 
> _______________________________________________
> Davinci-linux-open-source mailing list 
> Davinci-linux-open-source@linux.davincidsp.com
> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
diff mbox

Patch

=====================
From: David Brownell <dbrownell@users.sourceforge.net>

Minor fixes to the davinci-nand driver patch.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/mtd/nand/davinci_nand.c |   35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -161,7 +161,7 @@  static inline u32 nand_davinci_readecc_1
 static void nand_davinci_hwctl_1bit(struct mtd_info *mtd, int mode)
 {
 	struct davinci_nand_info *info;
-	u32 retval;
+	u32 nandcfr;
 	unsigned long flags;
 
 	info = to_davinci_nand(mtd);
@@ -172,9 +172,9 @@  static void nand_davinci_hwctl_1bit(stru
 	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);
+	nandcfr = davinci_nand_readl(info, NANDFCR_OFFSET);
+	nandcfr |= BIT(8 + info->core_chipsel);
+	davinci_nand_writel(info, NANDFCR_OFFSET, nandcfr);
 
 	spin_unlock_irqrestore(&davinci_nand_lock, flags);
 }
@@ -186,13 +186,13 @@  static int nand_davinci_calculate_1bit(s
 				      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);
+	unsigned int ecc24 = (ecc_val & 0x0fff) | ((ecc_val & 0x0fff0000) >> 4);
 
 	/* 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);
+	ecc24 = ~ecc24;
+	ecc_code[0] = (u_char)(ecc24);
+	ecc_code[1] = (u_char)(ecc24 >> 8);
+	ecc_code[2] = (u_char)(ecc24 >> 16);
 
 	return 0;
 }
@@ -278,7 +278,7 @@  static int nand_davinci_dev_ready(struct
 
 static void __init nand_dm6446evm_flash_init(struct davinci_nand_info *info)
 {
-	u32 regval, tmp;
+	u32 regval, a1cr;
 
 	/*
 	 * NAND FLASH timings @ PLL1 == 459 MHz
@@ -297,11 +297,11 @@  static void __init nand_dm6446evm_flash_
 		| (3 << 2)            /* turnAround      ?? ns */
 		| (0 << 0)            /* asyncSize       8-bit bus */
 		;
-	tmp = davinci_nand_readl(info, A1CR_OFFSET);
-	if (tmp != regval) {
+	a1cr = davinci_nand_readl(info, A1CR_OFFSET);
+	if (a1cr != regval) {
 		dev_dbg(info->dev, "Warning: NAND config: Set A1CR " \
 		       "reg to 0x%08x, was 0x%08x, should be done by " \
-		       "bootloader.\n", regval, tmp);
+		       "bootloader.\n", regval, a1cr);
 		davinci_nand_writel(info, A1CR_OFFSET, regval);
 	}
 }
@@ -338,7 +338,7 @@  static int __init nand_davinci_probe(str
 	if (!res1 || !res2) {
 		dev_err(&pdev->dev, "resource missing\n");
 		ret = -EINVAL;
-		goto err_res;
+		goto err_nomem;
 	}
 
 	vaddr = ioremap(res1->start, res1->end - res1->start);
@@ -347,7 +347,6 @@  static int __init nand_davinci_probe(str
 		dev_err(&pdev->dev, "ioremap failed\n");
 		ret = -EINVAL;
 		goto err_ioremap;
-
 	}
 
 	info->dev		= &pdev->dev;
@@ -525,16 +524,14 @@  err_clk_enable:
 
 err_ecc:
 err_clk:
+err_ioremap:
 	if (base)
 		iounmap(base);
 	if (vaddr)
 		iounmap(vaddr);
 
-err_ioremap:
-	kfree(info);
-
 err_nomem:
-err_res:
+	kfree(info);
 	return ret;
 }