| Message ID | 1415727993-22032-3-git-send-email-sjg@chromium.org |
|---|---|
| State | Accepted |
| Delegated to: | Simon Glass |
| Headers | show |
On Tue, Nov 11, 2014 at 10:46:18AM -0700, Simon Glass wrote: > When the device is created from a device tree node, it matches a compatible > string. Allow access to that string and the associated data. > > Signed-off-by: Simon Glass <sjg@chromium.org> Reviewed-by: Tom Rini <trini@ti.com>
Hello Simon, Am 11.11.2014 18:46, schrieb Simon Glass: > When the device is created from a device tree node, it matches a compatible > string. Allow access to that string and the associated data. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > Changes in v2: None > > drivers/core/device.c | 5 +++++ > drivers/core/lists.c | 17 ++++++++++++----- > include/dm/device.h | 11 +++++++++++ > 3 files changed, 28 insertions(+), 5 deletions(-) Acked-by: Heiko Schocher <hs@denx.de> bye, Heiko
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 > 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) > @@ -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? Best Regards Masahiro Yamada
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. 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
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? /** * 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
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. Regards, Simon
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.
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; +} 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++; } @@ -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; found = true; if (devp) *devp = dev; diff --git a/include/dm/device.h b/include/dm/device.h index 9ce95a8..287504c 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -47,6 +47,7 @@ struct driver_info; * @name: Name of device, typically the FDT node name * @platdata: Configuration data for this device * @of_offset: Device tree node offset for this device (- for none) + * @of_id: Pointer to the udevice_id structure which created the device * @parent: Parent of this device, or NULL for the top level device * @priv: Private data for this device * @uclass: Pointer to uclass for this device @@ -65,6 +66,7 @@ struct udevice { const char *name; void *platdata; int of_offset; + const struct udevice_id *of_id; struct udevice *parent; void *priv; struct uclass *uclass; @@ -206,6 +208,15 @@ void *dev_get_parentdata(struct udevice *dev); void *dev_get_priv(struct udevice *dev); /** + * dev_get_of_data() - get the device tree data used to bind a device + * + * When a device is bound using a device tree node, it matches a + * particular compatible string as in struct udevice_id. This function + * returns the associated data value for that compatible string + */ +ulong dev_get_of_data(struct udevice *dev); + +/** * device_get_child() - Get the child of a device by index * * Returns the numbered child, 0 being the first. This does not use
When the device is created from a device tree node, it matches a compatible string. Allow access to that string and the associated data. Signed-off-by: Simon Glass <sjg@chromium.org> --- Changes in v2: None drivers/core/device.c | 5 +++++ drivers/core/lists.c | 17 ++++++++++++----- include/dm/device.h | 11 +++++++++++ 3 files changed, 28 insertions(+), 5 deletions(-)