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

David Hendricks dhendrix at google.com
Thu Jun 30 00:06:00 CEST 2011


A couple quick comments in-line below...

On Mon, Jun 27, 2011 at 8:33 PM, Stefan Tauner <
stefan.tauner at student.tuwien.ac.at> 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 <
stefan.tauner at student.tuwien.ac.at> 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.

-- 
David Hendricks (dhendrix)
Systems Software Engineer, Google Inc.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20110629/0f53fc9b/attachment.html>


More information about the flashrom mailing list