On Wed, 29 Jun 2011 15:06:00 -0700 David Hendricks <[email protected]> wrote:
> A couple quick comments in-line below... > > On Mon, Jun 27, 2011 at 8:33 PM, Stefan Tauner < > [email protected]> wrote: > > > > > + static void ich_read_data(uint8_t *data, uint8_t len, int reg0_off) > > + { > > + int a; > > + uint32_t temp32 = 0; > > + > > + if (len > spi_programmer->max_data_read) > > + len = spi_programmer->max_data_read; > > + > > + for (a = 0; a < len; a++) { > > + if ((a % 4) == 0) > > + temp32 = REGREAD32(reg0_off + (a)); > > + > > + data[a] = (temp32 & (((uint32_t) 0xff) << ((a % 4) * 8))) > > + >> ((a % 4) * 8); > > > > How about "data[a] = (temp32 >> ((a % 4) * 8)) & 0xff" instead? That is > clearer IMHO, and you won't even need to break the line. (80 columns really > *is* enough :-P) i just copied the existing code from swseq and did not change it at all on purpose. improvements are welcome... the code certainly is not very readable. i wonder if this is due to some compiler optimization friendly style... but changes should be in their on commit though anyway and so we can postpone it. > On Mon, Jun 27, 2011 at 8:33 PM, Stefan Tauner < > [email protected]> wrote: > > > +static uint8_t ich_fill_data(const uint8_t *data, uint8_t len, int > > reg0_off) > > > As mentioned on IRC, the caller tends to pass in a "len" value > 255, e.g. 1 > block of 4096 bytes or more. Since len is handled as an int in the caller, > it should probably get passed in as an int here. inderdaad. i would have expected the compiler to warn in such cases. obviously i am too used to my usual embedded makefile with ~20 -W terms :) please see 9/10 and 10/10 for my proposed fixes of this (and other problems like the possibility of setting SME ) -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner
>From 35a72b40185c75818cee8d98de2c6e2e44509e69 Mon Sep 17 00:00:00 2001 From: Stefan Tauner <[email protected]> Date: Thu, 30 Jun 2011 02:02:32 +0200 Subject: [PATCH 09/10] fixup! ichspi.c: refactor filling and reading the fdata/spid registers Signed-off-by: Stefan Tauner <[email protected]> --- ichspi.c | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-) diff --git a/ichspi.c b/ichspi.c index fd3b808..8f5c0b5 100644 --- a/ichspi.c +++ b/ichspi.c @@ -605,7 +605,7 @@ static void ich_set_bbar(uint32_t min_addr) /* Reads up to len byte from the fdata/spid register into the data array. * The amount actually read is limited by the maximum read size of the * chipset. */ - static void ich_read_data(uint8_t *data, uint8_t len, int reg0_off) + static void ich_read_data(uint8_t *data, int len, int reg0_off) { int a; uint32_t temp32 = 0; @@ -624,9 +624,8 @@ static void ich_set_bbar(uint32_t min_addr) /* Fills up to len bytes from the data array into the fdata/spid registers. * The amount actually written is limited by the maximum write size of the - * chipset and is returned by the function on success. Negative values indicate - * an error. */ -static uint8_t ich_fill_data(const uint8_t *data, uint8_t len, int reg0_off) + * chipset and is returned by the function. */ +static uint8_t ich_fill_data(const uint8_t *data, int len, int reg0_off) { uint32_t temp32 = 0; int a; @@ -634,6 +633,9 @@ static uint8_t ich_fill_data(const uint8_t *data, uint8_t len, int reg0_off) if (len > spi_programmer->max_data_write) len = spi_programmer->max_data_write; + if (len <= 0) + return 0; + for (a = 0; a < len; a++) { if ((a % 4) == 0) { temp32 = 0; -- 1.7.1
_______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
