[flashrom] [PATCH] Enable spi clock setting in dediprog driver

[Nico Huber]

Hello Carl-Daniel,

thank you for your review.

Am 13.06.2012 23:22, schrieb Carl-Daniel Hailfinger:
> Hi Nico,
> 
> thanks for your patch. Review follows.
> 
> 
> Am 13.06.2012 11:01 schrieb Nico Huber:
>> This adds a programmer parameter 'speed' in the dediprog driver to
>> controll the transfer rate on the spi bus. The following rates are
>> available (all in kHz):
>>   24000, 12000, 8000, 3000, 2180, 1500, 750, 375
>>
>>
>> Signed-off-by: Nico Huber <nico.huber at secunet.com>
> 
> Sorry, this patch does not work as intended.
You are right. I can't believe I sent this.

>>  	/* Case 1 and 2 are in weird order. Probably an organically "grown"
>>  	 * interface.
>>  	 * Base frequency is 24000 kHz, divisors are (in order)
>>  	 * 1, 3, 2, 8, 11, 16, 32, 64.
>>  	 */
>> -	switch (speed) {
>> -	case 0x0:
>> -		khz = 24000;
>> +	switch (khz) {
>> +	case 24000:
>> +		speed = 0;
>>  		break;
>> -	case 0x1:
>> -		khz = 8000;
>> +	case 8000:
>> +		khz = 1;
> 
> This looks like an incomplete search/replace operation. I think you
> wanted to set speed instead of khz in all cases below.
> speed = 1;
Admitted. Thank you for pointing that out.

>> +	speed = extract_programmer_param("speed");
>> +	if (speed) {
>> +		khz = strtol(speed, NULL, 0);
> 
> We want a sensible base, and I can't envision anybody wanting base 8 or
> base 16 here.
> strtoul(speed, NULL, 10);
> That said, the Bus Pirate driver offers SPI speed selection as well, but
> the interface differs a bit ("spispeed" instead of "speed", specifying
> frequency e.g as "8M" instead of "8000"). It would be nice if we had a
> consistent interface for this feature.
I hadn't looked at the other programmer drivers. I'll unify this.
How about if I also extract some common code? like:

struct param_value_lut {
	const char *const	name;
	const int		value;
};
int lookup_programmer_param_value(const char *const param_name,
				  const struct param_value_lut *const lut);

which looks up an parameter value for a string (like 8M => 0x7 in
buspirate).

> Another question is whether selecting the speed in one flashrom run will
> have any impact on subsequent flashrom runs. If yes, we should probably
> always set the SPI frequency.
Yes it will affect subsequent runs. I thought it would be better to make
this optional, as I can only test it with one device and can't assure
that it works with other hardware revisions/firmware versions.

Best,
Nico