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<[email protected]>
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 <[email protected]>
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/
_______________________________________________
flashrom mailing list
[email protected]
http://www.flashrom.org/mailman/listinfo/flashrom
