[flashrom] [PATCH 3/3] serprog: add support for setting the SPI frequency

[Uwe Hermann]

On Thu, Sep 08, 2011 at 12:56:23AM +0200, Stefan Tauner wrote:
> +0x14	Set SPI clock frequency		16-bit requested frequency	ACK + 16-bit set frequency / NAK
                                ^
                 "Set SPI clock frequency (in Hz)"


> +	0x14 (S_SPI_SCK):

Please rename to S_SPI_FREQ (also all other places with 'SCK'). SCK is
often used to name a clock pin, this is unnecessarily confusing here.


> +		Set the SPI clock frequency.
                                           ^
                                         in Hz


> +		f_spi = extract_programmer_param("sck");

sck -> freq


> +		if (f_spi && strlen(f_spi)) {
> +			uint8_t buf[2];
> +			uint16_t tmp = atoi(f_spi);

Hm, maybe use strtol() later and check for errors, atoi() is not so
good for that.


> +			if (sp_check_commandavail(S_CMD_S_SPI_SCK) == 0)
> +				msg_pdbg(MSGHEADER "Setting SPI clock rate is "
> +					 "not supported!\n");
> +			else if (sp_docommand(S_CMD_S_SPI_SCK, 2, buf, 2, buf)
> +				 == 0) {
> +				tmp = buf[0] | (buf[1]<<8);
                                                      ^^
                                               Missing spaces.


> +				msg_pdbg(MSGHEADER "Requested to set SPI clock "
> +					 "frequency to %s kHz. It was actually "
> +					 "set to %d kHz\n", f_spi, tmp);

kHz -> Hz

I would highly recommend to use Hz as the unit for setting the frequency
(in the API). The frontend (flashrom) can easily allow freq=1000,
or freq=1khz, or freq=1mhz etc. for user friendlyness, but the API
should maintain max. flexibility IMHO.


Uwe.
-- 
http://hermann-uwe.de     | http://sigrok.org
http://randomprojects.org | http://unmaintained-free-software.org