Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4:
Is this patch blocking anything important? I see a big patch chain of clean-up patches that have almost all been merged, so it seems progress is being made here. Why can't the common code be merged first with this as part of the chain?
I think this is more about a general problem. Should we have code on our master branch that is neither build tested nor reviewed, makes maintenance harder and let's the community pay for some nonsense? (refactored, put into common code or not, this code will likely never be useful for Picasso) It shouldn't have been merged (I think I even pointed that out while the review was skipped), now it drags coreboot down; and it's protected just to satisfy some ego (I doubt it can be anything else, because it's a five minute job to add it back in case it'd really be needed).
Patch Set 4: Especially if the blocker is CB:33772, a commit that has not been touched in over a month and has never managed to build successfully.
That's misleading considering the time of year.
Marshall, as per the Gerrit guidelines, [1] I have to ask that you please remove your -2.
What part of that are you citing? Marshall has outlined the plan for resolving this and +2'd CB:38222. In his -2 he made a reasonable suggestion to merge the common code first before dropping this code, as to avoid breaking platform development that is underway.
I wouldn't call any suggestion reasonable that can't be done immediately. And nothing can be done immediately if the code isn't even hooked up for build testing. If there is nothing that can be done better, how can a -2 be warranted?
The guidelines also say "Don’t submit patches that you know will break other platforms", though it's murky in this case since Picasso appears to be broken in its current state. If it were orphaned or obsolete hardware then the answer could simply be to remove it entirely. But since it's being actively developed, that is another matter.
Well, I trust that it's actively developed. But for whom? AFAIK, it's still not clear if compatible binaries will ever be available for the coreboot community... [1] Same community that already pays for the maintenance.
While we're citing guidelines, the ad hominem attacks used in some of the later comments definitely fails to meet the threshold for respectful conduct.
Cleaning up APIs is certainly necessary and Kyösti's efforts here are appreciated. AFAICT nobody is against this patch, the only resistance is due to timing of platform development currently underway.
Timing is reversed here. The longer the code is hanging around untested on the master branch, the more problems will sneak in. It might even safe Marshall some time if we'd kill it earlier... (applies to the whole picasso/ code btw. IMHO, it should be removed and be added back once it can be build tested).
[1] https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot.org/thread/PHSD2...