[flashrom] [PATCH] ICH SPI Preop handling

[FENG Yu Ning]

> 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