[U-Boot] [PATCH v2 02/17] dm: core: Allow access to the device's driver_id data

[Masahiro Yamada]

Hi Simon,




On Wed, 19 Nov 2014 09:35:54 +0000
Simon Glass <sjg at chromium.org> wrote:

> Hi Masahiro,
> 
> On 19 November 2014 08:25, Masahiro Yamada <yamada.m at jp.panasonic.com> wrote:
> > Hi Simon,
> >
> >
> >
> > On Tue, 11 Nov 2014 10:46:18 -0700
> > Simon Glass <sjg at chromium.org> wrote:
> >
> >> diff --git a/drivers/core/device.c b/drivers/core/device.c
> >> index 49faa29..0d84776 100644
> >> --- a/drivers/core/device.c
> >> +++ b/drivers/core/device.c
> >> @@ -548,3 +548,8 @@ int device_find_next_child(struct udevice **devp)
> >>
> >>       return 0;
> >>  }
> >> +
> >> +ulong dev_get_of_data(struct udevice *dev)
> >> +{
> >> +     return dev->of_id->data;
> >> +}
> >
> >
> > Since this function is short enough, perhaps you might want to
> > define it as "static inline" in the header file include/dm/device.h
> >
> >
> 
> Thanks for looking at this series.
> 
> The background here is that I'm quite worried about all the pointers
> in driver model. It might be quite easy to use the wrong one and get
> confused.

Me too.

> So my plan is to add code to check that the pointers are
> what we think they are, like:
> 
>    DM_CHECK_PTR(dev, "udevice");
> 
> or similar. Then that code would compile to nothing unless it is
> enabled with a CONFIG_DM_CHECK_PTRS or whatever. That's the reason for
> accessors.
> 

Sounds good!


> >
> >> @@ -116,6 +120,7 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,
> >>  {
> >>       struct driver *driver = ll_entry_start(struct driver, driver);
> >>       const int n_ents = ll_entry_count(struct driver, driver);
> >> +     const struct udevice_id *id;
> >>       struct driver *entry;
> >>       struct udevice *dev;
> >>       bool found = false;
> >> @@ -127,7 +132,8 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,
> >>       if (devp)
> >>               *devp = NULL;
> >>       for (entry = driver; entry != driver + n_ents; entry++) {
> >> -             ret = driver_check_compatible(blob, offset, entry->of_match);
> >> +             ret = driver_check_compatible(blob, offset, entry->of_match,
> >> +                                           &id);
> >>               name = fdt_get_name(blob, offset, NULL);
> >>               if (ret == -ENOENT) {
> >>                       continue;
> >> @@ -147,6 +153,7 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,
> >>                       dm_warn("Error binding driver '%s'\n", entry->name);
> >>                       return ret;
> >>               } else {
> >> +                     dev->of_id = id;
> >
> >
> > Instead of all the chages above, only one line change,
> >
> >                         dev->of_id = entry->of_match
> >
> >
> >
> > Does this work for you?
> >
> >
> 
> entry->of_match is the first element in an array of records. I want to
> know exactly which one matches, so I can't just use the first one.
> 


Sorry, it was my misunderstanding.
Thanks for explaining this.



Could you update the comment block of driver_check_compatible?



/**
 * driver_check_compatible() - Check if a driver is compatible with this node
 *
 * @param blob:		Device tree pointer
 * @param offset:	Offset of node in device tree
 * @param of_matchL	List of compatible strings to match
 * @return 0 if there is a match, -ENOENT if no match, -ENODEV if the node
 * does not have a compatible string, other error <0 if there is a device
 * tree error
 */


  - Add description of "@param of_idp"
  - Fix  "of_matchL"  -> "of_match"




Best Regards
Masahiro Yamada