Code review comment for lp:~berolinux/linaro-image-tools/android-iMX53

On Tue, 2011-07-26 at 19:24 +0000, Bernhard Rosenkraenzer wrote:
[...]
> >> @@ -158,6 +151,9 @@
> >> def populate_raw_partition(cls, media, boot_dir):
> >> super(AndroidBoardConfig, cls).populate_raw_partition(boot_dir, media)
> >>
> >> + @classmethod
> >> + def install_boot_loader(cls, boot_partition, boot_device_or_file):
> >> + pass
> >
> > Do we need a new classmethod for this? ISTM that
> > populate_raw_partition(), which is already called in l-a-m-c, should be
> > the one used to populate a raw partition with the board-specific
> > bootloader instead, no?
>
> Probably populate_raw_partition can be adapted to do the right thing instead.

Cool, would you give that a try?

>
> >> --- linaro_image_tools/media_create/partitions.py 2011-06-29 09:16:39 +0000
> >> +++ linaro_image_tools/media_create/partitions.py 2011-07-25 00:21:08 +0000
> >> @@ -337,8 +337,14 @@
> >> media.path, 1 + board_config.mmc_part_offset)
> >> system_partition = _get_device_file_for_partition_number(
> >> media.path, 2 + board_config.mmc_part_offset)
> >> - cache_partition = _get_device_file_for_partition_number(
> >> - media.path, 3 + board_config.mmc_part_offset)
> >> + if board_config.mmc_part_offset != 1:
> >> + cache_partition = _get_device_file_for_partition_number(
> >> + media.path, 3 + board_config.mmc_part_offset)
> >> + else:
> >> + # In the current setup, partition 4 is always the
> >> + # extended partition container, so we need to skip 4
> >> + cache_partition = _get_device_file_for_partition_number(
> >> + media.path, 5)
> >
> > I'm slightly confused by this change as the mmc_part_offset of
> > Mx53LoCoConfig is still 0, so it won't use the new code (in the else
> > block). Maybe this is not related to the rest of the changes in this
> > branch?
>
> Actually it is related to the rest of the changes and is vital to avoid a crash.
> The patch removes the mmc_part_offset=0 line, and mmc_part_offset is
> set to 1 in Mx5Config (and in turn Mx53Config and Mx53LoCoConfig).

Oh, right, I didn't notice that Mx5Config sets mmc_part_offset to 1 so I
thought it'd have the default value from BoardConfig (which is 0) here.