[U-Boot] [PATCH] [UBI] Basic Unsorted Block Image (UBI) support (part 1)

[Kyungmin Park]

Dear Wolfgang and Stefan,

On Tue, Oct 21, 2008 at 10:16 PM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Stefan Roese,
>
> In message <200810211435.39059.sr at 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:
>
> 1) Do we really need all these files?
> 2) 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