Re: [flashrom] PONY_SPI, now with Windows support

[Michael Karcher]

On Sun, 2012-04-29 at 23:58 +0200, Virgil-Adrian Teaca wrote:
> Then, there is attached a new patch, where code is cleaned and some
> main bitbanging functions are moved to serial.c, to simplify the
> pony_spi.c and to make them available to all possible serial drivers
> which want bitbanging...

Great! The code is now styled and designed as I expect it for flashrom
code, just some minor nitpicks and one detail I missed to mention in the
last review, and is a problem we can't expect you to know. Let's start
with that one:

In programmer.h, we have:

#if CONFIG_OGP_SPI == 1 || CONFIG_NICINTEL_SPI == 1 || CONFIG_RAYER_SPI == 1 || CONFIG_PONY_SPI == 1 || (CONFIG_INTERNAL == 1 && (defined(__i386__) || defined(__x86_64__)))
        SPI_CONTROLLER_BITBANG,
#endif

If the only bitbang programmer enabled is your programmer,
SPI_CONTROLLER_BITBANG doesn't get defined. I hit that problem when I
tried to cross-compile for Windows, as on Windows, we don't support the
internal programmer or PCI-based programmers (which also means direct
port I/O programmers like rayer for now), so your programmer would be
the only one enabled, but is not in the list of programmers tested.

In my oppion, the whole test should be replaced by
    #if CONFIG_BITBANG_SPI == 1
because the Makefile already handles defining CONFIG_BITBANG_SPI the
right way.

[Makefile]
> LIBFLASHROM_OBJS = $(CHIP_OBJS) $(PROGRAMMER_OBJS) $(LIB_OBJS)
> -OBJS = $(CLI_OBJS) $(LIBFLASHROM_OBJS) 
> +OBJS = $(CLI_OBJS) $(LIBFLASHROM_OBJS)

This change is an white-space change (removing a space at end of line.
It should not have been there in the first place, though) which is
unrelated to your patch. If it doesn't take too much hassle for you,
please remove this change from your patch.

[pony_spi.c]
> +static void pony_bitbang_set_cs(int val)
> +{
>   if(pony_type == TYPE_SI_PROG)

Please add a space after "if"
>   if( pony_type == TYPE_SERBANG )

Please add a space after "if" and remove the inner spaces
>   /* The parameter is on format "dev=/dev/device,type=serbang" */
>   arg = extract_programmer_param( "dev" );

No spaces inside the parens, please.
>           } else {
>                   msg_perr("Error: Invalid programmer type specified.\n");
>                   free( arg );
>                   return 1;
>           }
>   }
>   free( arg );
>   msg_pdbg("Using the %s programmer.\n", ((pony_type == TYPE_SI_PROG ) ? "SI-Prog" : "SERBANG"));

In the "free" calls, remove space, too.
Generally, remove the space after "!" for negation.

[programmer.h]
> +enum sp_pin {
>   PIN_CD = 1,
>   PIN_RXD,
>   PIN_TXD,
>   PIN_DTR,
>   PIN_GND,
>   PIN_DSR,
>   PIN_RTS,
>   PIN_CTS,
>   PIN_RI,
>
> +};

Can you rename them to SP_PIN_...? This enum is defined in programmer.h
now, which is included nearly everywhere in flashrom, not only on serial
port related stuff, so limiting to names starting with serialport or SP_
seems appropriate.

[serial.c]
>           ioctl( sp_fd, val ? TIOCSBRK : TIOCCBRK, 0 );

spacing inside parentheses.
>           ioctl( sp_fd, TIOCMGET, &ctl );

dito.
>           if ( val ) {

dito.
>   GetCommModemStatus( sp_fd, &ctl );

dito.
> @@ -212,7 +266,7 @@
>                 }
>                 if (!tmp)
>                         msg_pdbg("Empty write\n");
>           writecnt -= tmp; 
>           writecnt -= tmp;
>           buf += tmp;
>   }

unrelated whitespace change.

Regards,
  Michael Karcher