[U-Boot] [PATCH v3 6/7] tegra: nand: Add Tegra NAND driver

Simon Glass
2012-05-22T20:24:55Z

Hi Scott,

On Tue, May 22, 2012 at 1:06 PM, Scott Wood <scottwood@freescale.com> wrote:

> On 05/22/2012 03:04 PM, Simon Glass wrote:
> > Hi Scott,
> >
> > On Mon, May 21, 2012 at 8:47 AM, Scott Wood <scottwood@freescale.com
> > <mailto:scottwood@freescale.com>> wrote:
> >
> > On 05/21/2012 05:07 AM, Jim Lin wrote:
> > >> diff --git a/drivers/mtd/nand/tegra2_nand.h
> > b/drivers/mtd/nand/tegra2_nand.h
> > >> new file mode 100644
> > >> index 0000000..7e74be7
> > >> --- /dev/null
> > >> +++ b/drivers/mtd/nand/tegra2_nand.h
> > >> @@ -0,0 +1,257 @@
> > >> +/*
> > >> + * (C) Copyright 2011 NVIDIA Corporation <www.nvidia.com
> > <http://www.nvidia.com>>
> > >> + *
> > >> + * See file CREDITS for list of people who contributed to this
> > >> + * project.
> > >> + *
> > >> + * 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., 59 Temple Place, Suite 330, Boston,
> > >> + * MA 02111-1307 USA
> > >> + */
> > >> +
> > >> +/* register offset */
> > >> +#define COMMAND_0 0x00
> > >> +#define CMD_GO (1 << 31)
> > >> +#define CMD_CLE (1 << 30)
> > >> +#define CMD_ALE (1 << 29)
> > >> +#define CMD_PIO (1 << 28)
> > >> +#define CMD_TX (1 << 27)
> > >> +#define CMD_RX (1 << 26)
> > >> +#define CMD_SEC_CMD (1 << 25)
> > >> +#define CMD_AFT_DAT_MASK (1 << 24)
> > >> +#define CMD_AFT_DAT_DISABLE 0
> > >> +#define CMD_AFT_DAT_ENABLE (1 << 24)
> > >> +#define CMD_TRANS_SIZE_SHIFT 20
> > >> +#define CMD_TRANS_SIZE_PAGE 8
> > >
> > > Please use proper namespacing on symbols defined in headers.
> >
> > >
> > > [Jim] Scott, I don't know what you meant. Could you give me an
> > example?
> > > Thanks.
> > > The names like AFT_DAT_MASK, TRANS_SIZE_SHIFT, and
> > TRANS_SIZE_PAGE come from field names of register in data sheet.
> >
> > Names in your data sheet are presumably unique within that chip, but
> not
> > within U-Boot as a whole. Names like CMD_CLE, CMD_TX, etc. are
> > particularly likely to conflict (at least, if at least one other
> driver
> > were similarly loose with its naming).
> >
> > Prefix these with TEGRA_ or TEGRA_NAND_
> >
> >
> > I'm not thrilled with this idea. It adds a lot of needless boilerplate
> > to the code IMO.
> >
> > This header can only be included by the tegra NAND driver after all.
> > Please can you reconsider this?
>
> If it can only be included in the NAND driver, why does it need to be in
> a header at all?
>

It doesn't - it's just a convenient way to separate out the definitions
from the rest of the code. Quite a lot of drivers in U-Boot have private .h
files.

>
> -Scott
>
> Regards,
Simon