Attention is currently required from: Jakub Czapiga, Julius Werner, Martin L Roth.
Maximilian Brune has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81081?usp=email )
Change subject: lib/device_tree: Add some FDT helper functions ......................................................................
Patch Set 14:
(11 comments)
Patchset:
PS13:
BTW can you please fix all the checkpatch errors?
I usually reserve the checkpatch errors for last. You never what have to change in between during the review.
File src/include/device_tree.h:
https://review.coreboot.org/c/coreboot/+/81081/comment/e866936d_a36cdaff : PS13, Line 210: fdt_find_nodes_by_prefix
nit: I would choose a different name because all the other `fdt_find_... […]
Done
File src/lib/device_tree.c:
https://review.coreboot.org/c/coreboot/+/81081/comment/d2641846_7f4b1feb : PS12, Line 200: if (!strncmp(*path, node_name, path_sub_len)) {
Okay, then why not use the stack? These path strings aren't gonna be very long. […]
Isn't that the same as creating variable length arrays? I thought we don't do that anymore in coreboot.
https://review.coreboot.org/c/coreboot/+/81081/comment/43e62c6a_fae74294 : PS12, Line 214: ince address-cells and size-cells are not inherited
I think I will investigate a bit on how other projects handle that. […]
I asked in this thread: https://github.com/devicetree-org/devicetree-specification/issues/72
https://review.coreboot.org/c/coreboot/+/81081/comment/59f5305f_cd4832c5 : PS12, Line 234: (offset - size)
How about calling it `prop_offset` (because that's where the properties start) and the other one `no […]
Done
https://review.coreboot.org/c/coreboot/+/81081/comment/aa909f52_f0d05203 : PS12, Line 235: } while (be32dec(blob + offset) != FDT_TOKEN_END_NODE);
I don't really see how your code does anything different? You're still calling `fdt_next_node_name() […]
Well `fdt_next_node_name` expects a `FDT_TOKEN_BEGIN_NODE`. The current implementation expects a `FDT_TOKEN_END_NODE`. Imagine at the offset where `fdt_next_node_name` checks is something different than both tokens. In the current implementation it will loop one over (since no `FDT_TOKEN_END_NODE` was read) reach `fdt_next_node_name` and then notices there is also no `FDT_TOKEN_BEGIN_NODE`. At this point we know there is something seriously wrong with the FDT and could potentially print it out (which I currently do not do ^^). If we only use `fdt_next_node_name()` we just leave the loop, but don't know if that is because of an error in the FDT or because we just reached the last node. (I think)
Personally I think we should catch the errors that we can and accept those that we cannot. But I can't really be very objective about the readabillity since I am the one that wrote it. So if you think it is really more readable to just use `fdt_next_node_name` and it is worth the tradeoff, I will happily change it.
https://review.coreboot.org/c/coreboot/+/81081/comment/4a88b806_ec52f25c : PS12, Line 337: results[(*count_results)++] = offset;
Maybe make it return the actual count of entries, but only fill as much as fits in the array? Then t […]
Maybe do both? count the results that are actually found and print a warning if there are more than fits the array. But return only the amount of entries written in the results array? On the other hand maybe someone actually wants to find the first nth nodes matching a given prefix instead of all of them. Hmm... I think I will just return the number of entries actually written into results. Seems the easiest option and less error prone option.
File src/lib/device_tree.c:
https://review.coreboot.org/c/coreboot/+/81081/comment/675af2d7_a7d3c962 : PS13, Line 169: return offset; // return offset to reg property
Should this maybe return `count` instead? Otherwise how does the caller know how many addresses it g […]
right
https://review.coreboot.org/c/coreboot/+/81081/comment/e654ea4e_a27b33e7 : PS13, Line 333: && count_results < results_len
Shouldn't this be an `if (... […]
I was just lazy. Changed it.
https://review.coreboot.org/c/coreboot/+/81081/comment/ed22bfb1_632ad9a0 : PS13, Line 342: if (count_results == 0)
nit: This case seems to be redundant with the main return path below.
Done
File tests/lib/device_tree-test.c:
https://review.coreboot.org/c/coreboot/+/81081/comment/15e86d3c_14e49ae4 : PS7, Line 13: // regular FDT from RISC-V QEMU virt target
Yes, of course, in a separate file. […]
I will add separate commit to create the common code. The current patch is already big enough.