Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34689 )
Change subject: use JEDEC_SE as the default sector erase opcode for ICH southbridge ......................................................................
Patch Set 1:
TLRD; I don't think this is needed.
Please look into old patches more carefully, especially if their commit message doesn't state what the change is about. Generally, I wouldn't reject this change per se. But it would need a commit message that makes sense in the history of the branch it is pushed to.
In this case, it seems the original problem was already solved in different ways upstream. So a commit on either side (upstream/ downstream) should state that its for alignment of the branches. e.g.
ichspi: Replace default JEDEC_BE_D8 with JEDEC_SE This aligns the upstream master branch with chromium's. Both branches support on-the-fly opcode reprogramming by now, so it shouldn't matter which opcode is the default.Or the other way around for a commit to the downstream branch.
NB. In my limited experience with porting things over the years over from chromium to upstream, it almost never made sense to take the downstream commits as is because of the loss of context. Unless the commits introduced completely new code, of course. It might be better to pick topics, gather the code changes and write new commits. Though, even that can be slower compared to a clean reimplementation of the topic for upstream.
Hi Nico,
Just so I understand you correctly and I can help Mayur with his patch here.. The two issues here are that the commit message is essentially obsolete for the context of today and this change isn't 'strictly' needed because both trees support on-the-fly opcode reprogramming? However, it should be harmless for the purposes of resync the two trees and making the comment make a bit more sense with the define name? Thus, provided the commit message is something aliken your suggestion then this commit is still alright to go forward with?
Yes, you can go forward. The change really seems like a no-op to me.
However, people can fail, and upstream has very limited reviewing capacities. So generally, I would prefer a downstream change in such cases.