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

[Wolfgang Denk]

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.


> +/*
> + * 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: 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"