Re: [flashrom] [PATCH] ICH SPI Preop handling

[FENG Yu Ning]

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)?
> 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?
> 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;
        /* better if I could write cmd->wcnt, cmd->rcnt */
        for (; cmd->writecnt || cmd->readcnt; cmd++) {
                /* if we are sure there is a (cmd + 1) */
                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 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))
                                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?

yu ning