Hi Masahiro,
On 20 November 2014 06:06, Masahiro Yamada <yamad...@jp.panasonic.com> wrote:
> Hi Simon,
>
>
>
>
> On Wed, 19 Nov 2014 09:35:54 +0000
> Simon Glass <s...@chromium.org> wrote:
>
>> Hi Masahiro,
>>
>> On 19 November 2014 08:25, Masahiro Yamada <yamad...@jp.panasonic.com> wrote:
>> > Hi Simon,
>> >
>> >
>> >
>> > On Tue, 11 Nov 2014 10:46:18 -0700
>> > Simon Glass <s...@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.
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
