[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
