Re: [U-Boot] [PATCH v3 6/7] tegra: nand: Add Tegra NAND driver
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>
- 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_
-Scott
Hi Scott,
On Mon, May 21, 2012 at 8:47 AM, Scott Wood 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>
- 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?
-Scott
Regards, Simon
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?
-Scott
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 beuseful,
>> + * 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 PublicLicense
>> + * 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, butnot
within U-Boot as a whole. Names like CMD_CLE, CMD_TX, etc. are particularly likely to conflict (at least, if at least one otherdriver
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
On 05/22/2012 03:06 PM, Scott Wood 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?
That said, I see it's in drivers/mtd/nand/ and not include/, so if you really want to structure it that way, it's OK with me.
-Scott
participants (2)
-
Scott Wood -
Simon Glass