On Wed, 2009-12-02 at 22:54 +0100, Øyvind Harboe wrote: > >> + > >> +/* The minidriver contains inline versions of JTAG fn's */ > >> +#include "minidriver.h" > >> + > > > > This is bad, as you are creating a new layering violation that will need > > to be removed. You should move this #include to somewhere other than > > what should be our public API, probably inside the C files that need it. > > Presently, this change exposes _more_ internals, not less.
Let me emphasize my point above: remove the #include from jtag.h. Do not expose it outside the JTAG layer. If it is a public API too, then you must rename the inline version and call it from a new wrapper function in jtag/core.c. You are exposing all of the minidriver to the entire system, and that's unacceptable. If this hurts performance, then please show us the numbers here first. Even then, this needs to be done in a way that maintains encapsulation. > I'm not quite sure how you define a layer violation here. How can I tell > that there is a violation happening? jtag.h >>> public APIs for use by external applications minidriver.h >>> internal APIs. These drivers will _always_ be built-in to the tree (for performance reasons). Thus, their interfaces should __never__ be exposed to users through the public JTAG API. minidummy/jtag_interface.h >>> implementation APIs are even more internal, yet your patch will make these available in the public API!! No one should be adding new internals into public headers. If this means we need to create a slew of "<module>_imp.h" files to share bits of the internal "imp"lementation, then so be it. It's worth it, if it means that we can stop expose our internals. Nevertheless, you have to stop and really consider the consequences of stick stuff in headers! Who will see it? Should they?! ;) Create a new function to wrap the inline call if needed, but stop sticking stuff in the wrong place!! I can make our libraries usable for 0.4.0, though the APIs will not be stable for some time yet. Still, it opens the door to writing external applications; that option is closed today, and we are hurting the code by delaying. We'll never find out the problems until we start writing applications that use the code -- such as a test suite. Given these hard constraints, I recommend taking another stab at the patch and finding a way around this. To be honest, this might reflect fundamental design flaws with the minidriver, such that it can only be fixed after some rather drastic re-writing of the code. If the code relies this heavily on these optimizations, it's broken by design. Cheers, --Z _______________________________________________ Openocd-development mailing list [email protected] https://lists.berlios.de/mailman/listinfo/openocd-development
