Am 11.06.2011 21:04 schrieb Carl-Daniel Hailfinger:
Am 09.06.2011 06:44 schrieb Stefan Tauner:
On Thu, 09 Jun 2011 00:56:08 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 08.06.2011 04:55 schrieb Stefan Tauner:
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
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.
Admittedly that's my fault because my text wasn't good either and you tried to stay close to it.
New suggested text for ich_spi_send_command (your location):
The maximum data length is identical for the maximum read length and for the maximum write length excluding opcode and address. Opcode and address are stored in separate registers, not in the data registers and are thus not counted towards data length. The only exception applies if the opcode definition (un)intentionally classifies said opcode incorrectly as non-address opcode or vice versa.
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.
Followup, pasted from IRC: Maybe add some short blurb like "max_data_read == max_data_write for all Intel/VIA SPI masters" to run_opcode(). That would be readable and short enough.
Regards, Carl-Daniel