On Sat, 2009-11-14 at 08:22 +0000, Andrew Rogers wrote:
> SWD is not finished yet, a long way to go. Adding the actual hardware
> drivers is a almost the last thing on my list. I want to get the
> framework in place first.
>
> What I have done is to get a SWD interface listed when you use 'openocd
> -c interface_list'. There's a new struct (interface_base_t) added that
> holds the interface type and name. The global variable
> 'chosen_interface' is type cast to 'jtag_interface_t *' or
> 'swd_interface_t *' as needed.
>
> A new function called 'interface_init' is added that calls either
> 'jtag_interface_init' or 'swd_interface_init' as determined from the
> 'type' element in 'chosen_interface'.
>
> I would really appreciate some constructive criticism on the code or the
> patches. I'm new to git and to working with a community so I expect a
> few teething probs.
It's looking good, but there are a number of things to do, presented in
order of expected difficulty:
Easy: Remove all instances of 'typedef struct' from the code. You
should refer to 'foo_t' as 'struct foo'.
Medium: Post one patch per message to make review easier. I strongly
suggest learning how to use 'git format-patch' and 'git send-email'.
Start out by sending the work-in-progress series to yourself to review.
I usually see problems with my patches when I do this.
Hard: Split the patches so that each one introduces a single change.
Assuming that nothing else was wrong, your first patch might deserve to
be split as following to provide "one change per patch":
- add struct interface_base
- add struct swd_interface
- add swd_interfaces global
- add rlink_interface_swd
- in tcl.c, list swd_interfaces in interface_list command
Expert: You should not be putting any SWD support in src/jtag. I think
we should plan on creating src/{jtag,swd,rs232,i2c,spi}/, and so on.
It might be better to plan ahead and put all of these in src/iface/.
Others maintainers should have opinions on this point, so you should
avoid spending any energy making these changes anytime soon. We can get
this setup in the tree, so your patches can just fall into place without
having to move these mountains. In that regard, your present path was
good because it helped give context required to see this step is needed.
> How about some '#if BUILD_RLINK_SWD == 1' and '#if BUILD_SWD == 1' so
> that SWD can be conditionally compiled?
I think the core SWD or JTAG features should be included automatically
when an adapter that supports it is enabled. In your case, BUILD_RLINK
should be enough to enable the SWD features automatically. The
directory structure suggested above will help with this too.
It looks like you're making progress, but the above issues should be
addressed before any of these changes hit the tree.
Cheers,
Zach
_______________________________________________
Openocd-development mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/openocd-development
