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