Re: [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
> > 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