Re: [PATCH v2 05/10] test/py: android: Add create_gpt_image() for image creation

[Mattijs Korpershoek <mkorpershoek@kernel.org>]

Hi Simon,

Thank you for the review.

On Thu, Oct 22, 2026 at 01:26, Simon Glass <sjg@chromium.org> wrote:

> Hi Mattijs,
>
> On 2026-10-20T09:25:49, Mattijs Korpershoek <mkorpershoek@kernel.org> wrote:
>> test/py: android: Add create_gpt_image() for image creation
>>
>> There is quite some code duplication between the image creation for
>> mmc7.img and mmc8.img. If we want to add more images, this will be even
>> more duplication.
>>
>> Add a common function, create_gpt_image() which removes all the code
>> duplication.
>>
>> Reviewed-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
>> Tested-by: Guillaume La Roque <glaroque@baylibre.com>
>> Tested-by: Igor Opaniuk <igor.opaniuk@oss.qualcomm.com>
>> Signed-off-by: Mattijs Korpershoek <mkorpershoek@kernel.org>
>> Reviewed-by: Guillaume La Roque <glaroque@baylibre.com>
>>
>>  test/py/tests/test_android/android_image.py | 78 +++++++----------------------
>>  1 file changed, 19 insertions(+), 59 deletions(-)
>
>> diff --git a/test/py/tests/test_android/android_image.py b/test/py/tests/test_android/android_image.py
>> @@ -7,13 +7,12 @@ import os
>> -def setup_android_image(ubman):
>> +Partition = collections.namedtuple('part', 'start,size,name')
>> +
>> +def create_gpt_image(ubman, mmc_dev, required_parts, images):
>>      """Create a 20MB disk image with Android partitions"""
>> -    Partition = collections.namedtuple('part', 'start,size,name')
>>      parts = {}
>>      disk_data = None
>
> This is now a general helper that callers have to fill in, but the
> format of 'required_parts' (a list of dicts with 'num', 'label' and
> 'size', where size is either a sector count or 'NM') and 'images'
> (tuples of partition number, file name and hex data) is only visible
> from reading the body. Please can you add an Args: section to the
> docstring describing each parameter, as set_part_data() has earlier in
> the series? Since the name now says GPT, the docstring could also say
> which mmc image it writes rather than 'Android partitions'.

Ok, will do for v3.

>
> Also, 'disk_data = None' is now redundant since the variable is only
> assigned once, further down. Moving Partition to module level seems
> unnecessary too, since only this function uses it.

Ok, dropped disk_data = None for v3.
Ok, put Partition back into the function for v3.

>
>> diff --git a/test/py/tests/test_android/android_image.py b/test/py/tests/test_android/android_image.py
>> @@ -69,55 +56,28 @@ def setup_android_image(ubman):
>> +def setup_android_image(ubman):
>> +    mmc_dev = 7
>
> This drops the docstring from setup_android_image(), so pylint
> complains at this patch. The next patch adds it back, but it would be
> better to keep it here so that each patch is clean on its own.

Ok, will put the docstring back in in v3.

>
> Regards,
> Simon