[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