Hi Marek,
On Fri, Jun 28, 2013 at 2:53 PM, Marek Vasut <ma...@denx.de> wrote:
> Dear Simon Glass,
>
> > Add driver model functionality for generic board.
> >
> > This includes data structures and base code for registering devices and
> > uclasses (groups of devices with the same purpose, e.g. all I2C ports
> will
> > be in the same uclass).
> >
> > The feature is enabled with CONFIG_DM.
> >
> > Signed-off-by: Simon Glass <s...@chromium.org>
> > Signed-off-by: Marek Vasut <ma...@denx.de>
> > Signed-off-by: Pavel Herrmann <morpheus.i...@gmail.com>
> > Signed-off-by: Viktor Křivák <viktor.kri...@gmail.com>
> > Signed-off-by: Tomas Hlavacek <tmshl...@gmail.com>
>
I am going to tidy this up for a v4, in case you are wondering why I am
only getting to this now.
> > ---
> > Changes in v3:
> > - Tidy up commenting of functions and structures
> > - Rename per_device_priv_size to per_device_auto_alloc_size, etc.
> > - Add a flag for tracking whether DM allocates/frees platform_data
> >
> > Changes in v2: None
> >
> > Makefile | 2 +
> > common/dm/Makefile | 43 +++++
> > common/dm/device.c | 331
> > ++++++++++++++++++++++++++++++++++++++ common/dm/lists.c
> |
> > 168 +++++++++++++++++++
> > common/dm/root.c | 115 +++++++++++++
> > common/dm/uclass.c | 298
> ++++++++++++++++++++++++++++++++++
> > common/dm/util.c | 50 ++++++
>
> Maybe we should move these into drivers/core like Linux has it ?
>
Done
>
> [...]
>
> > +#include <common.h>
> > +#include <malloc.h>
> > +#include <dm/device.h>
> > +#include <dm/device-internal.h>
> > +#include <dm/lists.h>
> > +#include <dm/platform_data.h>
> > +#include <dm/uclass.h>
> > +#include <dm/uclass-internal.h>
> > +#include <dm/util.h>
> > +#include <linux/err.h>
> > +#include <linux/list.h>
> > +
> > +/**
> > + * device_chld_unbind() - Unbind all device's children from the device
> > + * @dev: The device that is to be stripped of its children
> > + * @return 0 on success, -ve on error
> > + */
> > +static int device_chld_unbind(struct device *dev)
> > +{
> > + struct device *pos, *n;
> > + int ret;
> > +
> > + assert(dev);
> > +
> > + list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node) {
> > + ret = device_unbind(pos);
> > + if (ret)
> > + return ret;
> > + }
>
> This is used for stuff like destroying USB bus topology after running "usb
> rescan" for the second time, right? We might want to continue unbinding
> and only
> report the error instead of returning on the first error.
>
Done
>
> > + return 0;
> > +}
> > +
> > +/**
> > + * device_chld_remove() - Stop all device's children
> > + * @dev: The device whose children are to be removed
> > + * @return 0 on success, -ve on error
> > + */
> > +static int device_chld_remove(struct device *dev)
> > +{
> > + struct device *pos, *n;
> > + int ret;
> > +
> > + assert(dev);
> > +
> > + list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node) {
> > + ret = device_remove(pos);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
>
> [...]
>
> We might really want to document these.
>
Yes - they are documented in the header file include/dm/device-internal.h
>
> > +int device_bind_by_name(struct device *parent, const struct driver_info
> > *info, + struct device **devp)
>
> [...]
>
> > +struct uclass *uclass_find(enum uclass_id key)
> > +{
> > + struct uclass *uc;
> > +
> > + /*
> > + * TODO(s...@chromium.org): Optimise this, perhaps moving the found
> > + * node to the start of the list, or creating a linear array
> mapping
> > + * id to node.
> > + */
>
> We better do this later, after it's all in place.
>
Yes, I'm not sure how important it is. We only have about 3 uclasses. This
will be a nice problem to have when all the drivers are converted...
>
> > + list_for_each_entry(uc, &gd->uclass_root, sibling_node) {
> > + if (uc->uc_drv->id == key)
> > + return uc;
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +/**
> > + * uclass_add() - Create new uclass in list
> > + * @id: Id number to create
> > + * @ucp: Returns pointer to uclass, or NULL on error
> > + * @return 0 on success, -ve on error
> > + *
> > + * The new uclass is added to the list. There must be only one uclass
> for
> > + * each id.
> > + */
> > +static int uclass_add(enum uclass_id id, struct uclass **ucp)
> > +{
>
> I might be a bit lost here. Do we now support adding uclass at runtime ?
>
Sort-of. The uclass_get() function is the only one exported, but it in turn
calls uclass_add(). We could open this up later, but I don't see a lot of
benefit, since we only want one uclass per ID.
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
