Code review comment for lp:~shawnguo/linaro-image-tools/add-dtb

 review: approve

On Tue, Apr 12, 2011, Shawn Guo wrote:
> I just updated the branch with your comments addressed. Can you
> please start anther round of review and let me know if you have
> further comments.

 Yup; in the future, please try to commit separate changes separately.

 In _get_kflavor_files, I'd use two different names for cls.dtb_name and
 the result of _get_file_matching; the latter is a pathname while the
 former is just the last part of the pathname.

 But there is an actually even nicer change you could do: include the
 dtb_name in the regexp:
 DT_GLOB = 'dt-*-%(kernel_flavor)s/%(dtb_name)'

 dregex = DT_GLOB % {'kernel_flavor': flavor, 'dtb_name': cls.dtb_name}

 You could add a comment in the function to state that the dtb is
 optional, or better: expand the inline doc to state that dtb might be
 None in the output:
- """Search for kernel, initrd and dtb in path."""
+ """Search for kernel, initrd and optional dtb in path."""

 Don't set cls.dtb_name = cls.dtb_addr in this function; we don't change
 class variables down there, and the values you set should already be
 set if the definition is correct.

 I don't understand the layout though: is it supposed to be
 /boot/dt-something-2.6.38-1/mx51-babbage.dtb?

 What are the possible values for "something" above?

 Are these files provided directly by the kernel package?

 In make_dtb(), I don't really like the name of the file, but I can't
 think of a better filename. :-/

 There are a couple gratuitous changes in the diff, but they seem
 harmless:
 * make_boot_files(): you move the _get_kflavor_files call one line up,
   doesn't seem to matter
 * in test_igep, switching from board_configs['igep'] to
   boards.IgepConfig
 these don't matter to me either way.

--
Loïc Minier