Re: [flashrom] PONY_SPI, now with Windows support

[Michael Karcher]

On Sun, 2012-04-29 at 18:35 +0200, Virgil-Adrian Teaca wrote:
> This PONY_SPI patch have also the Windows support, but, to note that
> this new code is untested.

Thank you very much for sharing your work!

It might make sense to put the serial port pin manipulation functions
into serial.c instead of pony_spi.c (of course they can't be static
then, should be named sp_xxx instead of pony_xxx and the PIN_xxx
enumeration should be named "SERIAL_PIN_xxx")
Comments from other developers?

I will start with some coding style issues, but please see the code
comments below, too.

I can't provide you any indent command line for flashrom, but we
generally don't use spaces on the inner sider of parentheses. So inside
flashrom:
WRONG: ioctl( sp_fd, TIOCMGET, &ctl );
RIGHT: ioctl(sp_fd, TICMGET, &ctl);
On the other hand, we *do* write a space before the opening parenthesis,
but only after keywords (if, while, for), not after function names, so
for code in flashrom:
WRONG: if( pin == PIN_TXD )
RIGHT: if (pin == PIN_TXD)

Furthermore, I noticed you are using a lot of blank lines. I appreciate
structuring your code by inserting blank lines between logical blocks to
make it more readable, but having every second line blank (like you do
in the programmer detection loop for example) does not really add any
value over not having blank lines. In that specific example, the only
blank line I might leave in is the one before the "if" block.
> @@ -122,6 +122,11 @@
>  else
>  override CONFIG_RAYER_SPI = no
>  endif
> +ifeq ($(CONFIG_PONY_SPI), yes)
> +UNSUPPORTED_FEATURES += CONFIG_PONY_SPI=yes
> +else
> +override CONFIG_PONY_SPI = no
> +endif
>  ifeq ($(CONFIG_NIC3COM), yes)
>  UNSUPPORTED_FEATURES += CONFIG_NIC3COM=yes
>  else
> @@ -246,6 +251,11 @@
>  else
>  override CONFIG_RAYER_SPI = no
>  endif
> +ifeq ($(CONFIG_PONY_SPI), yes)
> +UNSUPPORTED_FEATURES += CONFIG_PONY_SPI=yes
> +else
> +override CONFIG_PONY_SPI = no
> +endif
>  ifeq ($(CONFIG_ATAHPT), yes)
>  UNSUPPORTED_FEATURES += CONFIG_ATAHPT=yes
>  else

Drop these two hunks. They are for programmers that directly access
hardware through processor I/O cycles. You don't do that, as you use the
OS driver for the serial port instead.
> +ifeq ($(CONFIG_PONY_SPI), yes)
> +FEATURE_CFLAGS += -D'CONFIG_PONY_SPI=1'
> +PROGRAMMER_OBJS += pony_spi.o
> +endif

Add a "NEED_SERIAL := yes" in the if block, otherwise serial.c might not
get included.
> +#include <stdlib.h>
> +#include <string.h>
> +#include <termios.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <sys/ioctl.h>

The last six headers are not needed on Windows, and some of them don't
even exist. Wrap these includes by "#ifndef _WIN32"
> +#ifdef _WIN32
> +#include <conio.h>
> +#endif

That include is useless, drop it.
> +enum pony_pin {
>   PIN_CD = 0,
>   PIN_RXD,
>   PIN_TXD,
>   PIN_DTR,
>   PIN_GND,
>   PIN_DSR,
>   PIN_RTS,
>   PIN_CTS,
>   PIN_RI,
>
> +};

I wonder whether it makes sense to define all 9 pins of the socket. For
example: What are you intending to use "PIN_GND" for? In the case, you
just define them to have all pins defined, I would suggest to start with
PIN_CD at 1, so the enumerators match the conventional pin numbers.
> +static void pony_set_pin( int pin, int val ) {

Why don't you use "enum pony_pin" as type for the pin?
> +static int pony_get_pin( int pin ) {

dito.

One might even think about using two different enums, one for input
pins, one for output pins.
> +static void pony_bitbang_set_cs( int val )
> +{
>   if( pony_type == TYPE_SI_PROG )
>   {
>           /* CS# is negated by the SI-Prog programmer. */
>           val =  val ? 0 : 1;

please write this as "val = !val;" or "val ^= 1;"
>   }
>   pony_set_pin( PIN_TXD, val );
>
> +}
> +static int pony_bitbang_get_miso(void)
> +{
>   int tmp;
>   tmp = pony_get_pin( PIN_CTS );
>   if( pony_type == TYPE_SERBANG )
>   {
>           /* MISO is negated by the SERBANG programmer. */
>           tmp = tmp ? 0 : 1;

dito, use "tmp = !tmp;" here, or do it like

/* MISO is negated by the SERBANG programmer, */
    if (pony_type == TYPE_SERBANG)
    	return !tmp;
    else
    	return tmp;
>   /* Register the shutdown callback. */
>   if ( register_shutdown( serialport_shutdown, NULL ) ) {
>           return 1;
>   }

If you would register the shutdown function directly after opening the
port, you would not have to call serialport_shutdown(NULL) yourself in
the error cases before this line.
>   /* Get the initial value before writing to any line. */

Remove this comment. It doesn't apply.
>   if ( bitbang_spi_init( &bitbang_spi_master_pony ) ) {
>           return 1;
>   }
>   return 0;
>
> +}
> diff -urN flashrom.orig/print.c flashrom/print.c
> --- flashrom.orig/print.c       2012-03-13 16:52:50.000000000 +0100
> +++ flashrom/print.c    2012-04-24 00:50:46.329483950 +0200
> @@ -507,6 +507,12 @@
>         /* FIXME */
>         msg_ginfo("RayeR parallel port programmer\n");
>  #endif
> +#if CONFIG_PONY_SPI == 1
>   msg_ginfo("\nSupported devices for the %s programmer:\n",
>          programmer_table[PROGRAMMER_PONY_SPI].name);
>   /* FIXME */
>   msg_ginfo("SI-Prog serial port programmer\n");

Add serbang here, too.

Regards,
  Michael Karcher