Dear Wolfgang
2009/9/10 Wolfgang Denk wd@denx.de:
Dear Minkyu Kang,
In message 4AA8AC3B.5000604@samsung.com you wrote:
This patch includes the onenand driver for s5pc100
Signed-off-by: Minkyu Kang mk7.kang@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
...
+struct s3c_onenand {
- struct mtd_info *mtd;
- void __iomem *base;
- void __iomem *ahb_addr;
- int bootram_command;
- void __iomem *page_buf;
- void __iomem *oob_buf;
- unsigned int (*mem_addr)(int fba, int fpa, int fsa);
- struct samsung_onenand *reg;
+};
Please drop these blank lines.
ok.
+/*
- 1Gb: FBA[21:12] FPA[11:6] FSA[5:4]
- 2Gb: FBA[22:12] FPA[11:6] FSA[5:4]
- 4Gb: FBA[23:12] FPA[11:6] FSA[5:4]
- */
+static unsigned int s3c64xx_mem_addr(int fba, int fpa, int fsa) +{
- return (fba << 12) | (fpa << 6) | (fsa << 4);
+}
+/*
- 1Gb: FBA[22:13] FPA[12:7] FSA[6:5]
- 2Gb: FBA[23:13] FPA[12:7] FSA[6:5]
- 4Gb: FBA[24:13] FPA[12:7] FSA[6:5]
- */
+static unsigned int s5pc100_mem_addr(int fba, int fpa, int fsa) +{
- return (fba << 13) | (fpa << 7) | (fsa << 5);
+}
Hm... when I wrote "This function needs explanation. Please add a comment what it does, and how." I meant some comment that really explains what is goong on here - what the arguments are, what the return value is, and which sort of transformation the function performs. Your comment is not exactly helpful.
I can see what the code is doing, but I have no idea what that means - reading your comments don't help my understanding.
ok I'll write more comments.
What I see raises just additional questions: should there be no error checking? I mean, something like
... ((fpa & 0x3f) << 7) | ((fsa & 3) << 5)
?
hm.. I think no need. because of each capacity have different offset and each argument is got from mtd. Kyungmin, how you think?
diff --git a/include/linux/mtd/samsung_onenand.h b/include/linux/mtd/samsung_onenand.h new file mode 100644 index 0000000..d389606 --- /dev/null +++ b/include/linux/mtd/samsung_onenand.h
...
+#ifndef __ASSEMBLY__ +struct samsung_onenand {
- unsigned long MEM_CFG; /* 0x0000 */
- unsigned char res1[0xc];
- unsigned long BURST_LEN; /* 0x0010 */
- unsigned char res2[0xc];
- unsigned long MEM_RESET; /* 0x0020 */
- unsigned char res3[0xc];
...
Upper case vaiable names are not allowed. Upper case identifiers are reserved for macros only. Please use lower case names.
sure i do.
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de That's the thing about people who think they hate computers. What they really hate is lousy programmers.
- Larry Niven and Jerry Pournelle in "Oath of Fealty"
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
thanks Minkyu Kang