Thanks for your comments!
On Fri, Jun 17, 2011 at 1:58 PM, Mike Frysinger <[email protected]> wrote:
>> +static int get_hex_digit (char *p)
>> +static int get_hex_byte (char *p)
>> +static int get_hex_half_word (char *p)
>
> these look like useful utility funcs that should probably go into common code.
> and rather than use "byte" and "half_word", let's use "u8" and "u16".
>
Can we not do this until there is such requirement from other parts of
UrJTAG? I want to keep as many functions static as possible.
> the "p" arg needs to be constified
>
>> + hex_file = fopen (filename, "r");
>
> probably just paranoid, but should use "rbe"
>
Why?
>> + urj_log (URJ_LOG_LEVEL_ERROR, _("Open file %s failed\n"), filename);
>
> should use IO level so the errno is included in the output ?
>
There is no IO level. You might meant URJ_ERROR_IO. But using
urj_error_set here might not be good since the error will be discarded
deliberately in some cases.
>> + q = (struct flash_block *) malloc (sizeof (struct flash_block));
>
> no need for the cast, and i'd use sizeof (*q)
>
OK. Seems I'm still living in old days.
>> + q->data = (uint8_t *) malloc (byte_count);
>
> no need for the cast with any alloc func
>
OK.
>> + if (!q->data)
>> + {
>> + urj_log (URJ_LOG_LEVEL_ERROR, _("malloc failed.\n"));
>> + return URJ_STATUS_FAIL;
>> + }
>> ...
>> + }
>> + else
>> + {
>> + q->data = (uint8_t *) realloc (q->data, q->bytes + byte_count);
>> + }
>
> your first q->data alloc has error checking, but the 2nd doesnt. i would drop
> the malloc and move the realloc to outside of the if checking. after all,
> realloc(q->data=NULL, x) is the same as malloc(x), so no point in calling both
> yourself.
>
OK.
> the error string should probably use the same OOM error as the rest of urjtag
> (in the future we probably should unify this somewhere ...)
>
As mentioned above, there is no good to use OOM error here.
>> +/* Calculate the CRC using forward CRC-16-CCITT algorithm. */
>> +
>> +static uint16_t ice_calculate_crc (struct flash_block *p)
>
> sounds like a utility func that should be moved to common code. have it take
> a const buffer and a length.
>
>> + memcpy (buffer + 4, (void *) &address, 4);
>> + memcpy (buffer + 8, (void *) &count, 4);
>> + memcpy (buffer + 12, (void *) &crc, 2);
>
> no need for casts
>
OK.
>> "Error: The firmware on the ICE-100B needs to be updated. Please go to:\n"
>> " %sto learn how to update the firmware.\n",
>> "http://docs.blackfin.uclinux.org/doku.php?id=hw:jtag:ice100b\n";
>
> a bit odd to have the URL include the newline. i know this isnt from your
> patch, but i'd fix it now.
> " %s\nto learn how to update the firmware.\n",
>
OK.
>> + if (cable_params->firmware_filename)
>> + free (cable_params->firmware_filename);
>
> no need for the if statement. free(NULL) works fine.
>
OK. Another habit from old days.
>> + const char *ex_short = "[firmware=FILE]";
>> + const char *ex_desc = "FILE Upgrade the ICE firmware. See this
> page:\n"
>
> const char foo[] = "..."
>
Why?
>> +"<https://docs.blackfin.uclinux.org/doku.php?id=hw:jtag:ice100b>\n";
>
> we should probably place this URL into a define at the top of the file to
> avoid copying & pasting it through out
>
Good idea.
Regards,
Jie
------------------------------------------------------------------------------
EditLive Enterprise is the world's most technically advanced content
authoring tool. Experience the power of Track Changes, Inline Image
Editing and ensure content is compliant with Accessibility Checking.
http://p.sf.net/sfu/ephox-dev2dev
_______________________________________________
UrJTAG-development mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/urjtag-development
