Dear Kyungmin Park,
In message 9c9fda240909040406m2a8597d4q4b10ef55d33d628a@mail.gmail.com you wrote:
PLease see comments to previous patch about usage of register plus offset versus using proper C structures.
I have different opinion. How much register map is required? I mean in
Size does actually not matter as you do not create any objects of such type, you just set a pointer to allow the compiler to the arithmetics and type checking for you.
case of read/write cmd, it has about 256MiB range. then how do you cast is as C structures.
Well, the OneNAND Controller offsets are all withion a single 1 kB range, right?
Only some small range for example. 1K or less. C structures possible. but more than this. it's too huge to case it.
Where exactly do you think is a data structure too big to be handled this way?
Please change this code to use structs, too.
+static unsigned int s5pc100_mem_addr(int fba, int fpa, int fsa) +{
return (fba << 13) | (fpa << 7) | (fsa << 5);+}
This function needs explanation. Please add a comment what it does, and how.
It's mandatory. no need to shift. It's fixed. with this reason. we covers alomst 256MiB memory range.
You don't understand me.
I asked you to add a comment that documents what this function does, whet the arguments mean, which value it returns, and what the magic constants used in the code mean.
+#ifdef CONFIG_S5PC1XX
/* S5PC100 specific values */onenand->base = (void *) 0xE7100000;onenand->ahb_addr = (void *) 0xB0000000;onenand->mem_addr = s5pc100_mem_addr;+#else +#error Please fix it at s3c6410 +#endif
What does this mean?
Now we got two version of samsung.c one for s3c6410, another is this one. No problem to add it for s3c6410
Ah! Two versions of samsung.c? This is bad. Please avoid such code duplication. Create a common file that covers both systems.
+/*
- OneNAND Controller
- */
+#define MEM_CFG_OFFSET 0x0000 +#define BURST_LEN_OFFSET 0x0010 +#define MEM_RESET_OFFSET 0x0020 +#define INT_ERR_STAT_OFFSET 0x0030 +#define INT_ERR_MASK_OFFSET 0x0040 +#define INT_ERR_ACK_OFFSET 0x0050
Please use a C struct instead.
The end address is 0x400. Are you really need it to cast? I don't think so.
I don't understand what you complain about. What's wrong about to have a structure for that? That its sparse populated? No problem.
Keep in mind that we do not create any such objects, we just use the defintion to desctibe the structure of your hardware, so the compiler can to the address calculation and typoe checking for us.
Best regards,
Wolfgang Denk