> +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".
the "p" arg needs to be constified
> + hex_file = fopen (filename, "r");
probably just paranoid, but should use "rbe"
> + urj_log (URJ_LOG_LEVEL_ERROR, _("Open file %s failed\n"), filename);
should use IO level so the errno is included in the output ?
> + q = (struct flash_block *) malloc (sizeof (struct flash_block));
no need for the cast, and i'd use sizeof (*q)
> + q->data = (uint8_t *) malloc (byte_count);
no need for the cast with any alloc func
> + 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.
the error string should probably use the same OOM error as the rest of urjtag
(in the future we probably should unify this somewhere ...)
> +/* 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
> "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",
> + if (cable_params->firmware_filename)
> + free (cable_params->firmware_filename);
no need for the if statement. free(NULL) works fine.
> + const char *ex_short = "[firmware=FILE]";
> + const char *ex_desc = "FILE Upgrade the ICE firmware. See this
page:\n"
const char foo[] = "..."
> +"<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
-mike
signature.asc
Description: This is a digitally signed message part.
------------------------------------------------------------------------------ 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
