On Friday, June 17, 2011 16:09:47 Jie Zhang wrote: > On Fri, Jun 17, 2011 at 1:58 PM, Mike Frysinger 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 problem is that people wont notice them. no one is going to read through
the ADI ICE cable driver looking for util funcs that are applicable to them.
they will however look at the urjtag headers to see what is already done for
them to leverage.
> > the "p" arg needs to be constified
> >
> >> + hex_file = fopen (filename, "r");
> >
> > probably just paranoid, but should use "rbe"
>
> Why?
"b" is needed for proper operation in Windows, and "e" is to keep fd's leaking
into forked processes
> >> + 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.
in what cases ? ice_update_firmware() is only called in one place, and that's
in the cable's init func. if that fails, the init func should fail. and no
errors get discarded at that point.
which also points out that the current init code doesnt check the return value
of ice_update_firmware() ...
> >> + 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.
it's needed in C++ code, but that's not what urjtag is written in :)
> >> + const char *ex_short = "[firmware=FILE]";
> >> + const char *ex_desc = "FILE Upgrade the ICE firmware. See
> >> this
> >
> > page:\n"
> >
> > const char foo[] = "..."
>
> Why?
one less pointer. compare the assembly output:
$ echo 'const char foo[] = "asdf";' | gcc -S -o - -x c -
$ echo 'const char *foo = "asdf";' | gcc -S -o - -x c -
-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
