Dear Minkyu Kang,
In message <4aa8ac3b.5000...@samsung.com> you wrote:
> This patch includes the onenand driver for s5pc100
>
> Signed-off-by: Minkyu Kang <mk7.k...@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.p...@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.
> +/*
> + * 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.
What I see raises just additional questions: should there be no error
checking? I mean, something like
... ((fpa & 0x3f) << 7) | ((fsa & 3) << 5)
?
> 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.
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: w...@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
