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

[Marek Vasut <marex@denx.de>]

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.

> > > +{
> > > +	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?

> > > +/* 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