Dear Wolfgang
2009/9/10 Wolfgang Denk <w...@denx.de>:
> 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.
>
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: 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
>
thanks
Minkyu Kang
--
from. prom.
www.promsoft.net
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
