[flashrom] [PATCH] ICH SPI Preop handling

[Carl-Daniel Hailfinger]

On 16.09.2009 17:49, FENG Yu Ning wrote:
> Carl-Daniel Hailfinger wrote:
>   
>> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
>>
>> Index: flashrom-ich9_multicommand_preop/ichspi.c
>> ===================================================================
>> --- flashrom-ich9_multicommand_preop/ichspi.c   (Revision 725)
>> +++ flashrom-ich9_multicommand_preop/ichspi.c   (Arbeitskopie)
>> @@ -745,18 +745,25 @@
>>  int ich_spi_send_multicommand(struct spi_command *spicommands)
>>  {
>>        int ret = 0;
>> +       int oppos, preoppos;
>>        while ((spicommands->writecnt || spicommands->readcnt) && !ret) {
>> +               /* Will the next loop iteration be taken? */
>> +               if ((spicommands + 1)->writecnt || (spicommands + 1)->readcnt) {
>>     
>
> Are we sure there is a (spicommands + 1)?
>   

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.


>> +                       preoppos = find_preop(curopcodes, spicommands->writearr[0]);
>> +                       oppos = find_opcode(curopcodes, (spicommands + 1)->writearr[0]);
>> +                       /* 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.


>> +                       if ((oppos != -1) && (preoppos != -1) &&
>> +                           (curopcodes->opcode[oppos].atomic == preoppos)) {
>> +                               printf_debug("opcode 0x%02x will be run as PREOP\n",
>> +                                            spicommands->writearr[0]);
>> +                               ret = 0;
>> +                               spicommands++;
>> +                               continue;
>> +                       }
>> +               }
>> +
>>                ret = ich_spi_send_command(spicommands->writecnt, spicommands->readcnt,
>>                                           spicommands->writearr, spicommands->readarr);
>> -               /* This awful hack needs to be smarter.
>> -                */
>> -               if ((ret == SPI_INVALID_OPCODE) &&
>> -                   ((spicommands->writearr[0] == JEDEC_WREN) ||
>> -                    (spicommands->writearr[0] == JEDEC_EWSR))) {
>> -                       printf_debug(" due to SPI master limitation, ignoring"
>> -                                    " and hoping it will be run as PREOP\n");
>> -                       ret = 0;
>> -               }
>>                spicommands++;
>>        }
>>        return ret;
>>
>>     
>
> I will write it this way:
>
> int ich_spi_send_multicommand(struct spi_command *cmd)
> {
>         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?


>         /* 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.


>         for (; cmd->writecnt || cmd->readcnt; cmd++) {
>   

I like the for loop. We should use it for all multicommand functions.


>                 /* if we are sure there is a (cmd + 1) */
>   

Yes.


>                 if ((cmd + 1)->writecnt || (cmd + 1)->readcnt) {
>                         /* my bad to invent this eye-hurting 'curopcodes' */
>                         pre = find_preop (curopcodes,  cmd->writearr[0]);
>                         op  = find_opcode(curopcodes, (cmd + 1)->writearr[0]);
>                         if ((op != -1) && (pre != -1) &&
>                             /* see struct _OPCODE comments, atomic value is error prone.
>   

I did not see any comments there.


>                                I am considering flattening struct OPCODES, so that we can
>                                write curop[i].atomic and curpre[i] instead of this looong
>                                expression */
>                             (curopcodes->opcode[op].atomic-- == pre))
>   

The .atomic-- is a bug because it changes the atomic value.


>                                 continue;
>                 }
>
>                 ret = ich_spi_send_command(cmd->writecnt, cmd->readcnt,
>                                            cmd->writearr, cmd->readarr);
>                 if (ret) break;
>         }
>         return ret;
> }
>
> do you think it is easier to read?
>   

Yes. I'll send a new patch in a few minutes.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/