[PATCH v5] ubi: Introduce block devices for UBI volumes

[Artem Bityutskiy]

Thanks for the driver Ezequiel!

A couple of nit-pick below, though.

On Thu, 2014-02-13 at 16:58 -0300, Ezequiel Garcia wrote:
> +++ b/drivers/mtd/ubi/block.c
> @@ -0,0 +1,656 @@
> +/*
> + * Copyright (c) 2014 Ezequiel Garcia
> + * Copyright (c) 2011 Free Electrons
> + *
> + * 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, version 2.
> + *
> + * 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.
> + *
> + * A block implementation on UBI volume
> + * ------------------------------------
> + *
> + * How to use this? A bunch of examples worth a thousand words:
> + *
> + * If you want to attach a block device on bootup time, e.g. in order
> + * to mount the rootfs on such a block device:
> + *
> + * Using the UBI volume path:
> + *  ubi.block=/dev/ubi0_0
> + *
> + * Using the UBI device, and the volume name:
> + *  ubi.block=0,rootfs
> + *
> + * Using both UBI device number and UBI volume number:
> + *  ubi.block=0,0
> + *
> + * In this case you would have such kernel parameters:
> + *  ubi.mtd=5 ubi.block=0,0 root=/dev/ubiblock0_0
> + *
> + * Of course, if you compile ubi as a module you can use this
> + * parameter on module insertion time:
> + *  modprobe ubi mtd=/dev/mtd5 block=/dev/ubi0_0
> + *
> + * For runtime block attaching/detaching, see mtd-utils' ubiblkvol tool.
> + */

Good comments is a great thing, very appreciated. However, you already
put a similar piece of documentation to "MODULE_PARM_DESC()", which is a
good place, because it is visible to the end-user via 'modinfo'.

Do you think it will make users of this driver understand the the usage
model better if you duplicate the documentation here? May be, not sure.

I'd say the risk is that people modifying this driver may change this
comment, but forget to change the modinfo output, or vice-versa.

I'd refactor this comment to make it look more like a piese of doc for
the developer, explaining how this module is related to the ubi module.
Rather than being a duplicate piece of doc for the end user explaining
how to use this module...

> +/* Linked list of all ubiblock instances */
> +static LIST_HEAD(ubiblock_devices);
> +static DEFINE_MUTEX(devices_mutex);
> +static int ubiblock_major;
> +
> +/* Ugh! this parameter parsing code is awful */

If it is awful, would you also expand the comment and explain why it is
awful, and what should be done to make it nice. Or better just do it! I
mean, generally, when criticizing something it is better to explain why
and point to the alternatives, no?

Thanks!

-- 
Best Regards,
Artem Bityutskiy