Re: [flashrom] [PATCH 1/3] ichspi.c: refactor filling and reading the fdata/spid registers

[Carl-Daniel Hailfinger]

Am 13.07.2011 15:33 schrieb Stefan Tauner:
> i have added a variable to hold (i % 4) in ich_fill_data:
> unsigned int bite; /* offset of the current byte within a 32b word */
>
> dunno if that makes it more or less readable? should i drop it again?

Hm. Maybe "octet" would be a better name than "bite". Or maybe "offs" or
"offset". Then again, the readability improvements are not really huge,
so keeping (i % 4) is also an option.
> besides that i think there is not much room left for improvement.
>
> and i got rid of the return values (hwseq needs a fixup to work with
> this).

Thanks!
> From: Stefan Tauner 
> Date: Tue, 28 Jun 2011 05:15:17 +0200
> Subject: [PATCH 1/3] ichspi.c: refactor filling and reading the fdata/spid registers
> add ich_fill_data to fill the chipset registers from an array
> add ich_read_data to copy the data from the chipset register into an array
> replace the existing code with calls to those functions
> Signed-off-by: Stefan Tauner
>
> ichspi.c |  121 +++++++++++++++++++++++++++++---------------------------------
>  1 files changed, 57 insertions(+), 64 deletions(-)
>
> diff --git a/ichspi.c b/ichspi.c
> index 99c4613..fb4184c 100644
> --- a/ichspi.c
> +++ b/ichspi.c
> @@ -592,6 +592,54 @@ static void ich_set_bbar(uint32_t min_addr)
>    	msg_perr("Setting BBAR failed!\n");
>  }
>
> +/* Reads len bytes from the fdata/spid register into the data array.

s/Reads/Read/
> Note that using len > spi_programmer->max_data_read will return garbage or
> may even crash.
> */
> static void ich_read_data(uint8_t *data, int len, int reg0_off)
> {
> int i;
> uint32_t temp32 = 0;
> for (i = 0; i < len; i++) {
> if ((i % 4) == 0)
> 	temp32 = REGREAD32(reg0_off + i);
> data[i] = (temp32 >> ((i % 4) * 8)) & 0xff;
> }
>
> +}
>
> +/* Fills len bytes from the data array into the fdata/spid registers.

s/Fills/Fill/
> Note that using len > spi_programmer->max_data_write will trash the registers
> following the data registers.
> */
>
> +static void ich_fill_data(const uint8_t *data, int len, int reg0_off)
> +{
> uint32_t temp32 = 0;
> int i;
> unsigned int bite; /* offset of the current byte within a 32b word */

If you indeed keep the variable, please use this comment (or something
similar) instead:
/* Byte offset in the current little-endian 32bit register. */
> if (len <= 0)
> return;
> for (i = 0; i < len; i++) {
> bite = (i % 4);
> if (bite == 0)
> 	temp32 = 0;
> temp32 |= ((uint32_t) data[i]) << (bite * 8);
> if (bite == 3) /* last byte in this 32b word */

Can you use the following comment instead?
/* 32 bits are full, write them to registers. */
> 	REGWRITE32(reg0_off + (i - bite), temp32);
> }
> i--;
> bite = (i % 4);
> if (bite != 3) /* if last byte is not on a 32b boundary write it here */

Can you use the following comment instead?
/* Write remaining data to registers. */
> REGWRITE32(reg0_off + (i - bite), temp32);
>
> +}
>
> /* This function generates OPCODES from or programs OPCODES to ICH according to
> the chipset's SPI configuration lock.

Thanks for the cleanup!

Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel