Hi Kim,
On Mon, Apr 1, 2013 at 5:13 PM, Kim Phillips kim.phillips@freescale.com wrote:
On Mon, 18 Mar 2013 16:51:20 -0700 Simon Glass sjg@chromium.org wrote:
I have received a number of off-list comments - please do copy the list when replying so that everyone can see your comments.
I don't have time to fully review 45 patches, let alone the subject matter (e.g., no support for RSA in h/w, eh? ;), but I did notice some things that bugged me:
Re: "[PATCH v2 04/45] libfdt: Add fdt_next_subnode() to permit easy subnode iteration":
- Where's our libfdt maintainer? libfdt patches should be submitted
to the dtc project first. It appears "[PATCH v2 40/45] libfdt: Add fdt_find_regions()" also suffers from this problem.
The fdt_next_subnode() is a pretty trivial change. I will submit it to the dtc project.
The fdt_find_regions() was submitted to dtc as part of the dtc fdtgrep series. I modified the form of it in response to review feedback and that is ongoing.
- can we improve on the readability of that for loop - the improved
one the patch gives us is this:
for (ndepth = 0, noffset = fdt_next_subnode(fit, image_noffset, &ndepth); noffset >= 0; noffset = fdt_next_subnode(fit, noffset, &ndepth)) {
whereas perhaps something like this:
ndepth = 0; noffset = fdt_next_subnode(fit, image_noffset, &ndepth); while (noffset >= 0) { // body noffset = fdt_next_subnode(fit, noffset, &ndepth); }
would be much easier to quickly visually parse?
This suffers from the problem that a 'continue' will not work as expected. I think the existing for() loop is a little better overall - everything is in one place.
this would also affect:
Re: "[PATCH v2 12/45] image: Move hash checking into its own function":
- which also adds lines like these:
*err_msgp = " error!\nCan't get hash algo ""property";upon which a subsequent patch ("[PATCH v2 13/45] image: Move error! string to common place", if I'm not mistaken) corrects. Are you intentionally trying to make people review the same code twice??
No, what is happening here is that the first patch moves the code into a function, and the second patch removes the 'error' part of the strings, not just from that moved code but from everywhere. I find that doing two things at once is harder to review.
I am certainly not added 'error' to the strings in one patch and taking it away in the next :-)
Re: "[PATCH v2 07/45] image: Split FIT code into new image-fit.c"
- after the code is split, it appears the rest of the patchseries
works on improving image-fit.c instead of updating equivalent code still existing in image.c, IIRC, "[PATCH v2 13/45] image: Move error! string to common place"
- I also found it odd that git format-patch -M on this doesn't
produce a shorter patch with "% equivalent" statistics: what else changed?
Are you suggesting that more patches should affect image.c? If so, please let me know what should specifically you want done and I will take a look. Or perhaps we can improve it once the dust clears.
Re: "[PATCH v2 22/45] env: Fix minor comment typos in cmd_nvedit":
- I personally appreciate it when authors of such large patchseries
put the trivial stuff first, as if to follow the logical progression of the complexity (and therefore acceptability) of the patches in the series. Make things like this the first patch in the series - that way maintainers can be tempted to start applying obvious patches from early patchseries versions. This also applies to patches like [PATCH v2 24/45] Revert "fdt- Tell the FDT library where the device tree is", and:
Re: "[PATCH v2 25/45] Add stdarg to vsprintf.h":
- which contains the text "TODO: Probably should drop this patch?".
Well? Did you mean this series to be an RFC?
Yes I am going to split this up into 3-4 series. The first one (sandbox changes) was sent earlier today. The next one will focus on the initial improvements to the image code, and will include these trivial patches - I will ensure that they are at the start - I agree it is better that way.
The series was originally RFC, but in version 2 I removed that tag. Part 25 was added to deal with HOSTCC building vsprintf.h, but I subsequently found that we could avoid that. I will drop it from v3.
I'm stopping my already brief review here - please clean this up.
Thanks for looking at this.
Kim
Regards, Simon