Re: [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
>
> 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