[PATCH 1/2] mtd: bcm47xxpart: move TRX parsing code to separated function

[Marek Vasut]

On 01/10/2017 05:27 PM, Rafał Miłecki wrote:
> On 10 January 2017 at 17:19, Marek Vasut <marek.vasut at gmail.com> wrote:
>> On 01/10/2017 12:50 PM, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal at milecki.pl>
>>>
>>> This change simplifies main parsing loop logic a bit. In future it may
>>> be useful for moving TRX support to separated module / parser (if we
>>> implement support for them at some point).
>>> Finally parsing TRX at the end puts us in a better position as we have
>>> better flash layout knowledge. It may be useful e.g. if it appears there
>>> is more than 1 TRX partition.
>>>
>>> Signed-off-by: Rafał Miłecki <rafal at milecki.pl>
>>> ---
>>>  drivers/mtd/bcm47xxpart.c | 124 ++++++++++++++++++++++++++++------------------
>>>  1 file changed, 77 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c
>>> index 283ff7e..d7d1b6e 100644
>>> --- a/drivers/mtd/bcm47xxpart.c
>>> +++ b/drivers/mtd/bcm47xxpart.c
>>> @@ -83,6 +83,70 @@ static const char *bcm47xxpart_trx_data_part_name(struct mtd_info *master,
>>>       return "rootfs";
>>>  }
>>>
>>> +static int bcm47xxpart_parse_trx(struct mtd_info *master,
>>> +                              struct mtd_partition *trx,
>>> +                              struct mtd_partition *parts,
>>> +                              size_t parts_len)
>>> +{
>>> +     struct mtd_partition *part;
>>> +     struct trx_header header;
>>> +     size_t bytes_read;
>>> +     int curr_part = 0;
>>> +     int i, err;
>>> +
>>> +     if (parts_len < 3) {
>>> +             pr_warn("No enough space to add TRX partitions!\n");
>>> +             return -ENOMEM;
>>> +     }
>>> +
>>> +     err = mtd_read(master, trx->offset, sizeof(header), &bytes_read,
>>> +                    (uint8_t *)&header);
>>> +     if (err && !mtd_is_bitflip(err)) {
>>> +             pr_err("mtd_read error while reading TRX header: %d\n", err);
>>> +             return err;
>>> +     }
>>> +
>>> +     i = 0;
>>> +
>>> +     /* We have LZMA loader if offset[2] points to sth */
>>> +     if (header.offset[2]) {
>>> +             part = &parts[curr_part++];
>>> +             part->name = "loader";
>>> +             part->offset = trx->offset + header.offset[i];
>>> +             i++;
>>> +     }
>>> +
>>> +     if (header.offset[i]) {
>>> +             part = &parts[curr_part++];
>>> +             part->name = "linux";
>>> +             part->offset = trx->offset + header.offset[i];
>>> +             i++;
>>> +     }
>>> +
>>> +     if (header.offset[i]) {
>>> +             size_t offset = trx->offset + header.offset[i];
>>> +
>>> +             part = &parts[curr_part++];
>>> +             part->name = bcm47xxpart_trx_data_part_name(master, offset);
>>> +             part->offset = offset;
>>
>> Why don't you still use bcm47xxpart_add_part() here ?
> 
> Good point. I think I got two reasons:
> 1) Making this function more generic to it can be moved to separated
> module/parser as some point (without dependency)
> 2) Sadly I'm not sure how bcm47xxpart_add_part is useful at all. Do
> you think it really simplifies code?

Not really, but at least it's consistent.

-- 
Best regards,
Marek Vasut