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 <[email protected]> > 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 <[email protected]> > --- > 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 <[email protected]> Regards, Carl-Daniel -- http://www.hailfinger.org/ _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
