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

[Simon Glass]

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. 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.

>
>
>
>> diff --git a/drivers/core/lists.c b/drivers/core/lists.c
>> index 3a1ea85..9f33dde 100644
>> --- a/drivers/core/lists.c
>> +++ b/drivers/core/lists.c
>> @@ -89,22 +89,26 @@ int lists_bind_drivers(struct udevice *parent, bool pre_reloc_only)
>>   * tree error
>>   */
>>  static int driver_check_compatible(const void *blob, int offset,
>> -                                const struct udevice_id *of_match)
>> +                                const struct udevice_id *of_match,
>> +                                const struct udevice_id **of_idp)
>>  {
>>       int ret;
>>
>> +     *of_idp = NULL;
>>       if (!of_match)
>>               return -ENOENT;
>>
>>       while (of_match->compatible) {
>>               ret = fdt_node_check_compatible(blob, offset,
>>                                               of_match->compatible);
>> -             if (!ret)
>> +             if (!ret) {
>> +                     *of_idp = of_match;
>>                       return 0;
>> -             else if (ret == -FDT_ERR_NOTFOUND)
>> +             } else if (ret == -FDT_ERR_NOTFOUND) {
>>                       return -ENODEV;
>> -             else if (ret < 0)
>> +             } else if (ret < 0) {
>>                       return -EINVAL;
>> +             }
>>               of_match++;
>>       }
>
>
>
> I think you are making things more complicated than is needed.
> I guess what you want to do in this patch is just to set "dev->of_id".
>
> Why do you need to touch this function?   (Please see below)
>
>

See below.

>
>
>
>
>> @@ -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.

Regards,
Simon