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

[Carl-Daniel Hailfinger]

Am 03.08.2011 01:27 schrieb 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.

OK. My point was about adding a . at the end of the second sentence.
> > 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 :)

I think Uwe was the one who wanted "ASUS" instead of "Asus", so I
thought the "Bridge"->"bridge" conversion was an unintended mistake on
his side.
Then again, I don't really care about capitalization here.

Uwe?
> > 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?

/* Check for the ISA bridge first. */
> > […]
> >    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? :)

I was reading though the reversed patch and missed that, thanks for the
hint.
> > […]
> > -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 :)

Indeed. Unfortunately I didn't find any beautiful solution which fit in
80 columns. The second line should be moved a bit to the right, but then
we violate the 80 column limit even more.
Ideas anyone?
> some comments apply of course to all other instances of the discussed
> problem too.
>
> Acked-by: Stefan Tauner

Thanks for the review.

I'l wait a bit so Uwe has a chance to comment as well.

Regards,
Carl-Daniel