Re: [flashrom] [PATCH 4/8] ichspi: improve prettyprint_opcodes

[Carl-Daniel Hailfinger]

Am 20.08.2011 12:39 schrieb Stefan Tauner:
> add headers for the columns and some decoding into human readable format.
>
> before:
> […]
> Reading OPCODES... done
> preop0=0x06, preop1=0x50
> op[0]=0x02, 3, 0
> op[1]=0x03, 2, 0
> op[2]=0x20, 3, 0
> op[3]=0x05, 0, 0
> op[4]=0x9f, 0, 1
> op[5]=0x20, 1, 2
> op[6]=0x01, 1, 0
> op[7]=0x06, 0, 0
>
> SPI Read Configuration: prefetching disabled, caching enabled, OK.
> This chipset supports the following protocols: FWH, SPI.
> […]
>
> after:
> […]
> Reading OPCODES... done
>         OP        Type      Pre-OP
> op[0]: 0x02, write w/  addr, none
> op[1]: 0x03, read  w/  addr, none
> op[2]: 0x20, write w/  addr, none
> op[3]: 0x05, read  w/o addr, none
> op[4]: 0x9f, read  w/o addr, none
> op[5]: 0x20, write w/o addr,  1
> op[6]: 0x01, write w/o addr,  2
> op[7]: 0x06, read  w/o addr, none
> Pre-OP 0: 0x06, Pre-OP 1: 0x50
>
> SPI Read Configuration: prefetching disabled, caching enabled, OK.

Can you kill the empty line above?
Besides that, the whole stuff should be dbg2 instead of dbg. My hope is
to have a usable progress printing (which is done with dbg) without
flooding the user with useless (for him) messages before.
> This chipset supports the following protocols: FWH, SPI.
> […]
>
> i could also print the preops directly in the preop column instead of
> using that redirection. but i think as debug output it is better to
> know the index of the two preops too.
>
> side note: does it make sense to create an opcode -> string decoder
> for flashrom-wide usage in user output (in spi25.c)?
> an erase opcode -> function pointer decoder is used in sfdp, that could
> also be generalized like that.

Yes, that may make sense. How do you handle conflicting definitions for
opcodes? That's my biggest worry with such a decoder.
> Signed-off-by: Stefan Tauner

Acked-by: Carl-Daniel Hailfinger 
with the comments above.

Regards,
Carl-Daniel

[PATCH 4/8] ichspi: improve prettyprint_opcodes

[Stefan Tauner]

On Sat, 17 Sep 2011 16:06:45 +0200
Carl-Daniel Hailfinger  wrote:
> Am 20.08.2011 12:39 schrieb Stefan Tauner:
> > add headers for the columns and some decoding into human readable format.
> >
> > before:
> > […]
> > Reading OPCODES... done
> > preop0=0x06, preop1=0x50
> > op[0]=0x02, 3, 0
> > op[1]=0x03, 2, 0
> > op[2]=0x20, 3, 0
> > op[3]=0x05, 0, 0
> > op[4]=0x9f, 0, 1
> > op[5]=0x20, 1, 2
> > op[6]=0x01, 1, 0
> > op[7]=0x06, 0, 0
> >
> > SPI Read Configuration: prefetching disabled, caching enabled, OK.
> > This chipset supports the following protocols: FWH, SPI.
> > […]
> >
> > after:
> > […]
> > Reading OPCODES... done
> >         OP        Type      Pre-OP
> > op[0]: 0x02, write w/  addr, none
> > op[1]: 0x03, read  w/  addr, none
> > op[2]: 0x20, write w/  addr, none
> > op[3]: 0x05, read  w/o addr, none
> > op[4]: 0x9f, read  w/o addr, none
> > op[5]: 0x20, write w/o addr,  1
> > op[6]: 0x01, write w/o addr,  2
> > op[7]: 0x06, read  w/o addr, none
> > Pre-OP 0: 0x06, Pre-OP 1: 0x50
> >
> > SPI Read Configuration: prefetching disabled, caching enabled, OK.
>
> Can you kill the empty line above?

i have removed msg_pdbg("\n"); in ich_init_opcodes, although i like it
because it introduced a bit of structure. having less lines does not
make output more comprehensible automatically.
> Besides that, the whole stuff should be dbg2 instead of dbg. My hope is
> to have a usable progress printing (which is done with dbg) without
> flooding the user with useless (for him) messages before.

done.
hm... i have not thought about that in detail, so maybe it is a stupid
idea... but progress printing should actually be a cli switch/library
setting/parameter that is independent from the verbosity of other
messages.
> > This chipset supports the following protocols: FWH, SPI.
> > […]
> >
> > i could also print the preops directly in the preop column instead of
> > using that redirection. but i think as debug output it is better to
> > know the index of the two preops too.
> >
> > side note: does it make sense to create an opcode -> string decoder
> > for flashrom-wide usage in user output (in spi25.c)?
> > an erase opcode -> function pointer decoder is used in sfdp, that could
> > also be generalized like that.
>
> Yes, that may make sense. How do you handle conflicting definitions for
> opcodes? That's my biggest worry with such a decoder.

there are various options... printing all of them, printing the raw
value instead, something even more sophisticated...
first we need a list, then we can decide what's the best solution.
> > Signed-off-by: Stefan Tauner
>
> Acked-by: Carl-Daniel Hailfinger 
> with the comments above.

thanks, committed in r1444.