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

[David Hendricks]

A couple quick comments in-line below...

On Mon, Jun 27, 2011 at 8:33 PM, Stefan Tauner <
[e-mail]> 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)

On Mon, Jun 27, 2011 at 8:33 PM, Stefan Tauner <
[e-mail]> 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.