Re: [PATCH v2 2/7] fdt: Add function to allow aliases to refer to multiple nodes

Hi Stephen,

On Thu, Jan 19, 2012 at 12:49 PM, Stephen Warren [off-list ref] wrote:

On 01/12/2012 12:00 PM, Simon Glass wrote:

quoted

Some devices can deal with multiple compatible properties. The devices
need to know which nodes to bind to which features. For example an
I2C driver which supports two different controller types will want to
know which type it is dealing with in each case.

The new fdtdec_add_aliases_for_id() function deals with this by allowing
the driver to search for additional compatible nodes for a different ID.
It can then detect the new ones and perform appropriate processing.

Another option considered was to return a tuple (node offset, compat id)
and have the function be passed a list of compatible IDs. This is more
overhead for the common case though. We may add such a function later if
more drivers in U-Boot require it.

quoted

+int fdtdec_add_aliases_for_id(const void *blob, const char *name,
+                     enum fdt_compat_id id, int *node_list, int maxcount)

...

quoted

@@ -232,11 +238,13 @@ int fdtdec_find_aliases_for_id(const void *blob, const char *name,

               * it as done.
               */
              if (fdtdec_get_is_enabled(blob, node)) {
+                     if (node_list[number])
+                             continue;
                      node_list[number] = node;
                      if (number >= num_found)
                              num_found = number + 1;
              }
-             nodes[j] = 0;
+             nodes[found] = 0;
      }

      /* Add any nodes not mentioned by an alias */

Aren't those two changes really bug-fixes to the underlying patch rather
than part of this enhancement?

Clean-ups maybe. The first change can change behaviour when someone
has two aliases the same. The second is equivalent (j == found) but
nicer I think.

I'm not convinced this patch is correct in all cases. It'll probably
work fine for simply device-trees though. Consider the following case:

4 device nodes
A: compat=i2c alias for usb0
B: compat=i2c no alias
C: compat=i2c alias for usb2
D: compat=i2cx alias for usb1

First, we search for all compat=i2c, the list comes back:

0=A
1=B
2=C

Then we search for all compat=i2cx, the list comes back:

-1
-1
-1
D

So D's alias of 1 isn't honored even though if both IDs were searched
for together, it would have been.

Do we really want that behaviour? Overall I think it is a bit odd to
have devices with no aliases if you actually care about the ordering.
At least in U-Boot ordering is important. The simple fix above is to
provide an alias for everything, which is what we do. If we do want
the behaviour you suggest then I will need the change to doing it in
one pass.

Also, what about the scenario where a driver searches for both
nvidia,tegra20-i2c then later nvidia,tegra30-i2c, given that all the DT
nodes are probably compatible with both?

If all DT notes are compatible with both, why are you searching for
one and then the other? Why not just search for one?

It sounds like a decision here as to whether we want a one-pass
algorithm with a list of compatible nodes, or the current additive
priorty-based approach.

Regards,
Simon

--
nvpublic