[U-Boot] [PATCH v3 07/16] dm: Add base driver model support

[Simon Glass]

Hi Marek,

On Fri, Jun 28, 2013 at 2:53 PM, Marek Vasut <marex at 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 <sjg at chromium.org>
> > Signed-off-by: Marek Vasut <marex at denx.de>
> > Signed-off-by: Pavel Herrmann <morpheus.ibis at gmail.com>
> > Signed-off-by: Viktor Křivák <viktor.krivak at gmail.com>
> > Signed-off-by: Tomas Hlavacek <tmshlvck at 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(sjg at 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