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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot