Am 09.06.2011 06:44 schrieb Stefan Tauner: > On Thu, 09 Jun 2011 00:56:08 +0200 > Carl-Daniel Hailfinger <[email protected]> wrote: > > >> Am 08.06.2011 04:55 schrieb Stefan Tauner: >> >>> Signed-off-by: Stefan Tauner <[email protected]> >>> --- >>> ichspi.c | 41 +++++++++++++++++------------------------ >>> 1 files changed, 17 insertions(+), 24 deletions(-) >>> >>> diff --git a/ichspi.c b/ichspi.c >>> index 12727d1..dff6f32 100644 >>> --- a/ichspi.c >>> +++ b/ichspi.c >>> @@ -913,37 +913,30 @@ static int ich9_run_opcode(OPCODE op, uint32_t offset, >>> static int run_opcode(OPCODE op, uint32_t offset, >>> uint8_t datalength, uint8_t * data) >>> { >>> + uint8_t maxlength = spi_programmer->max_data_read; >>> >>> >> Mh. I see what you're doing here, and it makes sense, but a comment >> would be appreciated in the code: >> /* The maximum data length is identical on ICH/VIA for the maximum read >> length and for the maximum write length without opcode and address. >> Opcode and address are stored in separate registers, not in the data >> registers. The only exception applies if the opcode definition >> (un)intentionally classifies said opcode incorrectly as non-address >> opcode or vice versa. */ >> >> Any suggestions on how to improve that comment? >> > i am not sure if this is the right place to put the second half of that > comment, because i dont really see the relevance inside that function: > it just delegates its inputs. > OTOH it makes sense to have it at a place where both versions share > code. > what about putting "The maximum payload
I don't really like the word "payload". It has a totally different meaning in the coreboot world. > lengths for read and write > operations without opcode and address are identical on ICH/VIA - using > max_data_read arbitrarily here." That description is incomplete and (for me) slightly confusing. > there and the rest inside > ich_spi_send_command in the "/* translate read/write array/count */" > comment-block? > Excellent idea, that location is where the _whole_ comment should live. It doesn't belong in run_opcode() anyway. Regards, Carl-Daniel -- http://www.hailfinger.org/ _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
