[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 <stefan.tauner at student.tuwien.ac.at>
> 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 <stefan.tauner at student.tuwien.ac.at>
> ---
>  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 <c-d.hailfinger.devel.2006 at gmx.net>

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/