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

Minkyu Kang
2009-09-15T02:40:51Z

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 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.