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

[Wolfgang Denk]

Dear Kyungmin Park,

In message <20081021091839.GA29297 at july> you wrote:
> Tm93IGl0IGNhbiB1c2UgVUJJIHN1cHBvcnQgb24gVS1Cb290CgpTaWduZWQtb2ZmLWJ5OiBLeXVu
> Z21pbiBQYXJrIDxreXVuZ21pbi5wYXJrQHNhbXN1bmcuY29tPgotLS0KIE1ha2VmaWxlICAgICAg
> ICAgICAgICAgICAgICAgICB8ICAgIDEgKwogZHJpdmVycy9tdGQvTWFrZWZpbGUgICAgICAgICAg
> IHwgICAgMSArCiBkcml2ZXJzL210ZC9tdGRjb3JlLmMgICAgICAgICAgfCAgMTQyICsrKysKIGRy
> aXZlcnMvbXRkL210ZHBhcnQuYyAgICAgICAgICB8ICA1MzEgKysrKysrKysrKysrKwogZHJpdmVy
...

Please post patches as PLAIN TEXT, not with base64 encoding!!!

There are lots of coding style violations: trailing white space, using
spaces instead of TABs for indentation and for vertical alignment,
using C++ comments, etc. 

> Now it can use UBI support on U-Boot

That's not really a good commit message, it seems.


> index 0000000..a9f494a
> --- /dev/null
> +++ b/drivers/mtd/mtdcore.c
> @@ -0,0 +1,142 @@
> +/*
> + * $Id: mtdcore.c,v 1.47 2005/11/07 11:14:20 gleixner Exp $

Please do not include versioninformation in files. We use git for
this.

> + * Core registration and callback routines for MTD
> + * drivers and users.
> + */

GPL header missing - here and in many other files.

> +int add_mtd_device(struct mtd_info *mtd)
> +{
> +	int i;
> +
> +	BUG_ON(mtd->writesize == 0);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

What's that?

> +	for (i=0; i < MAX_MTD_DEVICES; i++)

Need "{...}" for multi-line statements.

> +		if (!mtd_table[i]) {
> +			mtd_table[i] = mtd;
> +			mtd->index = i;
> +			mtd->usecount = 0;
> +
> +			printf("mtd: Giving out device %d to %s\n",i, mtd->name);
> +			/* No need to get a refcount on the module containing
> +			   the notifier, since we hold the mtd_table_mutex */
> +
> +			/* We _know_ we aren't being removed, because
> +			   our caller is still holding us here. So none
> +			   of this try_ nonsense, and no bitching about it
> +			   either. :) */
> +			return 0;
> +		}
> +
> +	return 1;
> +}
> +
> +/**

Please use plain "/*" as start of multiline comments. Here and
everywhere.

> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> new file mode 100644
> index 0000000..113516a
> --- /dev/null
> +++ 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?

I'm not exactly happy about this.

...
> +#if 0
> +	if (unlikely(res)) {
> +		if (res == -EUCLEAN)
> +			mtd->ecc_stats.corrected++;
> +		if (res == -EBADMSG)
> +			mtd->ecc_stats.failed++;
> +	}
> +#endif
> +	return res;
> +}
> +
> +#if 0
...

Please do not add dead code.

> index 0000000..bf8a19f
> --- /dev/null
> +++ b/drivers/mtd/ubi/build.c
> @@ -0,0 +1,1182 @@
> +/*
> + * Copyright (c) International Business Machines Corp., 2006
> + * Copyright (c) Nokia Corporation, 2007
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
> + * the GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + * Author: Artem Bityutskiy (Битюцкий Артём),
> + *         Frank Haverkamp
> + */
> +
> +/*
> + * This file includes UBI initialization and building of UBI devices.
> + *
> + * When UBI is initialized, it attaches all the MTD devices specified as the
> + * module load parameters or the kernel boot parameters. If MTD devices were
> + * specified, UBI does not attach any MTD device, but it is possible to do
> + * later using the "UBI control device".
> + *
> + * At the moment we only attach UBI devices by scanning, which will become a
> + * bottleneck when flashes reach certain large size. Then one may improve UBI
> + * and add other methods, although it does not seem to be easy to do.
> + */
> +
> +#if 0
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/stringify.h>
> +#include <linux/stat.h>
> +#include <linux/miscdevice.h>
> +#include <linux/log2.h>
> +#include <linux/kthread.h>
> +#endif

Please do not add dead code - here and everywhere.

> +
> +/**
> + * ubi_major2num - get UBI device number by character device major number.
> + * @major: major number
> + *
> + * This function searches UBI device number object by its major number. If UBI
> + * device was not found, this function returns -ENODEV, otherwise the UBI device
> + * number is returned.
> + */
> +int ubi_major2num(int major)
> +{
> +	int i, ubi_num = -ENODEV;
> +
> +	spin_lock(&ubi_devices_lock);
> +	for (i = 0; i < UBI_MAX_DEVICES; i++) {
> +		struct ubi_device *ubi = ubi_devices[i];
> +
> +		if (ubi && MAJOR(ubi->cdev.dev) == major) {
> +			ubi_num = ubi->ubi_num;
> +			break;
> +		}
> +	}
> +	spin_unlock(&ubi_devices_lock);
> +
> +	return ubi_num;
> +}

Hm... it does not make sense to me to pull in the whole Linux OS
unfiltered.

Should we not try and restrict the code to the functions that are
really needed and used in U-Boot?

We do not have any such stuff like major or minor device numbers here.
And no permissions and al the othe rstuff.

> +#if 0
> +/* "Show" method for files in '/<sysfs>/class/ubi/ubiX/' */
> +static ssize_t dev_attribute_show(struct device *dev,
> +				  struct device_attribute *attr, char *buf)

Argh... Again tons of dead code.



I stop reviewing here. It makes no sense.

Can you pelase discuss with Stefan internally how to reduce the amount
of code you are importing?

It definitely makes no sense to me to import the whole Linux OS here.
If we think we want to run this  code,  then  we  should  boot  Linux
instead :-(

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
"A complex system that works is invariably found to have evolved from
a simple system that worked."             - John Gall, _Systemantics_