[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 Fri, 01 Jul 2011 00:00:58 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Am 30.06.2011 02:13 schrieb Stefan Tauner:
> > On Wed, 29 Jun 2011 15:06:00 -0700
> > David Hendricks <dhendrix at google.com> wrote:
> >   
> >> On Mon, Jun 27, 2011 at 8:33 PM, Stefan Tauner
> >> <stefan.tauner at student.tuwien.ac.at> wrote:
> 
> We don't optimize for compilers.
> The ICH SPI code was donated to flashrom, and I'm happy we got it. The
> authors already rewrote most of it (compared to the initial submission)
> before I merged it, and back then the flashrom source code was not
> exactly a thing of beauty. Since then, quite a few cleanups have
> happened, but people tended to leave ICH SPI alone because the code was
> a bit more complex than the rest of flashrom. ICH SPI has quite a few
> refactoring opportunities: There are various helper functions which
> could be used more than once...

working on it
if you have something in mind specifically, just drop it off in my
corner. ;)

> > but changes should be in their on commit though anyway and so we can
> > postpone it.
> >   
> 
> Yes. However, using the better style for the new code would make it easy
> to just copy it over to the old code in a later commit.

parser exception.
the current patch does move the fill/read loops used equivalently in
ich7_run_opcode and ich9_run_opcode into two functions which are called instead.
much easier for review and testing.
it can then be optimized/de-obfuscated later.

> 
> >> 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 :)
> >   
> 
> Side note: Do we want a release mode Makefile switch which activates all
> warnings under the sun?

i would call it a developing makefile and would love it. the only
danger of such a thing is that you begin to rely on it and stop
thinking for yourself - i am all for it therefore! :)

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