On Fri, Jun 10, 2011 at 12:26 AM, Peter Stuge <pe...@stuge.se> wrote:
> Andreas Fritiofson wrote:
> > > + retval = ft2232_initone(ft2232_vid[i], ft2232_pid[i],
> more,
> > > &try_more);
> > >
> >
> > This is a good start, but what's with the function name??
>
> Short. And one from one device. As opposed to try to init *all*
> supported devices, which is what ft2232_init does.
>
ft2232_init really refers to init of the openocd driver, while
ft2232_init_{libftdi,ftd2xx} refers to init of the ftdi subsystem including
hardware (using either of the libraries). Different inits, hence "one" and
"all" is not the suitable distinction. How about ft2232_init_ftdi that could
be interpreted as initializing the ftdi library, the ftdi chip or the ftdi
handle, all of which are true?
>
> > Another ugliness in my point of view is using the try_more parameter
>
> Oh for sure. It was like that when I came here. Maybe small steps is
> a good idea. Please do send more patches to do further cleanup.
>
> > Seems the message belongs outside the function, making the
> > parameter useless.
>
> Note that I know nothing of the code style in OpenOCD. At least the
> ft2232 driver seems to consistently output error messages at the
> lowest level in the call stack, as opposed to letting the highest
> level (user interface) determine how the error should be handled. I
> found this quite awkward, but I wanted to make changes that were
> already some improvements over what was there. I am not saying the
> patches make things perfect, there's certainly still more to do!
>
The last paragraph was not really a comment on this patch, which is good to
go if the name is changed, but rather suggestions for further improvements
in the related parts of the code.
/Andreas
_______________________________________________
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development
