Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38209 )
Change subject: raiden_debug: Upstream ChromiumOS servo debug board prog ......................................................................
Patch Set 3:
(7 comments)
Patch Set 3:
(5 comments)
I like the USB code and its potential to unify programmer parameters. It belongs into a separate patch, though, to ease review.
Ah I squashed it in so this is a functional bit of code upon it's own merits. Maybe we can separate out in the final round? I would like to address all the higher level rubrics required to get a patch of this nature moved from the cros/flashrom tree into the upstream code. license/style/etc as I predicted there was going to be a lot of back and forth to get it into what it needs to be like?
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c@35 PS3, Line 35: * Software Foundation.
I guess one could infer GPLv3 compatibility from the 3-clause above. But […]
That is a complex question that I would need to ask legal about? The implementation herein was stripped out of the cros/flashrom fork and isn't just something I wrote vanilla with this header. I'll talk to Stefan for guidance here if you think this blocks getting this merged?
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c@154 PS3, Line 154: * maximum USB packet size for the device.
NB. Writing and reviewing such comments takes much more time than fixing the […]
I am not the author and I don't have enough control myself to change all the servo's firmwares to make this perfect from the getgo.. I simply don't think fixing interfaces is attainable in the initial support patch but maybe as a longer term goal.
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c@157 PS3, Line 157: command header maximum size
That's kind of true, but highly confusing when one knows what the macros […]
Sorry I wasn't clear on the expectation on what you wish me to change here if anything?
https://review.coreboot.org/c/flashrom/+/38209/3/raiden_debug_spi.c@238 PS3, Line 238: )
(void)
Done
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.h File usb_device.h:
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.h@64 PS3, Line 64: return
I agree, this macro is a blocker. Hidden control flow statements are not […]
I brought up the same point in the cros/flashrom tree. I very much agree. Can it be it's own patch though or do I need to fix this first in our tree then rebase this patch from our tree again? It's hard to juggle two trees when my goal is to make one go away in the end.
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.c File usb_device.c:
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.c@35 PS3, Line 35: * Software Foundation.
See `raiden_debug_spi.c`.
ditto, see there.
https://review.coreboot.org/c/flashrom/+/38209/3/usb_device.c@86 PS3, Line 86: void usb_match_value_default
Why are all externally visible functions undocumented (here, not in the . […]
I am aware, however this is the situation in the cros/flashrom tree. If I can get the functional differences sorted I can address these things as follow ups. However doing them together adds more diff and is too much up front work in one go to make that goal attainable since there is so many cases such as these.