From patchwork Fri Feb 27 22:29:32 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kevin Hilman X-Patchwork-Id: 23824 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from bombadil.infradead.org (bombadil.infradead.org [18.85.46.34]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id D5049DDD1B for ; Sat, 28 Feb 2009 09:32:58 +1100 (EST) Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.69 #1 (Red Hat Linux)) id 1LdBDG-0006zJ-ST; Fri, 27 Feb 2009 22:29:46 +0000 Received: from mail-fx0-f165.google.com ([209.85.220.165]) by bombadil.infradead.org with esmtp (Exim 4.69 #1 (Red Hat Linux)) id 1LdBDB-0006z1-HL for linux-mtd@lists.infradead.org; Fri, 27 Feb 2009 22:29:44 +0000 Received: by fxm9 with SMTP id 9so693234fxm.18 for ; Fri, 27 Feb 2009 14:29:39 -0800 (PST) Received: by 10.223.115.193 with SMTP id j1mr3744846faq.98.1235773778934; Fri, 27 Feb 2009 14:29:38 -0800 (PST) Received: from localhost (deeprooted.net [216.254.16.51]) by mx.google.com with ESMTPS id t2sm2235140gve.2.2009.02.27.14.29.35 (version=TLSv1/SSLv3 cipher=RC4-MD5); Fri, 27 Feb 2009 14:29:38 -0800 (PST) To: David Brownell Subject: Re: [patch/RESEND 2.6.29-rc3-git] NAND: davinci_nand driver References: <200902241509.54649.david-b@pacbell.net> <20090226123341.a2421733.akpm@linux-foundation.org> <200902261515.22197.david-b@pacbell.net> From: Kevin Hilman Organization: Deep Root Systems, LLC In-Reply-To: <200902261515.22197.david-b@pacbell.net> (David Brownell's message of "Thu\, 26 Feb 2009 15\:15\:21 -0800") User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.2 (gnu/linux) Date: Fri, 27 Feb 2009 14:29:32 -0800 Message-ID: <87k57ba4z7.fsf@deeprootsystems.com> MIME-Version: 1.0 X-Spam-Score: 0.0 (/) Cc: Andrew Morton , linux-mtd@lists.infradead.org, davinci-linux-open-source@linux.davincidsp.com, linux-kernel@vger.kernel.org X-BeenThere: linux-mtd@lists.infradead.org X-Mailman-Version: 2.1.9 Precedence: list List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-mtd-bounces@lists.infradead.org Errors-To: linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org David Brownell writes: > On Thursday 26 February 2009, Andrew Morton wrote: > >> > + * Copyright (C) 2006 Texas Instruments. >> > + * >> > + * Ported to 2.6.23 Copyright (C) 2008 by >> > + * Sander Huijsen >> > + * Troy Kisky >> > + * Dirk Behme >> >> 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 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.) > Done. It also needs this cleanup bit below which is in DaVinci git now that we've deprecated the use of the davinci cpu_is_* macros in drivers. This could be just folded into current patch if desired. Thanks, Kevin commit 1bacc33ccc9bd0f3c109bf8a8550e9b6f99397bd Author: Kevin Hilman Date: Thu Feb 26 17:15:18 2009 -0800 MTD: NAND: drop usage of cpu_is_* macro Usage of davinci-specific cpu_is macros is not allowed in drivers. These options should be passed in through platform_data. Signed-off-by: Kevin Hilman diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c index aa70b4e..a2f78ad 100644 --- a/drivers/mtd/nand/davinci_nand.c +++ b/drivers/mtd/nand/davinci_nand.c @@ -33,7 +33,6 @@ #include #include -#include #include #include @@ -392,8 +391,6 @@ static int __init nand_davinci_probe(struct platform_device *pdev) /* use board-specific ECC config; else, the best available */ if (pdata) ecc_mode = pdata->ecc_mode; - else if (cpu_is_davinci_dm355()) - ecc_mode = NAND_ECC_HW_SYNDROME; else ecc_mode = NAND_ECC_HW;