Grant Likely wrote:
On 8/23/07, Jerry Van Baren gerald.vanbaren@smiths-aerospace.com wrote:
The first half of Grant's comment was:
These 4 functions are pretty close to identical (except for the parameter to cpu_to_be32()). Surely there is a more compact way to do this.
My answer is "no, the whole reason there is one line different is because it *has to be*." As the French say about the "Y" chromosome "...and viva la difference." My first attempt, and Bartlomiej's proposed patch, tried (tries) to put the value in the table, but it is a *WORSE* solution because you have the problem with byteswapping or not and different sizes of values. The scorecard for putting the values in the tables and also having it maintainable is 0 for 2.
Yes, this is the icky bit; but I still argue that there is a more compact way to do it. Table driven is nice when it works; but (as I also mentioned in my original comment) whie it doesn't its probably better to just fall back to something programatic
ie (totally untested):
static int fixup_prop(void *fdt, char *node, char *prop, void *val, int size) { int nodeoffset; int rc;
nodeoffset = fdt_find_node_by_path(fdt, node); if (nodeoffset < 0) { debug("Couldn't find %s: %s\n", node, fdt_strerror(nodeoffset)); return nodeoffset; } rc = fdt_get_property(fdt, nodeoffset, prop, 0) if (rc) goto err; rc = fdt_setprop(fdt, nodeoffset, prop, val, size); if (rc) goto err;
goto ret;
err: debug("Problem setting %s = %s: %s\n", node, prop, fdt_strerror(rc));
ret: // hey, we *CAN* program FORTRAN in C ;-)
return rc;}
This is too simplistic for the known cases. * You pass in a size, but can only handle integers (rc is int). If the size is 6 bytes (MAC), Bad Things[tm] will happen. If it is 1 or 2 bytes, it probably will work, but is unclean type-wise.
* The rules are different for some properties: some are set-always and some are set only if it already exists (e.g. local-mac-address).
By the time you write enough "general" functions to cover all the cases currently covered, I suspect you will have the same number of functions as the current implementation. If not, we have too many "setter" functions in the current implementation and they should be coalesced. :-)
It is possible that I _did_ write more "setter" functions than necessary. I plan to look at it, but have been busy. :-/ Adding LIBFDT support to other CPUs and factoring out the common routines would help with a review.
static int fixup_int_prop(void *fdt, char *node, char *prop, u32 val) { val = cpu_to_be32(val); return ft_fixup_int_prop(fdt, node, prop, &val, sizeof(val)); }
void ft_cpu_setup(void *blob, bd_t *bd) { fixup_int_prop(blob, "/cpus/" OF_CPU, "timebase-frequency", OF_TBCLK); fixup_int_prop(blob, "/cpus/" OF_CPU, "bus-frequency", bd->bi_busfreq); fixup_int_prop(blob, "/cpus/" OF_CPU, "clock-frequency", bd->bi_intfreq); fixup_int_prop(blob, "/cpus/" OF_CPU, "bus-frequency", bd->ipbfreq); fixup_prop(blob, "/" OF_SOC "/ethernet@3000", "mac-address", bd->bi_enetaddr, 6); fixup_prop(blob, "/" OF_SOC "/ethernet@3000", "local-mac-address", bd->bi_enetaddr, 6); }
This adds 2 helper functions instead of 5, and one of them is the same path for all of the fixups. Drops the table approach entirely due to the problems with extracting values out of bd. (Which is what I meant in the third part of my original comment)
I think 2 is too optimistic, but it is still possible this approach would be better than the current 5. The above looks better, but I claim only because it is oversimplified - I contend the 2 helper functions will expand to 5 functions that basically are the current "setter" functions. On the other hand, maybe not. It is worth trying.
I thought a table approach would be more obvious than the #ifdef spaghetti that was in the original code, but I'm not all that wild about the resulting code. In my implementation, with the table and the "setter" functions separated, it still results in obfuscation, especially where table entries and corresponding "setter" functions are #ifdefed. :-( Note that the above proposal doesn't help a lot because the routine calling the new helper function and the helper functions are still separated. :-/
Part of what I'm unhappy about is that I ended up with two sets of #ifdefs where there is conditional code: one in the table and one for the corresponding "setter" function. The above proposal may help, but only if the "helper" functions are truly generalized and I am somewhat skeptical about 100% success.
The 2 new helpers could also be generalized for use by all boards.
s/2/n/ (where n is probably 5) and I would agree 100%. :-/
Cheers, g.
Best regards, gvb