[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<flashrom at mkarcher.dialup.fu-berlin.de>
>    

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 <c-d.hailfinger.devel.2006 at gmx.net>

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

-- 
http://www.hailfinger.org/