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 <[email protected]> > > 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 _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
