On Sat, 20 Apr 2013 16:03:20 -0700 Simon Glass <[email protected]> wrote:
> On Mon, Apr 1, 2013 at 5:13 PM, Kim Phillips <[email protected]> > wrote: > > On Mon, 18 Mar 2013 16:51:20 -0700 > > Simon Glass <[email protected]> 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. Ideally we'd apply the patch directly from how it was applied to the upstream 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. ok. > > - 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. It's not like that can't be fixed - either by manually assigning noffset before it, or by using a goto. > I think the existing for() loop is a little better overall - > everything is in one place. it's ugly and comparatively unreadable, esp. given its alignment and the ndepth initialization... > > 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 :-) it sounds like the 'error' string removal should appear earlier in the patchseries. > > 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. as above, I believe the answer is to make the common changes (to both image.c and image-fit.c) earlier in the patchseries. Thanks, Kim _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
