[Openocd-development] [PATCH 2/5] ft2232: Refactor ft2232_init_*() into ft2232_initone()
ft2232.c is compiled either for ftd2xx or for libftdi. The two init functions had slightly different prototypes, making it neccessary to distinguish between them not only at declaration, but also when they were being called. Since the code is only compiled for one library type we can homogenize the function parameters and contain the abstraction within a single ft2232_initone() function, which allows removing the CPP conditional in ft2232_init() that determined which function to call. ft2232_init_libftdi() took an extra channel parameter which came from the layout struct. layout is treated as a global variable however, already accessed directly both in ft2232_initone() and ft2232_init(), so the channel parameter is unneccessary, and can be removed without risk for new trouble or limitations. A context parameter that references the layout could be added if and when this code sees more significant cleanup in the future. --- src/jtag/drivers/ft2232.c | 24 +--- 1 files changed, 9 insertions(+), 15 deletions(-) diff --git a/src/jtag/drivers/ft2232.c b/src/jtag/drivers/ft2232.c index 1f0269a..cb50c50 100644 --- a/src/jtag/drivers/ft2232.c +++ b/src/jtag/drivers/ft2232.c @@ -2115,7 +2115,8 @@ static int ft2232_execute_queue(void) } #if BUILD_FT2232_FTD2XX == 1 -static int ft2232_init_ftd2xx(uint16_t vid, uint16_t pid, int more, int* try_more) + +static int ft2232_initone(uint16_t vid, uint16_t pid, int more, int* try_more) { FT_STATUS status; DWORD deviceID; @@ -2290,10 +2291,9 @@ static int ft2232_purge_ftd2xx(void) return ERROR_OK; } -#endif /* BUILD_FT2232_FTD2XX == 1 */ +#elif BUILD_FT2232_LIBFTDI == 1 -#if BUILD_FT2232_LIBFTDI == 1 -static int ft2232_init_libftdi(uint16_t vid, uint16_t pid, int more, int* try_more, int channel) +static int ft2232_initone(uint16_t vid, uint16_t pid, int more, int* try_more) { uint8_t latency_timer; @@ -2309,9 +2309,9 @@ static int ft2232_init_libftdi(uint16_t vid, uint16_t pid, int more, int* try_mo return ERROR_JTAG_INIT_FAILED; /* default to INTERFACE_A */ - if(channel == INTERFACE_ANY) { channel = INTERFACE_A; } + if(layout->channel == INTERFACE_ANY) { layout->channel = INTERFACE_A; } - if (ftdi_set_interface(&ftdic, channel) < 0) + if (ftdi_set_interface(&ftdic, layout->channel) < 0) { LOG_ERROR("unable to select FT2232 channel A: %s", ftdic.error_str); return ERROR_JTAG_INIT_FAILED; @@ -2376,7 +2376,7 @@ static int ft2232_purge_libftdi(void) return ERROR_OK; } -#endif /* BUILD_FT2232_LIBFTDI == 1 */ +#endif static int ft2232_set_data_bits_low_byte( uint8_t value, uint8_t direction ) { @@ -2452,14 +2452,8 @@ static int ft2232_init(void) int more = ft2232_vid[i + 1] || ft2232_pid[i + 1]; int try_more = 0; -#if BUILD_FT2232_FTD2XX == 1 - retval = ft2232_init_ftd2xx(ft2232_vid[i], ft2232_pid[i], - more, &try_more); -#elif BUILD_FT2232_LIBFTDI == 1 - retval = ft2232_init_libftdi(ft2232_vid[i], ft2232_pid[i], -more, &try_more, layout->channel); -#endif - if (retval >= 0) + retval = ft2232_initone(ft2232_vid[i], ft2232_pid[i], more, &try_more); + if (ERROR_OK == retval) break; if (!more || !try_more) return retval; -- 1.7.4.1.343.ga91df.dirty ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH 2/5] ft2232: Refactor ft2232_init_*() into ft2232_initone()
On Thu, Jun 9, 2011 at 12:25 PM, Peter Stuge 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?? How about ft2232_init_{device,interface,ftdilib,hardware,handle,whatever} depending on what the function actually initializes. Another ugliness in my point of view is using the try_more parameter to convey information about the type of success/failure. That's what return values are for. And the more parameter apparently only selects whether the diagnostic should be a warning or an error. Seems the message belongs outside the function, making the parameter useless. This function is supposed to initialize hardware and shouldn't care if the caller wants to retry with another vid/pid or not. I'll put removing those two parameters on my todo-list unless you or someone else pitches in or feels strongly against it. /Andreas ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH 2/5] ft2232: Refactor ft2232_init_*() into ft2232_initone()
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. > 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! //Peter ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH 2/5] ft2232: Refactor ft2232_init_*() into ft2232_initone()
On Fri, Jun 10, 2011 at 12:26 AM, Peter Stuge 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
Re: [Openocd-development] [PATCH 2/5] ft2232: Refactor ft2232_init_*() into ft2232_initone()
On Thu, Jun 9, 2011 at 10:57 PM, Andreas Fritiofson wrote: > 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? The current organization for me is okay - there is a standard ft2232_init() function that call ft2232_init_ftdi() if the program was built with libftdi or ft2232_init_ftd2xx() if the program was built with libftd2xx. There is no need to complicate this behavior as exactly the complication was the problem with previous patches. Please remember guys also to properly fix the ft2232_quit() as Laurent proposed, this is also necessary to bring interface to default state on quit :-) Gotos are okay in presented case where avoiding gotos bring unnecessary complication. Best regards! :-) Tomek -- CeDeROM, SQ7MHZ, http://www.tomek.cedro.info ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH 2/5] ft2232: Refactor ft2232_init_*() into ft2232_initone()
On Thu, Jun 9, 2011 at 10:57 PM, Andreas Fritiofson https://lists.berlios.de/mailman/listinfo/openocd-development>> wrote: >/ 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? / The current organization for me is okay - there is a standard ft2232_init() function that call ft2232_init_ftdi() if the program was built with libftdi or ft2232_init_ftd2xx() if the program was built with libftd2xx. There is no need to complicate this behavior as exactly the complication was the problem with previous patches. Please let //ft2232_init_{libftdi,ftd2xx} . Maybe we will have D2XX open source in next future :-) , and the both libftdi and d2xx could be loaded dynamically. // Please remember guys also to properly fix the ft2232_quit() as Laurent proposed, this is also necessary to bring interface to default state on quit :-) And not only the specific layout, but the mpsse must be reseted. We have to make sure to put the dongle at is initial state as if it was just plugged in the computer. Gotos are okay in presented case where avoiding gotos bring unnecessary complication. Go with goto ... it is only code style :-) Best regards! :-) Tomek Laurent http://www.amontec.com/jtagkey.shtml Amontec JTAGkey-2 making JTAG a snap ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH 2/5] ft2232: Refactor ft2232_init_*() into ft2232_initone()
Am 06/10/2011 09:21 AM, schrieb Laurent Gauch: > Please remember guys also to properly fix the ft2232_quit() as Laurent >> proposed, this is also necessary to bring interface to default state >> on quit :-) >> > And not only the specific layout, but the mpsse must be reseted. > We have to make sure to put the dongle at is initial state as if it > was just plugged in the computer. Um - why? That may remove control over the target, leaving it in an unpredictable state (depending on the circuit in the adapter), or even reset the target (if that is the default of the external circuit). Any code that assumes the dongle is already in a specific state on init is broken and needs to be fixed. Working around that by enforcing a certain state on deinit is plain broken. cu Michael ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development