Am 26.02.2012 02:05 schrieb Stefan Tauner:
> This is not tested very thoroughly, but it compiles and writing the dummy works.
> Most of the transformation is quite obvious and painless (well, it was no pleasure,
> so i would rather not rebase this often. :)
> I have introduced dedicated struct flashchip variables where the functions use the
> chip field a lot and some of those (not) introduced variables may be debatable,
> but one gets at least a feeling how the code would look like with this change.
>
> One major FIXME is: the code allocates "the new field" while probing and copies
> the data from the *const* struct flashchip in the flashchips.c array into it.
> We need to free that sometime... it is probably obvious (scope of fill_flash), but i
> have not looked into it yet and this is my reminder.
I have answered that in annotations.
> Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
Good idea. It was too dangerous to do this directly before 0.9.5, but
now that the tree has reopened, I think we can merge such a patch early
in the cycle (now) and have enough time to get it tested.
> diff --git a/cli_classic.c b/cli_classic.c
> index 972043b..4bce7d7 100644
> --- a/cli_classic.c
> +++ b/cli_classic.c
> @@ -166,6 +166,7 @@ int main(int argc, char *argv[])
> const struct flashchip *flash;
> struct flashctx flashes[3];
> struct flashctx *fill_flash;
> + const struct flashchip *chip;
> const char *name;
> int namelen, opt, i, j;
> int startchip = -1, chipcount = 0, option_index = 0, force = 0;
> @@ -441,11 +442,13 @@ int main(int argc, char *argv[])
> }
> }
>
> + fill_flash = &flashes[0];
Can you move this assignment back to where it was?
> + chip = fill_flash->chip;
And move this assignment together with the other one?
> if (chipcount > 1) {
> printf("Multiple flash chips were detected: \"%s\"",
> - flashes[0].name);
> + chip->name);
Can you please use flashes[0].chip->name here to keep the code
consistent? Besides that, do you have any idea why the current code is
so screwed up? Moving a printf for flashes[0] outside a perfectly
working for loop seems odd to me. It would be nice if you could just let
the loop below start at i=0 and remove the chip name part from the
printf above.
> for (i = 1; i < chipcount; i++)
> - printf(", \"%s\"", flashes[i].name);
> + printf(", \"%s\"", flashes[i].chip->name);
> printf("\nPlease specify which chip to use with the "
> "-c <chipname> option.\n");
> ret = 1;
> @@ -464,7 +467,7 @@ int main(int argc, char *argv[])
> /* This loop just counts compatible controllers. */
> for (j = 0; j < registered_programmer_count; j++) {
> pgm = ®istered_programmers[j];
> - if (pgm->buses_supported & flashes[0].bustype)
> + if (pgm->buses_supported & chip->bustype)
flashes[0].chip->bustype please.
Note to self: This code needs to be audited again if it should really
run against flashes[0] here. It looks odd, at least when looking only at
the patch.
> compatible_programmers++;
> }
> if (compatible_programmers > 1)
> @@ -491,19 +494,17 @@ int main(int argc, char *argv[])
> goto out_shutdown;
> } else if (!chip_to_probe) {
> /* repeat for convenience when looking at foreign logs */
> - tempstr = flashbuses_to_text(flashes[0].bustype);
> + tempstr = flashbuses_to_text(chip->bustype);
> msg_gdbg("Found %s flash chip \"%s\" (%d kB, %s).\n",
> - flashes[0].vendor, flashes[0].name,
> - flashes[0].total_size, tempstr);
> + chip->vendor, chip->name,
> + chip->total_size, tempstr);
And the flashes[0].chip dance again.
> free(tempstr);
> }
>
> - fill_flash = &flashes[0];
> -
> - check_chip_supported(fill_flash);
> + check_chip_supported(chip);
>
> - size = fill_flash->total_size * 1024;
> - if (check_max_decode(fill_flash->pgm->buses_supported & fill_flash->bustype, size) &&
> + size = chip->total_size * 1024;
> + if (check_max_decode(fill_flash->pgm->buses_supported & chip->bustype, size) &&
> (!force)) {
> fprintf(stderr, "Chip is too big for this programmer "
> "(-V gives details). Use --force to override.\n");
> diff --git a/flash.h b/flash.h
> index 0dac13d..ed64512 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -87,6 +87,7 @@ enum chipbustype {
> #define FEATURE_WRSR_EITHER (FEATURE_WRSR_EWSR | FEATURE_WRSR_WREN)
>
> struct flashctx;
> +typedef int (erasefunc_t)(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
>
> struct flashchip {
> const char *vendor;
> @@ -135,7 +136,7 @@ struct flashchip {
> } eraseblocks[NUM_ERASEREGIONS];
> /* a block_erase function should try to erase one block of size
> * 'blocklen' at address 'blockaddr' and return 0 on success. */
> - int (*block_erase) (struct flashctx *flash, unsigned int blockaddr, unsigned int blocklen);
> + erasefunc_t *block_erase;
I really really hate that typedef. I wonder if we could keep the code
here and use typeof() instead of the typedef elsewhere.
> } block_erasers[NUM_ERASEFUNCTIONS];
>
> int (*printlock) (struct flashctx *flash);
> @@ -148,35 +149,14 @@ struct flashchip {
> } voltage;
> };
>
> -/* struct flashctx must always contain struct flashchip at the beginning. */
> struct flashctx {
> - const char *vendor;
> - const char *name;
> - enum chipbustype bustype;
> - uint32_t manufacture_id;
> - uint32_t model_id;
> - int total_size;
> - int page_size;
> - int feature_bits;
> - uint32_t tested;
> - int (*probe) (struct flashctx *flash);
> - int probe_timing;
> - struct block_eraser block_erasers[NUM_ERASEFUNCTIONS];
> - int (*printlock) (struct flashctx *flash);
> - int (*unlock) (struct flashctx *flash);
> - int (*write) (struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len);
> - int (*read) (struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len);
> - struct voltage voltage;
> - /* struct flashchip ends here. */
> -
> + struct flashchip *chip;
Yay!
> chipaddr virtual_memory;
> /* Some flash devices have an additional register space. */
> chipaddr virtual_registers;
> struct registered_programmer *pgm;
> };
>
> -typedef int (erasefunc_t)(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
> -
> #define TEST_UNTESTED 0
>
> #define TEST_OK_PROBE (1 << 0)
> diff --git a/flashrom.c b/flashrom.c
> index cad043b..0b6cca3 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -522,7 +522,7 @@ char *extract_programmer_param(const char *param_name)
> }
>
> /* Returns the number of well-defined erasers for a chip. */
> -static unsigned int count_usable_erasers(const struct flashctx *flash)
> +static unsigned int count_usable_erasers(const struct flashchip *flash)
Do we want to count usable erasers for the current programmer (for chips
with erase functions limited to certain buses)? If yes, please keep
struct flashctx.
> {
> unsigned int usable_erasefunctions = 0;
> int k;
> @@ -941,32 +942,38 @@ int check_max_decode(enum chipbustype buses, uint32_t size)
> int probe_flash(struct registered_programmer *pgm, int startchip,
> struct flashctx *fill_flash, int force)
> {
> - const struct flashchip *flash;
> + const struct flashchip *chip;
> unsigned long base = 0;
> char location[64];
> uint32_t size;
> enum chipbustype buses_common;
> char *tmp;
>
> - for (flash = flashchips + startchip; flash && flash->name; flash++) {
> - if (chip_to_probe && strcmp(flash->name, chip_to_probe) != 0)
> + for (chip = flashchips + startchip; chip && chip->name; chip++) {
> + if (chip_to_probe && strcmp(chip->name, chip_to_probe) != 0)
> continue;
> - buses_common = pgm->buses_supported & flash->bustype;
> + buses_common = pgm->buses_supported & chip->bustype;
> if (!buses_common)
> continue;
> msg_gdbg("Probing for %s %s, %d kB: ",
> - flash->vendor, flash->name, flash->total_size);
> - if (!flash->probe && !force) {
> + chip->vendor, chip->name, chip->total_size);
> + if (!chip->probe && !force) {
> msg_gdbg("failed! flashrom has no probe function for "
> "this flash chip.\n");
> continue;
> }
>
> - size = flash->total_size * 1024;
> + size = chip->total_size * 1024;
> check_max_decode(buses_common, size);
>
> /* Start filling in the dynamic data. */
> - memcpy(fill_flash, flash, sizeof(struct flashchip));
> + /* FIXME: when to free? */
Tricky, but I think we are lucky here because there is only one path
which returns a non-match and that's the only path where we want to
free. I have annotated that place a few hunks below.
> + fill_flash->chip = malloc(sizeof(struct flashchip));
> + if (fill_flash->chip == NULL) {
> + msg_gerr("%s: out of memory.\n", __func__);
> + return -1;
Can we return an explicit code for OOM here or would that screw up the
caller?
> + }
> + memcpy(fill_flash->chip, chip, sizeof(struct flashchip));
> fill_flash->pgm = pgm;
>
> base = flashbase ? flashbase : (0xffffffff - size + 1);
> @@ -985,11 +992,11 @@ int probe_flash(struct registered_programmer *pgm, int startchip,
> * one for this programmer interface and thus no other chip has
> * been found on this interface.
> */
> - if (startchip == 0 && fill_flash->model_id == SFDP_DEVICE_ID) {
> + if (startchip == 0 && chip->model_id == SFDP_DEVICE_ID) {
We used fill_flash for consistency (i.e. only use the rw copy of the
flashchip entry) here, and if we want to keep that consistency, it's
fill_flash->chip->model_id.
> msg_cinfo("===\n"
> "SFDP has autodetected a flash chip which is "
> "not natively supported by flashrom yet.\n");
> - if (count_usable_erasers(fill_flash) == 0)
> + if (count_usable_erasers(chip) == 0)
See the count_usable_erasers comment a few hunks above.
> msg_cinfo("The standard operations read and "
> "verify should work, but to support "
> "erase, write and all other "
> @@ -1010,15 +1017,15 @@ int probe_flash(struct registered_programmer *pgm, int startchip,
> }
>
> if (startchip == 0 ||
> - ((fill_flash->model_id != GENERIC_DEVICE_ID) &&
> - (fill_flash->model_id != SFDP_DEVICE_ID)))
> + ((chip->model_id != GENERIC_DEVICE_ID) &&
> + (chip->model_id != SFDP_DEVICE_ID)))
See above for fill_flash->chip->model_id.
> break;
>
> notfound:
> programmer_unmap_flash_region((void *)fill_flash->virtual_memory, size);
> }
>
> - if (!flash || !flash->name)
> + if (!chip || !chip->name)
Here you have to free flash->chip. No other freeing is needed AFAICS.
> return -1;
>
> #if CONFIG_INTERNAL == 1
> @@ -1028,27 +1035,27 @@ notfound:
> #endif
> snprintf(location, sizeof(location), "on %s", programmer_table[programmer].name);
>
> - tmp = flashbuses_to_text(flash->bustype);
> + tmp = flashbuses_to_text(chip->bustype);
fill_flash->chip->...
> msg_cinfo("%s %s flash chip \"%s\" (%d kB, %s) %s.\n",
> - force ? "Assuming" : "Found", fill_flash->vendor,
> - fill_flash->name, fill_flash->total_size, tmp, location);
> + force ? "Assuming" : "Found", chip->vendor,
> + chip->name, chip->total_size, tmp, location);
dito
> free(tmp);
>
> /* Flash registers will not be mapped if the chip was forced. Lock info
> * may be stored in registers, so avoid lock info printing.
> */
> if (!force)
> - if (fill_flash->printlock)
> - fill_flash->printlock(fill_flash);
> + if (chip->printlock)
dito
> + chip->printlock(fill_flash);
dito
>
> /* Return position of matching chip. */
> - return flash - flashchips;
> + return chip - flashchips;
> }
>
> int verify_flash(struct flashctx *flash, uint8_t *buf)
> @@ -1308,7 +1315,7 @@ static int walk_eraseregions(struct flashctx *flash, int erasefunction,
> return 0;
> }
>
> -static int check_block_eraser(const struct flashctx *flash, int k, int log)
> +static int check_block_eraser(const struct flashchip *flash, int k, int log)
No conversion, please. See below.
> {
> struct block_eraser eraser = flash->block_erasers[k];
>
> @@ -1357,7 +1366,7 @@ int erase_and_write_flash(struct flashctx *flash, uint8_t *oldcontents,
> break;
> }
> msg_cdbg("Trying erase function %i... ", k);
> - if (check_block_eraser(flash, k, 1))
> + if (check_block_eraser(chip, k, 1))
This one is debatable. We know that some chips support certain erase
functions only on some buses, e.g. Sharp LHF00L04 which can do a
full-chip erase only if it is accesses in A/A Mux mode, not in FWH mode.
My mid-term plan is to augment erase functions (and possibly also
read/write/probe) with a list of valid buses for any given function.
> continue;
> usable_erasefunctions--;
> ret = walk_eraseregions(flash, k, &erase_and_write_block_helper,
> @@ -1550,14 +1559,6 @@ int selfcheck(void)
> msg_gerr("Flashchips table miscompilation!\n");
> ret = 1;
> }
> - /* Check that virtual_memory in struct flashctx is placed directly
> - * after the members copied from struct flashchip.
> - */
> - if (sizeof(struct flashchip) !=
> - offsetof(struct flashctx, virtual_memory)) {
> - msg_gerr("struct flashctx broken!\n");
> - ret = 1;
> - }
Glad to lose this hunk!
> for (flash = flashchips; flash && flash->name; flash++)
> if (selfcheck_eraseblocks(flash))
> ret = 1;
> @@ -1642,7 +1643,7 @@ void check_chip_supported(const struct flashctx *flash)
> /* FIXME: This function signature needs to be improved once doit() has a better
> * function signature.
> */
> -int chip_safety_check(struct flashctx *flash, int force, int read_it,
> +int chip_safety_check(const struct flashchip *flash, int force, int read_it,
Can you please make sure that all pointers to struct flashchip are
called "chip"? Otherwise grepping for "chip->" won't return all accesses
to struct flashchip. The same comment applies to other hunks of the
patch as well.
However, chip_safety_check can not be converted to a different calling
convention because we need struct flashctx here. Admittedly we don't
need it right now, but we will need it once programmer_may_write is part
of the programmer struct in struct flashctx.
> int write_it, int erase_it, int verify_it)
> {
> if (!programmer_may_write && (write_it || erase_it)) {
> @@ -1710,9 +1711,10 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it,
> uint8_t *oldcontents;
> uint8_t *newcontents;
> int ret = 0;
> - unsigned long size = flash->total_size * 1024;
> + const struct flashchip *chip = flash->chip;
> + unsigned long size = chip->total_size * 1024;
>
> - if (chip_safety_check(flash, force, read_it, write_it, erase_it, verify_it)) {
> + if (chip_safety_check(chip, force, read_it, write_it, erase_it, verify_it)) {
No conversion, please. See above.
> msg_cerr("Aborting.\n");
> ret = 1;
> goto out_nofree;
> @@ -1791,7 +1793,7 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it,
>
> // This should be moved into each flash part's code to do it
> // cleanly. This does the job.
> - handle_romentries(flash, oldcontents, newcontents);
> + handle_romentries(chip, oldcontents, newcontents);
No conversion, please. See two hunks below.
>
> // ////////////////////////////////////////////////////////////
>
> diff --git a/jedec.c b/jedec.c
> index 69c0c0c..5c549a2 100644
> --- a/jedec.c
> +++ b/jedec.c
> @@ -93,7 +93,7 @@ void data_polling_jedec(const struct flashctx *flash, chipaddr dst,
> msg_cdbg("%s: excessive loops, i=0x%x\n", __func__, i);
> }
>
> -static unsigned int getaddrmask(struct flashctx *flash)
> +static unsigned int getaddrmask(const struct flashchip *flash)
Hm yes, const makes sense. In fact, const makes sense in many more places.
> {
> switch (flash->feature_bits & FEATURE_ADDR_MASK) {
> case FEATURE_ADDR_FULL:
> diff --git a/layout.c b/layout.c
> index 90d3cce..f0f7a6f 100644
> --- a/layout.c
> +++ b/layout.c
> @@ -302,7 +302,7 @@ romlayout_t *get_next_included_romentry(unsigned int start)
> return best_entry;
> }
>
> -int handle_romentries(struct flashctx *flash, uint8_t *oldcontents, uint8_t *newcontents)
> +int handle_romentries(const struct flashchip *flash, uint8_t *oldcontents, uint8_t *newcontents)
Will we have to change this back once we try to merge the layout
patches? In that case, we need some way to get permissible regions from
the programmer, and that would probably be done via the programmer struct.
> {
> unsigned int start = 0;
> romlayout_t *entry;
> diff --git a/spi25.c b/spi25.c
> index b7e8189..2accb6d 100644
> --- a/spi25.c
> +++ b/spi25.c
> @@ -416,22 +416,23 @@ void spi_prettyprint_status_register_sst25vf040b(uint8_t status)
>
> int spi_prettyprint_status_register(struct flashctx *flash)
> {
> + const struct flashchip *chip = flash->chip;
const struct flashchip here, but just struct flashchip in the next hunk.
What's the reason? Same question applies to other hunks of the patch as
well.
> uint8_t status;
>
> status = spi_read_status_register(flash);
> msg_cdbg("Chip status register is %02x\n", status);
> - switch (flash->manufacture_id) {
> + switch (chip->manufacture_id) {
> case ST_ID:
> - if (((flash->model_id & 0xff00) == 0x2000) ||
> - ((flash->model_id & 0xff00) == 0x2500))
> + if (((chip->model_id & 0xff00) == 0x2000) ||
> + ((chip->model_id & 0xff00) == 0x2500))
> spi_prettyprint_status_register_st_m25p(status);
> break;
> case MACRONIX_ID:
> - if ((flash->model_id & 0xff00) == 0x2000)
> + if ((chip->model_id & 0xff00) == 0x2000)
> spi_prettyprint_status_register_st_m25p(status);
> break;
> case SST_ID:
> - switch (flash->model_id) {
> + switch (chip->model_id) {
> case 0x2541:
> spi_prettyprint_status_register_sst25vf016(status);
> break;
> @@ -862,16 +863,17 @@ static int spi_write_status_register_wren(struct flashctx *flash, int status)
>
> int spi_write_status_register(struct flashctx *flash, int status)
> {
> + struct flashchip *chip = flash->chip;
> int ret = 1;
>
> - if (!(flash->feature_bits & (FEATURE_WRSR_WREN | FEATURE_WRSR_EWSR))) {
> + if (!(chip->feature_bits & (FEATURE_WRSR_WREN | FEATURE_WRSR_EWSR))) {
> msg_cdbg("Missing status register write definition, assuming "
> "EWSR is needed\n");
> - flash->feature_bits |= FEATURE_WRSR_EWSR;
> + chip->feature_bits |= FEATURE_WRSR_EWSR;
> }
> - if (flash->feature_bits & FEATURE_WRSR_WREN)
> + if (chip->feature_bits & FEATURE_WRSR_WREN)
> ret = spi_write_status_register_wren(flash, status);
> - if (ret && (flash->feature_bits & FEATURE_WRSR_EWSR))
> + if (ret && (chip->feature_bits & FEATURE_WRSR_EWSR))
> ret = spi_write_status_register_ewsr(flash, status);
> return ret;
> }
I think the next iteration of this patch can go in.
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
AT49F010 patch is basically a copy of the existing AT49F020 code, but
with half the size and the correct ID.
The log was from a write of random data, after the chip already
contained different random data. I did separate read, erase, write tests
before but this log seems to show that PREW all work.
CAT28F512 I have successfully probed and read data from, but erase
fails. Could be because it needs 12v VPP. The CAT28F512 I have is
soldered on to a NIC. I haven't investigated if VPP is connected at all
yet. Thought I'd include the patch as-is anyway.
Both patches:
Signed-off-by: Andrew Morgan <ziltro(a)ziltro.com>
--
Andrew.
Enable writing/erasing on the ASUS P4P800-X, tested by mindcat on IRC.
This board enable is inspired by the board enable for the ASUS P4P800-VM.
As a bonus, testin the erasing/writing functionality on the SST
SST49LF003A/B chip is now known to work.
Signed-off-by: Idwer Vollering <vidwer(a)gmail.com>
---
Before (r1205/v0.9.3):
Write report: http://paste.flashrom.org/view.php?id=465
Write report: http://paste.flashrom.org/view.php?id=466
After (r1285):
Erase report: http://paste.flashrom.org/view.php?id=467
On Sat, Apr 28, 2012 at 01:30:53AM +0200, Stefan Tauner wrote:
> The PCI IDs are generic VIA IDs. Only Biostar IDs are those of the LOM, but
> that would not be a good choice for ID. Accept the generic IDs (+ DMI string)
> for now because this will be generic soonish(tm) anyway.
> + {0x1106, 0x3177, 0x1106, 0x3177, 0x1106, 0x3116, 0x1106, 0x3116, "^KM266-8235$", NULL, NULL, P3, "Biostar", "M7VIQ", 0, OK, w83697xx_memw_enable_2e},
"KM266-8235" is "VIA Northbridge marketing name"-"VIA Southbridge
marketing name with VT removed".
This DMI string is not unique at all.
Luc Verhaegen.
Hello All.
This patch simplifies processor_flash_enable function a bit by simplifying the
structure of the processor_enable.c. At least this patch removes more lines
than introdces w/o sacrificing the simplicity.
Signed-off-by: Peter Lemenkov <lemenkov(a)gmail.com>
---
processor_enable.c | 26 +++++++++-----------------
1 file changed, 9 insertions(+), 17 deletions(-)
diff --git a/processor_enable.c b/processor_enable.c
index d680f97..d7f421d 100644
--- a/processor_enable.c
+++ b/processor_enable.c
@@ -24,19 +24,6 @@
#include "flash.h"
#include "programmer.h"
-#if defined(__i386__) || defined(__x86_64__)
-
-int processor_flash_enable(void)
-{
- /* On x86, flash access is not processor specific except on
- * AMD Elan SC520, AMD Geode and maybe other SoC-style CPUs.
- * FIXME: Move enable_flash_cs5536 and get_flashbase_sc520 here.
- */
- return 0;
-}
-
-#else
-
#if defined (__MIPSEL__) && defined (__linux)
#include <stdio.h>
#include <string.h>
@@ -83,15 +70,20 @@ static int is_loongson(void)
int processor_flash_enable(void)
{
+ int ret = 1;
/* FIXME: detect loongson on FreeBSD and OpenBSD as well. */
#if defined (__MIPSEL__) && defined (__linux)
if (is_loongson()) {
flashbase = 0x1fc00000;
- return 0;
+ ret = 0;
}
+#elif defined(__i386__) || defined(__x86_64__)
+ /* On x86, flash access is not processor specific except on
+ * AMD Elan SC520, AMD Geode and maybe other SoC-style CPUs.
+ * FIXME: Move enable_flash_cs5536 and get_flashbase_sc520 here.
+ */
+ ret = 0;
#endif
/* Not implemented yet. Oh well. */
- return 1;
+ return ret;
}
-
-#endif
--
1.7.9.3
Am 03.03.2012 21:11 schrieb Stefan Tauner:
> Previously boards in the wiki were tagged either as working or as known
> bad. But we added support to various boards via board enables that were
> then never tested because the owners have not reported back. This can
> now be tagged with NT and is shown appropriately.
>
> Also, the underlying data structure indicating state was converted from
> macros to an enum while preserving original integer values.
>
> Because all lines specifying supported boards and laptops were touched
> anyway, this patch also re-indents them.
>
> ---
> TODO: change other occurrences to use it. wanted to get feedack first.
>
> Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
>
> diff --git a/print_wiki.c b/print_wiki.c
> index 377154d..9a9cd83 100644
> --- a/print_wiki.c
> +++ b/print_wiki.c
> @@ -136,9 +136,9 @@ static void wiki_helper(const char *devicetype, int cols,
> const struct board_match *b = board_matches;
>
> for (i = 0; boards[i].vendor != NULL; i++) {
> - if (boards[i].working)
> + if (boards[i].working == OK)
> boardcount_good++;
> - else
> + if (boards[i].working == BAD)
> boardcount_bad++;
You could replace that construct with a switch(), and it might make
sense to count untested boards as well.
> }
>
> @@ -171,7 +171,8 @@ static void wiki_helper(const char *devicetype, int cols,
> b[k].lb_vendor ? b[k].lb_vendor : "",
> b[k].lb_vendor ? ":" : "",
> b[k].lb_vendor ? b[k].lb_part : "",
> - (boards[i].working) ? "OK" : "No");
> + (boards[i].working == OK) ? "OK" :
> + (boards[i].working == NT) ? "?3" : "No");
The ?3 looks odd, but I assume you tested it.
>
> if (boards[i].note) {
> printf("<sup>%d</sup>\n", num_notes + 1);
> diff --git a/print.c b/print.c
> index 1fdeac7..544a846 100644
> --- a/print.c
> +++ b/print.c
> @@ -545,428 +545,427 @@ void print_supported(void)
> [...]
> - B("ZOTAC", "Fusion-ITX WiFi (FUSION350-A-E)", 1, NULL, NULL),
> - B("ZOTAC", "GeForce 8200", 1, "http://pden.zotac.com/index.php?page=shop.product_details&product_id=129&ca…", NULL),
> - B("ZOTAC", "H67-ITX WiFi (H67ITX-C-E)", 0, NULL, "Probing works (Winbond W25Q32, 4096 kB, SPI), but parts of the flash are problematic: descriptor is r/o (conforming to ICH reqs), ME region is locked."),
> - B("ZOTAC", "ZBOX HD-ID11", 1, "http://pdde.zotac.com/index.php?page=shop.product_details&product_id=240&ca…", NULL),
> + B("A-Trend", "ATC-6220", OK, "http://www.motherboard.cz/mb/atrend/atc6220.htm", NULL),
> + B("abit", "A-S78H", OK, "http://www.abit.com.tw/page/en/motherboard/motherboard_detail.php?pMODEL_NA…", NULL),
> + B("abit", "AN-M2", OK, "http://www.abit.com.tw/page/en/motherboard/motherboard_detail.php?DEFTITLE=…", NULL),
> + B("abit", "AV8", OK, "http://www.abit.com.tw/page/en/motherboard/motherboard_detail.php?DEFTITLE=…", NULL),
Is it possible that this patch made the board table one or two tabs
wider? The patch looks like that, and while I agree that some
files/sections should not have line length limits, adding another 16
columns of whitespace is something I'd like to avoid.
print_wiki related code is something I rarely touch (except for
programmer additions), so please don't expect in-depth reviews from me.
A cursory review suggests that the patch at least doesn't make the code
worse and I don't have any strong feelings about this code. If you feel
this patch is beneficial, I can send a weak Acked-by, more like Meh-by.
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
