[U-Boot] [PATCH 02/11] DM: add support for scanning DOS partitions to blockdev core

[Pavel Herrmann]

On Friday 21 of September 2012 14:47:24 Marek Vasut wrote:
> Dear Pavel Herrmann,
> 
> [...]
> 
> > > > +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?
> 
> Argh, not u-boot.map, sorry. But it's much easier for git grep to look up
> unique syms than this.

ah, OK then

> > > > +{
> > > > +	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?
> 
> Well, I didn't see the code before, so I couldn't make a firm decision,
> sorry.
> > No idea what you mean by "one or the other" - you need all this code for
> > it
> > to work.
> 
> You need the "driver wrapping API" if DM is enabled? I do not understand
> this, please elaborate!
> 
> What about having a common part for both cases and then compile either the
> DM part or non-DM part conditionally?

this is all DM-only.
driver-wrapping API is wrapper/proxy functions to driver ops (aka it wraps 
around the ops, doing lookup and activate and whatnot), not a wrapper for old 
API (which would be in a driver mind you)

> > > > +/* 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
> 
> Best regards,
> Marek Vasut