Am 25.01.2021 um 19:43 schrieb Gerhard Sittig: > On Sun, 2021-01-24 at 19:15 +0100, Helge Kruse wrote: >> I have recently fixed driver bugs for the Kingst LA2016 driver. >> I have created a pull request for you to merge. >> >> https://github.com/sigrokproject/libsigrok/pull/112 > Can you fixup the issues in that series?
I hope so. Can you help me finding what's wrong? > Bring the commit messages in line with the project's usual style. I assume you missing the "kingst-la2016:" in front of the commit message. I will add it. > Fixup comment style. Does this cover the // vs. /* */ comments introduced by me and others (e.g. fastflo)? Or are there any other problems with the comments? > Use the common helper for the trigger packet (if you > happen to change that aspect anyway). I thought the function write_u32le_inc() is a common helper and I very appriciate it. What other function should I use? > Separate the logically different aspects into individual commits. There are two aspects: wrong parameters during setup, wrong alignment while building USB transfers. There are two commits, each for one aspect. What separations would you like to see? > Who else had a look at these changes, or even tested it for > proper operation? Would be nice to increase coverage. The ideas came in cooperation with fastflo. I encourage all readers of this mailing list, who has a Kinst LA2016 device, to test the new driver in the available environment. > The pain that you went through was the result of the original > submitter's" works for me" approach, picking a non-portable > approach in the first place. And if there was a review, it > missed this issue. Would be good to get more eyes on such stuff, > and more hands helping out. I totally agree and hope to get more comments on here. While the changes in the comments will be done it would be good to get reviews of my code changes. I don't think that this would be the final, golden version of the driver. But it should be in improvement since it allows using the sigrok software with Kingst LA-2016 on Windows. Best regards, Helge _______________________________________________ sigrok-devel mailing list sigrok-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/sigrok-devel
