I wrote:
Are we sure there is a (spicommands + 1)?
Carl-Daniel Hailfinger wrote:
Yes. We use a struct spicommand with readcnt=writecnt=0 as last member of the array, so as long as readcnt||writecnt we know there will be at least one member after this one.
Thanks for the explanation.
- /* Is the current op a preop for the next op? */
s/current op/current byte/ easier to parse?
Maybe s/op/opcode/ which should be a bit more obvious.
Just make the two "op"s different. Not a big deal though.
int ret, op, pre;
op and pre are positions, not opcodes/preopcodes. That's why I called them oppos and preoppos. We could call them opindex and preopindex instead. What do you think?
I admit that my naming is misleading. Both your namings are OK then.
/* better if I could write cmd->wcnt, cmd->rcnt */
rcnt and wcnt are a bit short. We want to make this easy to read+understand. Too short variable names mean new developers have to search for the meaning.
I see.
for (; cmd->writecnt || cmd->readcnt; cmd++) {
I like the for loop. We should use it for all multicommand functions.
great.
/* see struct _OPCODE comments, atomic value is error prone.
I did not see any comments there.
http://www.flashrom.org/trac/flashrom/browser/trunk/ichspi.c#L105 atomic == 0 means no preop, while preop[0] is some op.
(curopcodes->opcode[op].atomic-- == pre))
The .atomic-- is a bug because it changes the atomic value.
Oops. That was stupid. I should have written .atomic - 1
yu ning