Laurent Gauch wrote: > First, thank you for giving objective comments on the patch. > Much appreciated from you. > (Much better than the "Student code galore" from Peter)
I imagine that Tomek spent a fair bit of time writing up his comments in such a clear way. I did not spend as much time writing my comment, because I don't have that much time to explain why bad code is bad. Let's try to get to the *actual* reason for your patch: > The objective is to provide at middle term a better code and more > comprehensible code for the FT2232.c. .. > Actually the deinit is not implemented and it should be done quickly. You claim to have the patch ready - what you should have done is to have sent the patch already. By sending only a single patch out of a patch set with several related patches you are being extremely unhelpful, because you are refusing anyone else than you to see the full picture. > The deinit should first deinitialize the specific layout (as a > jtagkey_deinit), then disable the MPSSE / bitbang mode for any USB > JTAG based ft2232. Objective is to let see the ft2232 device as if > it was just plugged (after a power-off -> power-on). This is absolutely obvious to anyone with a slight understanding. You really do not need to spend time writing about it. Please just send (small, smart) patches that fix it, ideally while also making the codebase simpler, more elegant and uniform. > Also, you can see the importance of decoupling the : > open / close of the FT2232 handle > init / deinit of the FT2232 device No. > But when we will come with new SWD transport support, the interface > layer ( ft2232.c ) will have to be modified to accept new hardware > as the Amontec JTAGkey-3 coming with the SWD support. Maybe OpenOCD is not the right software for your product. Dunno. In any case, yes, it is also absolutely obvious that SWD is not a great fit for OpenOCD. The correct way to deal with this would be for you to work together with the other (at least two) stakeholders in OpenOCD SWD (Tomek and Simon, sorry if I forgot anyone!) and see how you can contribute to improving OpenOCD infrastructure so that it will be ready for the future. The incorrect way is to try to microoptimize the ft2232 driver for your future products. Maybe that approach is successful for you elsewhere, but I'm not surprised that the OpenOCD community is calling bullshit on the change. > the patch corrects an important memory trouble in the FT2232.c, Which has been shown possible also with another, much simpler, solution. > and prepare the FT2232.c for a better open -> init -> deinit -> close ! > The patch is not only a rewrite, it is a correction. Since you are trying to advocate a largeish change you really must show everyone what your complete plan is. Here's how to successfully and quickly get large changes through: 1. Present high level overview of the idea in clear terms, while doing a bit of education on areas where you know more than most readers 2. Acquire buy-in from stakeholders near same area (hint: not by force) 3. Try to get third parties excited about your idea 4. Present implementation overview 5. Acquire buy-in on implementation 6. Implement, and send patches 7. Watch patches get committed Commit depends heavily on having passed 1-5. You skipped those parts, and only provide limited information when pressed for it by review. It must be the other way around. Trouble with 7 can come as no surprise. //Peter _______________________________________________ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
