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 19:
(7 comments)
File src/lib/device_tree.c:
https://review.coreboot.org/c/coreboot/+/81081/comment/9ab06159_956b5431 : PS12, Line 214: ince address-cells and size-cells are not inherited
I mean sounds like they're acknowledging that Linux is intentionally doing something else to improve […]
Done
https://review.coreboot.org/c/coreboot/+/81081/comment/3e9bebc0_040ded3c : PS12, Line 235: } while (be32dec(blob + offset) != FDT_TOKEN_END_NODE);
Yeah, I think keeping the code simple is more important here. […]
Done
File src/lib/device_tree.c:
https://review.coreboot.org/c/coreboot/+/81081/comment/83f7f05f_1f213c58 : PS18, Line 175: ""
Okay, I see. But it sounds like we agree that that doesn't make sense anymore after fdt_find_subnodes_by_prefix() became a separate function? So let's design fdt_find_node() such that it is as equivalent as possible in what it does to dt_find_node(), I think that makes understanding the design much easier.
I agree. Done
Yes, that was my idea, just make it skip the fdt_next_node_name() call. Anyway, I mostly proposed this because it sounded like you were very interested in maximizing performance for this code, but I understand now that you were referring to something else with that. If you don't think a few extra strlen() here and there matter, feel free to ignore those microoptimzations, I don't care that much either way.
Done
https://review.coreboot.org/c/coreboot/+/81081/comment/8d20aa9d_08213b3d : PS18, Line 188: size_t path_count
I think you should match `dt_find_node()` with this API for consistency (e.g. […]
Done
https://review.coreboot.org/c/coreboot/+/81081/comment/f4146d0a_6ef31ca2 : PS18, Line 251: #define PATH_ARRAY_MAX 10
I think this should go at the top of the file (maybe call it FDT_PATH_MAX_DEPTH to clarify that it d […]
Done
https://review.coreboot.org/c/coreboot/+/81081/comment/049c686e_9c6f8d0a : PS18, Line 255: 128
This should also be a named constant (e.g. FDT_PATH_MAX_LEN).
Done
https://review.coreboot.org/c/coreboot/+/81081/comment/1d924a6f_83e73a90 : PS18, Line 265: }
I think this would be cleaner with `memcpy()` and `strtok_r()`: […]
Done