[U-Boot] [PATCH v6 09/12] dm: core: Match compatible strings in order of priority

[Simon Glass]

Hi Paul,

On 4 August 2016 at 04:29, Paul Burton <paul.burton at imgtec.com> wrote:
> Device model drivers have previously been matched to FDT nodes by virtue
> of being the first driver in the driver list to be compatible with the
> node. This ignores the fact that compatible strings in the device tree
> are listed in order of priority - that is, if we have a node with 2
> compatible strings & a driver that matches each then we should always
> probe the driver that matches the first compatible string.
>
> Fix this by looping through the compatible strings for a node when
> attempting to bind it in lists_bind_fdt and checking each driver for
> a match of the first string, then each driver for a match of the second
> string etc. Effectively this inverts the loops over compatible strings &
> drivers.
>
> Signed-off-by: Paul Burton <paul.burton at imgtec.com>
> ---
>
> Changes in v6: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
>  drivers/core/lists.c | 83 ++++++++++++++++++++++++++--------------------------
>  1 file changed, 41 insertions(+), 42 deletions(-)
>

This looks generally OK, but please make sure that the tests pass
(./test/run) and see below.

> diff --git a/drivers/core/lists.c b/drivers/core/lists.c
> index 6a634e6..bde6ec1 100644
> --- a/drivers/core/lists.c
> +++ b/drivers/core/lists.c
> @@ -101,37 +101,21 @@ int device_bind_driver_to_node(struct udevice *parent, const char *drv_name,
>
>  #if CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)
>  /**
> - * driver_check_compatible() - Check if a driver is compatible with this node
> + * driver_check_compatible() - Check if a driver matches a compatible string
>   *
> - * @param blob:                Device tree pointer
> - * @param offset:      Offset of node in device tree
>   * @param of_match:    List of compatible strings to match
> - * @param of_idp:      Returns the match that was found
> - * @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
> + * @param compat:      The compatible string to search for
> + * @return 0 if there is a match, -ENOENT if no match
>   */
> -static int driver_check_compatible(const void *blob, int offset,
> -                                  const struct udevice_id *of_match,
> -                                  const struct udevice_id **of_idp)
> +static int driver_check_compatible(const struct udevice_id *of_match,
> +                                  const char *compat)
>  {
> -       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) {
> -                       *of_idp = of_match;
> +               if (!strcmp(of_match->compatible, compat))
>                         return 0;
> -               } else if (ret == -FDT_ERR_NOTFOUND) {
> -                       return -ENODEV;
> -               } else if (ret < 0) {
> -                       return -EINVAL;
> -               }
>                 of_match++;
>         }
>
> @@ -143,35 +127,52 @@ 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;
> -       const char *name;
> +       const char *name, *compat_list, *compat;
> +       int compat_length, i;
>         int result = 0;
>         int ret = 0;
>
> -       dm_dbg("bind node %s\n", fdt_get_name(blob, offset, NULL));
> +       name = fdt_get_name(blob, offset, NULL);
> +       dm_dbg("bind node %s\n", name);
>         if (devp)
>                 *devp = NULL;
> -       for (entry = driver; entry != driver + n_ents; entry++) {
> -               ret = driver_check_compatible(blob, offset, entry->of_match,
> -                                             &id);
> -               name = fdt_get_name(blob, offset, NULL);
> -               if (ret == -ENOENT) {
> -                       continue;
> -               } else if (ret == -ENODEV) {
> +
> +       compat_list = fdt_getprop(blob, offset, "compatible", &compat_length);
> +       if (!compat_list) {
> +               if (compat_length == -FDT_ERR_NOTFOUND) {
>                         dm_dbg("Device '%s' has no compatible string\n", name);
> -                       break;
> -               } else if (ret) {
> -                       dm_warn("Device tree error at offset %d\n", offset);
> -                       result = ret;
> -                       break;
> +                       return 0;
>                 }
>
> +               dm_warn("Device tree error at offset %d\n", offset);
> +               return compat_length;
> +       }
> +
> +       /*
> +        * Walk through the compatible string list, attempting to match each
> +        * compatible string in order such that we match in order of priority
> +        * from the first string to the last.
> +        */
> +       for (i = 0; i < compat_length; i += strlen(compat) + 1) {
> +               compat = compat_list + i;
> +               dm_dbg("   - attempt to match compatible string '%s'\n",
> +                      compat);
> +
> +               for (entry = driver; entry != driver + n_ents; entry++) {
> +                       ret = driver_check_compatible(entry->of_match, compat);
> +                       if (!ret)
> +                               break;
> +               }
> +               if (entry == driver + n_ents)
> +                       continue;
> +
>                 dm_dbg("   - found match at '%s'\n", entry->name);
>                 ret = device_bind_with_driver_data(parent, entry, name,
> -                                                  id->data, offset, &dev);
> +                                                  entry->of_match->data,

This uses the data from teh first matching compatible string. The
point of this field is to tell the driver which of its strings
matched. So you might need to put the parameter back to
driver_check_compatible().

> +                                                  offset, &dev);
>                 if (ret == -ENODEV) {
>                         dm_dbg("Driver '%s' refuses to bind\n", entry->name);
>                         continue;
> @@ -188,10 +189,8 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,
>                 break;
>         }
>
> -       if (!found && !result && ret != -ENODEV) {
> -               dm_dbg("No match for node '%s'\n",
> -                      fdt_get_name(blob, offset, NULL));
> -       }
> +       if (!found && !result && ret != -ENODEV)
> +               dm_dbg("No match for node '%s'\n", name);
>
>         return result;
>  }
> --
> 2.9.2
>

Regards,
Simon