[flashrom] [PATCH 09/10] fixup! ichspi.c: refactor filling and reading the fdata/spid registers (was Re: [PATCH 5/8] ichspi.c: refactor filling and reading the fdata/spid registers)

[Stefan Tauner]

On Wed, 29 Jun 2011 15:06:00 -0700
David Hendricks <dhendrix at google.com> wrote:

> 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)

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 <
> 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.

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0009-fixup-ichspi.c-refactor-filling-and-reading-the-fdat.patch
Type: text/x-patch
Size: 1751 bytes
Desc: not available
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20110630/d34ca4da/attachment.patch>