[flashrom] [PATCH] Revert parts of r1397

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Wed Aug 3 01:51:37 CEST 2011


Am 03.08.2011 01:27 schrieb Stefan Tauner:
> On Wed, 03 Aug 2011 00:07:14 +0200
> Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> 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 <stefan.tauner at student.tuwien.ac.at>
>   

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/





More information about the flashrom mailing list