Am 03.08.2011 01:27 schrieb Stefan Tauner:
> On Wed, 03 Aug 2011 00:07:14 +0200
> Carl-Daniel Hailfinger <[email protected]> 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 <[email protected]>
>
Thanks for the review.
I'l wait a bit so Uwe has a chance to comment as well.
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
_______________________________________________
flashrom mailing list
[email protected]
http://www.flashrom.org/mailman/listinfo/flashrom
