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

[Wolfgang Denk]

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.

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

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.

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
"Why should we subsidize intellectual curiosity?" - Ronald Reagan