[U-Boot] [PATCH 2/4 v2] s5pc1xx: support onenand driver

[Minkyu Kang]

Dear Wolfgang

2009/9/10 Wolfgang Denk <wd at denx.de>:
> Dear Minkyu Kang,
>
> In message <4AA8AC3B.5000604 at samsung.com> you wrote:
>> This patch includes the onenand driver for s5pc100
>>
>> Signed-off-by: Minkyu Kang <mk7.kang at samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park at 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 at 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 at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

thanks
Minkyu Kang
-- 
from. prom.
www.promsoft.net