Code review comment for lp://staging/~qzhang/lava-dispatcher/add-mx53-android

Revision history for this message
Paul Larson (pwlars) wrote :

25 + # Original android sdcard layout
26 + part_no = client.device_option("android_org_sys_part")
27 + org_sys_part = part_no if part_no else 2
28 + part_no = client.device_option("android_org_cache_part")
29 + org_cache_part = part_no if part_no else 3
30 + part_no = client.device_option("android_org_data_part")
31 + org_data_part = part_no if part_no else 5
32 + # Sdcard layout in Lava image
33 + part_no = client.device_option("android_lava_sys_part")
34 + lava_sys_part = part_no if part_no else 5
Since we are making conf options out of the partitions, it might be best to have these configed as defaults rather than hardcoding them in the source

170 +android_org_boot_part = 2
171 +android_org_sys_part = 3
172 +android_org_cache_part = 5
173 +android_org_data_part = 6
174 +android_org_sdcard_part = 7
175 +android_lava_sys_part = 6
176 +android_lava_sdcard_part = 7
177 +boot_cmds_android = mmc init,
While your at it, for the mx53 as well as the one in default, these should have comments and/or better names. These are confusing to me. For instance, what's the difference between android_org_sys_part and android_lava_sys_part?

51 + client.run_cmd_master('sed -i "s/mmcblk0p%s/mmcblk0p%s/g" init.rc'
52 + % (org_sys_part, lava_sys_part))
ah, I see, it looks like the android_org_* is what you are changing it *from* in the image. I'm not sure this belongs in a config option in that case. This config is describing things that are specific to this board. However, you are talking about a string that is specific to the image and has nothing to do with the board itself right?

review: Needs Fixing

« Back to merge proposal