Am Samstag, den 17.12.2011, 15:23 +0100 schrieb Carl-Daniel Hailfinger:
This depends on [PATCH] Add struct flashctx * everywhere
But not on the revision you posted on December 16, 14:41 CET.
A patch that is rebased to that revision and fixes a compiler warning (which gets fatal due to -Werror) is attached.
- mainboard native SPI (ICH SPI, SB600 SPI, VIA SPI)
ICH7 SPI works for reading. Currently I don't have no hardware at hand to test other stuff.
int spi_send_command(struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr) {
- if (!spi_programmer->command) {
- if (!flash->pgm->spi.command) { msg_perr("%s called, but SPI is unsupported on this " "hardware. Please report a bug at " "flashrom@flashrom.org\n", __func__); return 1; }
I suggest to drop the NULL pointer check here, and just do it once in register_spi_programmer (refusing to call register_programmer if any function pointer is NULL) Same for the other functions in here.
-static const struct par_programmer par_programmer_none = {
.chip_readb = noop_chip_readb,.chip_readw = fallback_chip_readw,.chip_readl = fallback_chip_readl,.chip_readn = fallback_chip_readn,.chip_writeb = noop_chip_writeb,.chip_writew = fallback_chip_writew,.chip_writel = fallback_chip_writel,.chip_writen = fallback_chip_writen,-};
I suggest to drop noop_chip_readb/noop_chip_writeb at the same time as we drop par_programmer_none, as these functions are no longer used.
+enum chipbustype get_buses_supported(void) +{
- int i;
- enum chipbustype ret = BUS_NONE;
- for (i = 0; i < registered_programmer_count; i++)
ret |= registered_programmers[i].buses_supported;- return ret;
}
Looking at this function, just keep in mind it returns a list of busses supported by all the programmers found.
+#if 0 // Does not really make sense anymore if we use a programmer-centric walk. msg_gspew("Probing for %s %s, %d kB: skipped. ", flash->vendor, flash->name, flash->total_size);
tmp = flashbuses_to_text(buses_supported);
tmp = flashbuses_to_text(get_buses_supported()); msg_gspew("Host bus type %s ", tmp);
If we reinstate this code (i.e. decide to not #if 0 it), don't use get_buses_supported(), but pgm->buses_supported. Also don't call it "Host bus type", but "programmer bus type"
--- flashrom-register_all_programmers_register_generic/ogp_spi.c (Revision 1473) +++ flashrom-register_all_programmers_register_generic/ogp_spi.c (Arbeitskopie) @@ -91,6 +91,8 @@ .get_miso = ogp_bitbang_get_miso, .request_bus = ogp_request_spibus, .release_bus = ogp_release_spibus,
- /* no delay for now. */
- .half_period = 0,
Is this comment really useful? Opposed to where it was used before, in this place, the name of the setting "half_period" is explicitly mentioned.
- tempstr = flashbuses_to_text(get_buses_supported()); msg_pdbg("This programmer supports the following protocols: %s.\n", tempstr);
It's definitely not "this programmer", as should be obvious if you "kept in mind" as I told you that get_buses_supportet returns the union of all programmers. So either move this into the loop over all the programmers and print programmer specific bus types, or call it something like "The programmers in this systems provide the following protocols: %s.\n"
- /* 1 usec halfperiod delay for now. */
- .half_period = 1,
Same comment as above. If you want to have the unit visible, rename the variable to half_period_usecs, instead of adding comments of questionable information content.
No ack yet because of the non-appliacation of your patch and the use of get_buses_supported where a specific programmer bus list would make more sense. The "superflous comment" issue, the "NULL pointer on each spi call" issue, and the "orphaned noop" issue are not something that blocks acking the patch.
Regards, Michael Karcher