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

Simon Glass
2014-11-23T12:59:55Z

On 20 November 2014 at 19:39, Simon Glass <sjg@chromium.org> wrote:
> Hi Masahiro,
>
> On 20 November 2014 06:06, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>> Hi Simon,
>>
>>
>>
>>
>> On Wed, 19 Nov 2014 09:35:54 +0000
>> Simon Glass <sjg@chromium.org> wrote:
>>
>>> Hi Masahiro,
>>>
>>> On 19 November 2014 08:25, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>>> > Hi Simon,
>>> >
>>> >
>>> >
>>> > On Tue, 11 Nov 2014 10:46:18 -0700
>>> > Simon Glass <sjg@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?
>
> OK
>
>>
>>
>>
>> /**
>> * 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"
>
> Will fix.

Fixed these and:

Applied to u-boot-dm.