On Mon, 18 Mar 2013 16:51:20 -0700
Simon Glass <s...@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.
- 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 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??
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?
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?
I'm stopping my already brief review here - please clean this up.
Kim
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
