[flashrom] [PATCH 1/3] ichspi.c: refactor filling and reading the fdata/spid registers
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Wed Jul 13 23:33:04 CEST 2011
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/
More information about the flashrom
mailing list