[flashrom] [PATCH 3/3] ENE Embedded Debug Interface (EDI) and ENE KB9012 EC internal flash support
Nico Huber
nico.h at gmx.de
Sun Nov 8 15:53:04 CET 2015
Hi Paul,
On 24.10.2015 13:19, Paul Kocialkowski wrote:
> I will probably send v2 right away, feel free to follow up the
> discussion on some of these comments in there, it'll probably be
> easier.
I'll keep some of the general discussion here. Some new comments
will come inline with v2.
>>> + unsigned char buffer[edi_read_buffer_length];
>>> + unsigned int i;
>>> + int rc;
>>> +
>>> + edi_read_cmd((unsigned char *)cmd, address);
>>> +
>>> + memset(buffer, 0, sizeof(buffer));
>> Why?
>
> Same thing here, spi_send_command *should* fill-in the whole buffer,
> but in case it doesn't, I'd rather have known values instead, just in
> case garbage turns out to take one of EDI_READY or EDI_NOT_READY.
>
> This is probably not a hard requirement either, but seems like a good
> programming habit to produce reliable code to me.
What bothers me is that you imply that spi_send_command() may behave
wrong. That is very defensive programming. And if spi_send_command()
really does it wrong, we'd never know.
Well, that's generally why I prefer to never initialize unless it's
necessary. So many errors out there that we can't see due to over
initialization :-/
>>> +
>>> + do {
>>> + rc = edi_read(flash, ENE_XBI_EFDAT, buf);
>>> + if (!rc)
>>> + break;
>> if (!timeout) return ...? or loop for ever!
>
> Thanks, sorry for such carelessness, there are other places where
> timeout is not checked (even though it's more critical in this spot).
>
>>> +
>>> + /* Just in case. */
>>> + while (edi_spi_busy(flash) && timeout--)
>>> + programmer_delay(10);
>>> + } while (rc);
>> Redundant check (rc can't be zero when we reach it) hides the endless
>> loop. Nice try! hehe
>
> That's correct, I simply prefer to have a dummy condition loosely
> related to the code logic (that is never actually reached) because,
> well, it doesn't hurt and looks better then while (1).
>
> If you believe it obfuscates the code, I'd be fine with removing that
> form.
Yes, please change that. I believe, a constant condition should always
be obvious.
Nico
More information about the flashrom
mailing list