[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)
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Fri Jul 1 00:00:58 CEST 2011
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:
>>
>>
>>> + 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...
>
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...
> 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.
>> 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?
> please see 9/10 and 10/10 for my proposed fixes of this (and other
> problems like the possibility of setting SME )
>
Oh well, I should probably have looked at those first.
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
More information about the flashrom
mailing list