Re: [flashrom] [PATCH 3/3] kill central list of SPI programmers

[Carl-Daniel Hailfinger]

Am 05.05.2011 03:04 schrieb Michael Karcher:
> Remove the array spi_programmer, replace it by dynamic registration
> instead. Also initially start with no busses supported, and switch to
> the default non-SPI only for the internal programmer.
>
> Signed-off-by: Michael Karcher

Thanks for your patch!
I have a few minor nitpicks and design questions, but the code changes 
seem to be bug-free, so you get an
Acked-by: Carl-Daniel Hailfinger

Your patch is very readable and got me thinking about future directions 
of that code, especially multiple registrations... so take the review 
with a grain of salt.
> diff --git a/dummyflasher.c b/dummyflasher.c
> index c4c9c4e..fdaa5f2 100644
> --- a/dummyflasher.c
> +++ b/dummyflasher.c
> @@ -339,7 +339,7 @@ static int emulate_spi_chip_response(unsigned int writecnt, unsigned int readcnt
>   }
>   #endif
>
> -int dummy_spi_send_command(unsigned int writecnt, unsigned int readcnt,
> +static int dummy_spi_send_command(unsigned int writecnt, unsigned int readcnt,
>    	      const unsigned char *writearr, unsigned char *readarr)
>   {
>    int i;
> @@ -375,12 +375,22 @@ int dummy_spi_send_command(unsigned int writecnt, unsigned int readcnt,
>    return 0;
>   }
>
> -int dummy_spi_write_256(struct flashchip *flash, uint8_t *buf, int start, int len)
> +static int dummy_spi_write_256(struct flashchip *flash, uint8_t *buf, int start, int len)
>   {
>    return spi_write_chunked(flash, buf, start, len,
>    			 spi_write_256_chunksize);
>   }
>
> +static const struct spi_programmer spi_programmer_dummyflasher = {
>    .type = SPI_CONTROLLER_DUMMY,
>    .max_data_read = MAX_DATA_READ_UNLIMITED,
>    .max_data_write = MAX_DATA_UNSPECIFIED,

256 instead?
>    .command = dummy_spi_send_command,
>    .multicommand = default_spi_send_multicommand,
>    .read = default_spi_read,
>    .write_256 = dummy_spi_write_256,

Same comment as for patch 1 of the series. AFAICS dummy_spi_write_256 
should be replaced by the default one.
> +};
> int dummy_init(void)
> {
>  char *bustext = NULL;

Please fix the whitespace (spaces instead of tabs) in it87spi.c and 
dummyflasher.c.
Apparently a newline is missing at the end of spi.c.
> diff --git a/bitbang_spi.c b/bitbang_spi.c
> index be30944..ca28434 100644
> --- a/bitbang_spi.c
> +++ b/bitbang_spi.c
> @@ -81,7 +81,7 @@ static uint8_t bitbang_spi_readwrite_byte(uint8_t val)
>    return ret;
>   }
>
> -int bitbang_spi_send_command(unsigned int writecnt, unsigned int readcnt,
> +static int bitbang_spi_send_command(unsigned int writecnt, unsigned int readcnt,
>    	const unsigned char *writearr, unsigned char *readarr)
>   {
>    int i;
> @@ -106,6 +106,16 @@ int bitbang_spi_send_command(unsigned int writecnt, unsigned int readcnt,
>    return 0;
>   }
>
> +static const struct spi_programmer spi_programmer_bitbang = {

Can you make this one non-const to allow changing .type?
> .type = SPI_CONTROLLER_BITBANG,

And leave type empty, to be filled by bitbang_spi_init()?
> .max_data_read = MAX_DATA_READ_UNLIMITED,
> .max_data_write = MAX_DATA_WRITE_UNLIMITED,
> .command = bitbang_spi_send_command,
> .multicommand = default_spi_send_multicommand,
> .read = default_spi_read,
> .write_256 = default_spi_write_256,
>
> +};
> int bitbang_spi_init(const struct bitbang_spi_master *master, int halfperiod)

Could you extend this function signature to have an additional enum 
spi_controller parameter? If each bitbang-using programmer calls 
bitbang_spi_init(spi_controller, master, halfperiod) we can keep the 
spi_controller enums unchanged for now which would help us keep things 
consistent (for example, ichspi.c uses 3 different spi_controller enums 
for the same functions).
> {
>    /* BITBANG_SPI_INVALID is 0, so if someone forgot to initialize ->type,

Insert in this function:
spi_programmer_bitbang.type=spi_controller;
> diff --git a/programmer.h b/programmer.h
> index fefb9cd..53a50f6 100644
> --- a/programmer.h
> +++ b/programmer.h
> @@ -527,7 +518,6 @@ enum spi_controller {
>    SPI_CONTROLLER_SB600,
>    SPI_CONTROLLER_VIA,
>    SPI_CONTROLLER_WBSIO,
> SPI_CONTROLLER_MCP6X_BITBANG,
> #endif
> #endif
> #if CONFIG_FT2232_SPI == 1
>
> @@ -542,16 +532,9 @@ enum spi_controller {
>   #if CONFIG_DEDIPROG == 1
>    SPI_CONTROLLER_DEDIPROG,
>   #endif
> -#if CONFIG_RAYER_SPI == 1
> SPI_CONTROLLER_RAYER,
>
> -#endif
> -#if CONFIG_NICINTEL_SPI == 1
> SPI_CONTROLLER_NICINTEL,
>
> -#endif
> -#if CONFIG_OGP_SPI == 1
> SPI_CONTROLLER_OGP,
>
> +#if CONFIG_OGP_SPI == 1 || CONFIG_NICINTEL_SPI == 1 || CONFIG_RAYER_SPI == 1 || (CONFIG_INTERNAL == 1&&  (defined(__i386__) || defined(__x86_64__)))
> SPI_CONTROLLER_BITBANG,
> #endif
> SPI_CONTROLLER_INVALID /* This must always be the last entry. */
> };
> extern const int spi_programmer_count;

If you use enum spi_controller as parameter for bitbang_spi_init(), we 
don't need an extra SPI_CONTROLLER_BITBANG and can keep the 
programmer-specific ones.

The calls to bitbang_spi_init() in mcp6x_spi.c, nicintel_spi.c, 
ogp_spi.c and rayer_spi.c would need to be adjusted as well.

I plan to add a union private_data{} to struct spi_programmer to allow 
storing stuff like struct bitbang_spi_master, avoiding the need for 
programmer-internal static configuration variables.

Regards,
Carl-Daniel