[M] Change in flashrom[master]: tree/: Move programmer_delay() out of programmer state machine
Attention is currently required from: Felix Singer, Nico Huber.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67393 )
Change subject: tree/: Move programmer_delay() out of programmer state machine ......................................................................
Patch Set 4:
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/67393/comment/973a6532_9fa9229f PS4, Line 13:
I'm missing the reason for the change. Especially because some […]
I am sorry I cannot follow your mind absolutely here as it goes off on too many speculative directions however I will have a crack at it;
From what I could understand from the above is the following;
* Reason for the change - make flashrom reentrant on the `programmer` handle because `programmer_delay()` does not need to be a closure upon `programmer` anymore. * Something regressed but it is unclear what you are actually stating is regressing? * Programmers that have multiple masters need redundant code but this seems speculative as that obviously isn't happening here. You seem to be alluding to dummyflasher as perhaps a example or ichspi as perhaps another.
Next paragraph I could not parse, clearly `programmer` global problem got fixed in the context of `programmer_delay()` here. I guess this paragraph is perhaps speculating beyond the scope of this patch?
Final paragraph seems to be the same theme of talking well beyond the scope of the patch to resolve design defects at the libflashrom entry-point and how it relates to core flashrom structures that carry the programmer state. While co-related to this patch certainly, that topic is vastly beyond the scope of resolving the very specific sub-problem of `programmer_delay()` in the overall `programmer` singleton pattern issue. Thus, probably more a suited as a topic for a bug and/or mailing list post if you see another control flow structure that can be derived.
By looking at this patch and the subsequent patches you will notice the underlying theme of using a declarative+procedural mindset over a OOP one in addressing the issue and ultimately that would be my interpretation of how to resolve the above issue.
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/67393/comment/7ee2a7b2_0da90163 PS4, Line 224: flash->mst->par.delay(usecs);
Actually, if there will be common information to store, we could […]
This did occur to me however given the current structure this isn't easy as `&mst` isn't so trivially accessible within the context of a driver implementation as drivers register into `&mst->{par,spi,opaque}`. Therefore it was more pragmatic to go in this direction first instead of trying to force the code into ivory towers of structures that currently are ill suited to the current control flow even if they do indeed conceptually make more sense.
-
Edward O'Callaghan (Code Review)