From mboxrd@z Thu Jan  1 00:00:00 1970
From: Pavel Herrmann <morpheus.ibis@gmail.com>
Date: Fri, 21 Sep 2012 09:22:59 +0200
Subject: [U-Boot] [PATCH 02/11] DM: add support for scanning DOS
	partitions to blockdev core
In-Reply-To: <201209202203.05607.marex@denx.de>
References: <1348169867-2917-1-git-send-email-morpheus.ibis@gmail.com>
	<1348169867-2917-3-git-send-email-morpheus.ibis@gmail.com>
	<201209202203.05607.marex@denx.de>
Message-ID: <4092011.oPyBLZ8jbN@bloomfield>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Thursday 20 of September 2012 22:03:05 Marek Vasut wrote:
> Dear Pavel Herrmann,
> 
> [..]
> 
> > +#define BLOCKDEV_IFTYPE_BITS 4
> > +#define BLOCKDEV_IFTYPE_COUNT (1<<BLOCKDEV_IFTYPE_BITS)
> > +#define BLOCKDEV_IFTYPE_MAX BLOCKDEV_IFTYPE_COUNT-1
> 
> I saw this in blockdev.h

My bad then, sorry.

...
> > +}
> > +
> > +static int replace(struct core_instance *core, struct instance *new,
> > +	struct instance *old)
> > +{
> > +	struct blockdev_core_entry *entry = get_entry_by_instance(old);
> > +
> > +	if (!entry)
> > +		return -ENOENT;
> > +
> > +	entry->instance = new;
> > +
> > +	return 0;
> > +}
> > +
> > +static int init(struct core_instance *core)
> 
> I'd say, rename it to block_core_init() or something, so the syms in
> u-boot.map are unique.

thic being static, how could it show in u-boot.map?

> > +{
> > +	INIT_LIST_HEAD(&core->succ);
> > +	core->private_data = NULL;
> > +
> > +	return 0;
> > +}
> > +
> > +static int reloc(struct core_instance *core, struct core_instance *old)
> > +{
> > +	struct blockdev_core_entry *entry, *new;
> > +
> > +	/* no private_data to copy, yet */
> > +
> > +	/* fixup links in old list and prepare new list head */
> > +	/* FIXME */
> > +	/* list_fix_reloc(&old->succ); */
> > +	INIT_LIST_HEAD(&core->succ);
> > +	core->private_data = NULL;
> > +
> > +	/* copy list entries to new memory */
> > +	list_for_each_entry(entry, &old->succ, list) {
> > +		new = malloc(sizeof(*new));
> > +		if (!new)
> > +			return -ENOMEM;
> > +
> > +		INIT_LIST_HEAD(&new->list);
> > +		new->instance = entry->instance;
> > +		new->ops = entry->ops;
> > +		new->name = entry->name;
> > +		list_add_tail(&new->list, &core->succ);
> > +		/*no free at this point, old memory should not be freed*/
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int destroy(struct core_instance *core)
> > +{
> > +	struct blockdev_core_entry *entry, *n;
> > +
> > +	/* destroy private data */
> > +	free(core->private_data);
> > +	core->private_data = NULL;
> > +
> > +	/* destroy successor list */
> > +	list_for_each_entry_safe(entry, n, &core->succ, list) {
> > +		list_del(&entry->list);
> > +		free(entry);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +U_BOOT_CORE(CORE_BLOCKDEV,
> > +	init,
> > +	reloc,
> > +	destroy,
> > +	get_count,
> > +	get_child,
> > +	bind,
> > +	unbind,
> > +	replace);
> 
> Sep the stuff below away into separate file. Conditionally compile in one or
> the other.

I distinctly remember you saying to put all this into one file (as opposed to 3 
it was before), so why the turn-around now?
No idea what you mean by "one or the other" - you need all this code for it to 
work.

> > +/* Driver wrapping API */
> > +lbaint_t blockdev_read(struct instance *i, lbaint_t start, lbaint_t
> > blkcnt, +	void *buffer)
> > +{
> > +	struct blockdev_core_entry *entry = NULL;
> > +	struct blockdev_ops *device_ops = NULL;
> > +	int error;
> > +
> > +	entry = get_entry_by_instance(i);
> > +	if (!entry)
> > +		return -ENOENT;
> > +
> > +	error = driver_activate(i);
> > +	if (error)
> > +		return error;
> > +
> > +	device_ops = entry->ops;
> > +	if (!device_ops || !device_ops->read)
> > +		return -EINVAL;
> > +
> > +	return device_ops->read(i, start, blkcnt, buffer);
> > +}

Pavel Herrmann