Re: [flashrom] [PATCH 04/12] use the max_data_read field of the new spi_programmer struct to simplify run_opcode.

[Carl-Daniel Hailfinger]

Am 08.06.2011 04:55 schrieb Stefan Tauner:
> Signed-off-by: Stefan Tauner
>
> 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?
> if (spi_programmer->type == SPI_CONTROLLER_NONE) {
> msg_perr("%s: unsupported chipset\n", __func__);
> return -1;
> }
> if (datalength > maxlength) {
> msg_perr("%s: Internal command size error for "
> 	"opcode 0x%02x, got datalength=%i, want <=%i\n",
> 	__func__, op.opcode, datalength, maxlength);
> return SPI_INVALID_LENGTH;
> }
> switch (spi_programmer->type) {
>  case SPI_CONTROLLER_VIA:
> if (datalength > 16) {
> 	msg_perr("%s: Internal command size error for "
> 		"opcode 0x%02x, got datalength=%i, want <=16\n",
> 		__func__, op.opcode, datalength);
> 	return SPI_INVALID_LENGTH;
> }
> return ich7_run_opcode(op, offset, datalength, data, 16);
> case SPI_CONTROLLER_ICH7:
> if (datalength > 64) {
> 	msg_perr("%s: Internal command size error for "
> 		"opcode 0x%02x, got datalength=%i, want <=16\n",
> 		__func__, op.opcode, datalength);
> 	return SPI_INVALID_LENGTH;
> }
> return ich7_run_opcode(op, offset, datalength, data, 64);
> return ich7_run_opcode(op, offset, datalength, data, maxlength);
> case SPI_CONTROLLER_ICH9:
> if (datalength > 64) {
> 	msg_perr("%s: Internal command size error for "
> 		"opcode 0x%02x, got datalength=%i, want <=16\n",
> 		__func__, op.opcode, datalength);
> 	return SPI_INVALID_LENGTH;
> }
> return ich9_run_opcode(op, offset, datalength, data);
> default:
> msg_perr("%s: unsupported chipset\n", __func__);
> /* If we ever get here, something really weird happened */
> return -1;
> }
> /* If we ever get here, something really weird happened */
> return -1;
>
> }
>
> static int ich_spi_send_command(unsigned int writecnt, unsigned int readcnt,

Apart from the comment issue (which can be fixed without resubmitting),
this is
Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel