[PATCH v4 09/10] tools: spkgimage: add Renesas SPKG format

[Marek Vasut]

On 4/17/23 22:17, Ralph Siemsen wrote:
> On Mon, Apr 17, 2023 at 07:23:46PM +0200, Marek Vasut wrote:
>> On 3/8/23 21:26, Ralph Siemsen wrote:
>>> +            spkgimage.o \
>>
>> Maybe just call the file renesas_spkgimage.o so its clear which 
>> SoC/vendor this file is associtated with.
> 
> Okay, will do.
> 
>>> +static struct spkg_file out_buf;
>>> +
>>> +static uint32_t padding;
>>
>> Is this padding here and the padding in struct config_file below 
>> different padding ? Can we get rid of these static global variables ?
> 
> I will give it a try.
> 
>>
>>> +static int check_range(const char *name, int val, int min, int max)
>>> +{
>>> +    if (val < min) {
>>> +        fprintf(stderr, "Warning: param '%s' adjusted to min %d\n",
>>> +            name, min);
>>> +        val = min;
>>> +    }
>>> +
>>> +    if (val > max) {
>>> +        fprintf(stderr, "Warning: param '%s' adjusted to max %d\n",
>>> +            name, max);
>>> +        val = max;
>>> +    }
>>
>> There is a macro clamp() which implements range limiting .
> 
> Thanks for pointing that out. However I think there is value in the 
> diagnostic print when the value is clamped. Ideally it should help the 
> user to fix their invoking script/binman/etc.
> 
> Of course, I could call clamp() and check if the value differs, but that 
> seems just as complex as the check_range().

ACK

>>> +    while (fgets(line, sizeof(line), fcfg)) {
>>> +        line_num += 1;
>>> +
>>> +        /* Skip blank lines and comments */
>>> +        if (line[0] == '\n' || line[0] == '#')
>>> +            continue;
>>> +
>>> +        /* Strip the trailing newline */
>>> +        len = strlen(line);
>>> +        if (line[len - 1] == '\n')
>>> +            line[--len] = 0;
>>
>> Use len - 1 here too to avoid confusion ?
> 
> Old habit. I always try to update the length in sync with modifying the 
> string. If done as a separate line/statement, it is more likely to be 
> lost during subsequent modifications.
> 
> In this case I do not need "len" at all, so I could just do:

Nice