Re: [flashrom] [PATCH] Revert parts of r1397

[Stefan Tauner]

On Wed, 03 Aug 2011 00:07:14 +0200
Carl-Daniel Hailfinger  wrote:
> Index: flashrom-revert_r1397_partially/ft2232_spi.c
>
> --- flashrom-revert_r1397_partially/ft2232_spi.c	(Revision 1402)
> +++ flashrom-revert_r1397_partially/ft2232_spi.c	(Arbeitskopie)
> @@ -246,17 +246,21 @@
>    			ftdic->error_str);
>    }
> if (ftdi_usb_reset(ftdic) < 0)
> if (ftdi_usb_reset(ftdic) < 0) {
> msg_perr("Unable to reset FTDI device\n");
> }
> if (ftdi_set_latency_timer(ftdic, 2) < 0)
> if (ftdi_set_latency_timer(ftdic, 2) < 0) {
> msg_perr("Unable to set latency timer\n");
> }
> if (ftdi_write_data_set_chunksize(ftdic, 256))
> if (ftdi_write_data_set_chunksize(ftdic, 256)) {
> msg_perr("Unable to set chunk size\n");
> }
> if (ftdi_set_bitmode(ftdic, 0x00, BITMODE_BITBANG_SPI) < 0)
> if (ftdi_set_bitmode(ftdic, 0x00, BITMODE_BITBANG_SPI) < 0) {
> msg_perr("Unable to set bitmode to SPI\n");
> }

those could be skipped until we really need them.
but since you have done the work already i think it is ok.
> Index: flashrom-revert_r1397_partially/chipset_enable.c
> […]
> @@ -1028,8 +1029,8 @@
>    		flashbase = parx << 12;
>    	}
>    } else {
> msg_pinfo("AMD Elan SC520 detected, but no BOOTCS. "
> 	  "Assuming flash at 4G\n");
> msg_pinfo("AMD Elan SC520 detected, but no BOOTCS. Assuming "
> 	  "flash at 4G.\n");
> }

i also like breaking full sentences like it was done here previously.
filling the whole line just because we can does not mean it is useful.
reading one sentence per line is much more natural and as long as the
line count does not change i prefer it.
> Index: flashrom-revert_r1397_partially/board_enable.c
> […]
> dev = pci_dev_find(0x10DE, 0x0050);	/* NVIDIA CK804 ISA bridge. */
> dev = pci_dev_find(0x10DE, 0x0050);	/* NVIDIA CK804 ISA Bridge. */

imo it should be bridge. there is all kind of crap in datasheets. this
includes orthographic nonsense like this. but we have our own quirks
too... so i dont really care :)
> if (!dev) {
>    	msg_perr("\nERROR: NVIDIA nForce4 ISA bridge not found.\n");
>    	return -1;
> @@ -860,7 +860,7 @@
>    	return -1;
>    }
> /* First, check the ISA bridge */
> /* First, check the ISA Bridge */

First, check (or look) _for_ the ISA [Bb]ridge?
> […]
>    if (!dev) {
> msg_perr("\nERROR: No known Intel LPC bridge found.\n");
> msg_perr("\nERROR: No Known Intel LPC Bridge found.\n");

well we can discuss (or not) about [Bb]ridge, but that 'Known' slipped
in i hope? :)
> […]
> -static const struct board_pciid_enable *board_match_coreboot_name(
> 			const char *vendor, const char *part)
>
> +static const struct board_pciid_enable *board_match_coreboot_name(const char *vendor,
> 					    const char *part)

both ugly :)

some comments apply of course to all other instances of the discussed
problem too.

Acked-by: Stefan Tauner