Dear Wolfgang and Stefan,
On Tue, Oct 21, 2008 at 10:16 PM, Wolfgang Denk wd@denx.de wrote:
Dear Stefan Roese,
In message 200810211435.39059.sr@denx.de you wrote:
That's directly imported from the Linux source. Most of your comments below also deal with this Linux code copying. I personally think it is a good idea to clone the code from Linux instead of writing a U-Boot specific UBI driver. We have done this before on other occasions and it makes perfect sense here too from my point of view. It makes porting easier and faster and less error prone and even more important, it makes porting of new versions, fixes etc easier.
We agree on that. So maybe we should send coding style cleanup patches for the Linux kenrel?
- Core registration and callback routines for MTD
- drivers and users.
- */
GPL header missing - here and in many other files.
I added the GPL.
This *is* a serious issue. And I don;t care wether the code comes from Linux or not, but I will not accept any code without absolutley clear licensing conditions.
Need "{...}" for multi-line statements.
Again from the Linux original source. When we start changing this code because of coding-style issue, it will get very hard to compare those versions and add fixes in the future. So I vote for keeping the code as is. It's not that ugly, at least from my understanding.
It is a clear violation of Linux Coding Style requirements.
If we don;t want to fix it here, I suggest to fixit at the origin, i.e. in Linux.
+++ b/drivers/mtd/mtdpart.c @@ -0,0 +1,531 @@ +/*
- Simple MTD partitioning layer
Hm... We were talking about UBI support.
Do we really have to pull in the whole MTD layer from Linux for this?
...
Please do not add dead code.
It's needed. almost flash related code except the NOR, are based on MTD and uses. Event though it's only compiled UBI command, it will be changed. Now partition codes use own implementation at u-boot, I think it's better MTD partition code.
Here I'm not sure which version I prefer. The one Kyungmin used, disabling > the not needed code via #if 0 (or something else, like #if UBI_LINUX), or completely removing it. Both has some advantages. But again for comparison> with the original Linux source it's perhaps better to just disable the code> . An "#if UBI_LINIX" is probably better than on "#if 0" though.
I change to use UBI_LINUX instead of if 0.
There is two things I am concerned about:
- Do we really need all these files?
- Do we really need all of this code?
Lots of the code that was left in was obviously dealing with features we don;t have in U-Boot, and will not have: concepts like user IDs, file permissions, major and minor numbers, etc.
Of course we haven't. But it's hard to draw the line *if* you decide to include the Linux source. Most OS functions (like spin_lock()...) are defin> ed to no-ops in ubi_uboot.h. So it doesn't really increase the code size for U-Boot. It just keeps the source in-line with the Linux version.
On the other hand it makes it semi-impossible to review the code in U-Boot context, or even to get an understanding of flow of control etc.
Basically I think if we use the imported code. we don't modify code itself. Instead we give some adaptation layer or wrapper. In this case I use the latter. ubi_uboot.h wrapped the UBI kernel code and can run it on u-boot. Of course it disables the some linux specific kernel if can't wrap.
Meanwhile who is the main users of this modules? It's linux kernel. It means users can familiar with kernel UBI code and they can use it more easily. They don't need to understand the UBI code at u-boot.
Thank you, Kyungmin Park